kubectl prune v2: switch to contains-group-kinds annotation #118942

justinsb · 2023-06-28T15:29:43Z

The contains-group-resources was an implementation error, we specified
contains-group-kinds in the KEP.

Because it is in alpha, we simply switch to the new annotation.


kubectl prune v2: Switch annotation from `contains-group-resources` to `contains-group-kinds`,
because this is what we defined in the KEP and is clearer to end-users.  Although the functionality is
in alpha, we will recognize the prior annotation; this migration support will be removed in beta/GA.

justinsb · 2023-06-28T15:29:53Z

/wip

justinsb · 2023-06-29T20:39:01Z

staging/src/k8s.io/kubectl/pkg/cmd/apply/applyset.go

 if !ok {
+ if annotations[DeprecatedApplySetGRsAnnotation] != "" {


Given this functionality is in alpha / feature-flagged, I don't know if we want to be compatible with the existing applysets. Three obvious options:

Do this and drop it after a few releases

Implement fsck that can map from the old annotation to the new one (we proposed this tool in our KEP, it will be handy anyway)

Just switch to the new annotation, it's alpha after all.

I have no preference for which one you choose, since there are not many apply sets out there now. This one seems fine to me.

justinsb · 2023-07-06T17:05:40Z

/retest

Looks like a timeout and an fsnotify issue, unrelated (?)

justinsb · 2023-07-06T17:06:52Z

/kind bugfix

k8s-ci-robot · 2023-07-06T17:06:54Z

@justinsb: The label(s) kind/bugfix cannot be applied, because the repository doesn't have them.

In response to this:

/kind bugfix

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

justinsb · 2023-07-06T17:08:41Z

/kind bug

justinsb · 2023-07-08T14:09:40Z

/retest

"Services [It] should not be able to connect to terminating and unready endpoints if PublishNotReadyAddresses is false" is unrelated (hopefully)

justinsb · 2023-07-18T11:16:23Z

@soltysh can we get this fix in before code freeze? I don't think too many people are yet using the feature, and I'm also a little reluctant to start evangelizing it when we know we have to change one of the core annotations...

seans3

This seems good to me. You mention

regression in that we can no longer apply manifests which contain both a (new) CRD
and an instance of that CRD in one "kubectl apply", when using prune-v2.

Should there be a test (possibly integration or e2e) to validate this regression is fixed, and to keep it from possibly returning?

seans3 · 2023-08-02T19:58:04Z

staging/src/k8s.io/kubectl/pkg/cmd/apply/applyset.go

 if !ok {
+ if annotations[DeprecatedApplySetGRsAnnotation] != "" {


I have no preference for which one you choose, since there are not many apply sets out there now. This one seems fine to me.

justinsb · 2023-08-14T15:24:38Z

Should there be a test (possibly integration or e2e) to validate this regression is fixed, and to keep it from possibly returning?

Good call, and I tried implementing the test. I was wrong though, kubectl "normal apply" isn't really able to reliably support this scenario either. Sometimes it works, but the deciding factor seems to be if we have a prior version of the GV in the discovery cache, rather than it being a race as I had previously thought. So it's not well-defined behaviour, and probably (?) not a regression, and it's very difficult to write a test because the behaviour isn't straightforward.

I'm happy to try to fix the behavior (in a separate PR), but I suspect it would be difficult to do as a non-breaking change (at least for some edge scenarios). This does support the idea you (@seans3) brought up in sig-cli last week though, that we should investigate doing an apply that is able to fix all these gotchas without risking compatibility, using a plugin or a new command.

I think this PR is still the correct behaviour for prunev2, in that (1) it is what we agreed in the KEP and (2) it will be an issue if we fix the general apply undefined behaviour here. But I don't think it is a regression.

Is it OK if I update the commit message etc to just reference the KEP and reword from regression to "unblock applying a CRD and a CR in the same manifest" or something like that?

Simple test in case anyone wants to try it out:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: things.example.com
spec:
  group: example.com
  names:
    kind: Thing
    listKind: ThingList
    plural: things
    singular: thing
  scope: Cluster
  versions:
  - name: v1
    served: true
    storage: true
    schema:
      openAPIV3Schema:
        type: object
        properties:
          spec:
            type: object

---

apiVersion: example.com/v1
kind: Thing
metadata:
  name: something
spec: {}

justinsb · 2023-08-30T15:14:48Z

I reworded the release note and commit messages to de-emphasize the regression, because it seems that kubectl apply can't actually reliably apply a CRD and an instance of the CRD in the same manifest, so it's not really a regression.

I also think we should recognize the existing annotation just so we don't have to worry about the edge-cases here, but I marked the old annotation as deprecated and suggested it could be removed as part of beta or GA of applyset, so we don't have to support it forever.

justinsb · 2023-09-15T13:33:41Z

/test pull-kubernetes-e2e-gce

The contains-group-resources was an implementation error, we specified contains-group-kinds in the KEP. Because it is in alpha, we simply switch to the new annotation. We will recognize the old annotation and migrate existing alpha applysets, but support for this migration can be removed in beta/GA of applyset.

justinsb · 2023-09-15T16:37:47Z

Looks like rebasing cleaned up the pull-kubernetes-e2e-gce test failures.

@seans3 are we OK to get this in?

seans3 · 2023-09-15T17:06:30Z

/approve
/lgtm
/triage accepted

k8s-ci-robot · 2023-09-15T17:06:37Z

LGTM label has been added.

Git tree hash: f63b98e7b413b12f5742c669f17fb63728117846

k8s-ci-robot · 2023-09-15T17:07:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, seans3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/kubectl/OWNERS~~ [seans3]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 28, 2023

k8s-ci-robot requested review from mpuckett159 and seans3 June 28, 2023 15:30

justinsb changed the title ~~kubectl apply v2: switch to contains-group-kinds~~ kubectl prune v2: switch to contains-group-kinds annotation Jun 29, 2023

justinsb force-pushed the switch_kubectl_prune_annotation branch 2 times, most recently from f5735ba to fe80a1a Compare June 29, 2023 20:37

justinsb commented Jun 29, 2023

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 6, 2023

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jul 6, 2023

seans3 reviewed Aug 2, 2023

View reviewed changes

justinsb force-pushed the switch_kubectl_prune_annotation branch from fe80a1a to 9d57700 Compare August 30, 2023 15:13

justinsb force-pushed the switch_kubectl_prune_annotation branch from 9d57700 to 10caecb Compare September 15, 2023 15:28

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 15, 2023

k8s-ci-robot assigned seans3 Sep 15, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2023

seans3 approved these changes Sep 15, 2023

View reviewed changes

k8s-ci-robot merged commit 963400f into kubernetes:master Sep 15, 2023
14 checks passed

k8s-ci-robot added this to the v1.29 milestone Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubectl prune v2: switch to contains-group-kinds annotation #118942

kubectl prune v2: switch to contains-group-kinds annotation #118942

justinsb commented Jun 28, 2023 •

edited

justinsb commented Jun 28, 2023

justinsb Jun 29, 2023

seans3 Aug 2, 2023

justinsb commented Jul 6, 2023

justinsb commented Jul 6, 2023

k8s-ci-robot commented Jul 6, 2023

justinsb commented Jul 6, 2023

justinsb commented Jul 8, 2023

justinsb commented Jul 18, 2023

seans3 left a comment

seans3 Aug 2, 2023

justinsb commented Aug 14, 2023

justinsb commented Aug 30, 2023

justinsb commented Sep 15, 2023

justinsb commented Sep 15, 2023

seans3 commented Sep 15, 2023

k8s-ci-robot commented Sep 15, 2023

k8s-ci-robot commented Sep 15, 2023

		if !ok {
		if annotations[DeprecatedApplySetGRsAnnotation] != "" {

kubectl prune v2: switch to contains-group-kinds annotation #118942

kubectl prune v2: switch to contains-group-kinds annotation #118942

Conversation

justinsb commented Jun 28, 2023 • edited

justinsb commented Jun 28, 2023

justinsb Jun 29, 2023

Choose a reason for hiding this comment

seans3 Aug 2, 2023

Choose a reason for hiding this comment

justinsb commented Jul 6, 2023

justinsb commented Jul 6, 2023

k8s-ci-robot commented Jul 6, 2023

justinsb commented Jul 6, 2023

justinsb commented Jul 8, 2023

justinsb commented Jul 18, 2023

seans3 left a comment

Choose a reason for hiding this comment

seans3 Aug 2, 2023

Choose a reason for hiding this comment

justinsb commented Aug 14, 2023

justinsb commented Aug 30, 2023

justinsb commented Sep 15, 2023

justinsb commented Sep 15, 2023

seans3 commented Sep 15, 2023

k8s-ci-robot commented Sep 15, 2023

k8s-ci-robot commented Sep 15, 2023

justinsb commented Jun 28, 2023 •

edited