Applyset dry run tests + ID value #116265

KnVerey · 2023-03-03T23:08:39Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This is part of a series of PRs to implement the ApplySet KEP in kubectl: http://git.k8s.io/enhancements/keps/sig-cli/3659-kubectl-apply-prune.

This one fixes a few of the small tasks on our new project board: https://github.com/orgs/kubernetes/projects/128/views/1

Related PRs:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

-->

- [KEP]: https://github.com/kubernetes/enhancements/tree/42a4b1c8a18780cf1d5c2460113d59b246002548/keps/sig-cli/3659-kubectl-apply-prune

/cc @justinsb @soltysh
/sig cli

KnVerey · 2023-03-03T23:09:00Z

/hold

This is based on #116243, which is pending merge

k8s-ci-robot · 2023-03-03T23:09:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KnVerey

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~~ [KnVerey]

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

KnVerey · 2023-03-03T23:11:38Z

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

+	if a.parentRef.IsNamespaced() {
+		unencoded = strings.Join([]string{a.parentRef.Name, a.parentRef.Namespace, a.parentRef.GroupVersionKind.Kind, a.parentRef.GroupVersionKind.Group}, applySetIDPartDelimiter)
+	} else {
+		unencoded = strings.Join([]string{a.parentRef.Name, a.parentRef.GroupVersionKind.Kind, a.parentRef.GroupVersionKind.Group}, applySetIDPartDelimiter)


Is this what we want for non-namespaced? The omission of a segment + use of a delimiter that appears inside the last segment could make correct parsing of the value difficult... but AFAIK tooling should only ever reconstruct this for verification, not attempt to parse it. Wdyt?

Good question. I'd argue for including the empty namespace (i.e. be consistent even if less aesthetically pleasing), because no human is going to notice the empty value once it goes through sha256/base64, and if we ever do need to parse it, then the ambiguous form is going to cause us trouble.

I think we might need to / want to parse it in our fsck command. But otherwise, yes I agree it should not be parsed.

This is not a strongly held opinion - if you want to go the other way that works too!

Yeah, I agree with Justin here, just make it consistent by concatenating all the elements, empty namespace is equally valid, and this will make the code easier to read as well 😄

KnVerey · 2023-03-03T23:12:15Z

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

+	encoder := base64.NewEncoder(base64.URLEncoding, &w)
+	_, err := encoder.Write([]byte(unencoded))
+	if err != nil {
+		klog.Fatalf("failed to encode parent ID %s: %w", unencoded, err)


Is this unlikely enough that the panic is ok? Otherwise we'll need error handling everywhere we use the ID.

Can we use EncodeToString? Then you avoid the issue (or rather let the base64 library make the assumption!) Your assumption is valid here though, I believe.

It looks like the sha256 is also missing BTW, and there I would similarly use Sum256 to avoid error handling

KnVerey · 2023-03-03T23:13:15Z

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

@@ -384,7 +400,7 @@ func generateResourcesAnnotation(resources sets.Set[schema.GroupVersionResource]
 }

 func (a ApplySet) FieldManager() string {
-	return fmt.Sprintf("%s-applyset-%s", a.toolingID.name, a.ID()) // TODO: validate this choice
+	return fmt.Sprintf("%s-applyset", a.toolingID.name)


I'm thinking that the inclusion of the ApplySet ID in the manager name was overly specific, particularly since this is only used to patch the parent itself (so it is a self-reference). We should make sure we all agree on what to do here, since changing it after anyone has tried this on a real object will be disruptive.

Agreed that we need to think about it early. I think the more valuable one to tie back to an applyset is the "child objects" - do we get to pick the field manager there (when using SSA)? Or can we annotate in some way? If we could explicitly that field manager back to say "these are the objects managed in this applyset, and these are the specific fields managed", then I think that would really be making SSA work for us...

In theory we could use a separate field manager of our choice when setting our labels on the children instead of adding them to the object prior to the main operation. However, the field manager is set per-request: PATCH https://127.0.0.1:51962/apis/company.com/v1/applysets/my-set?fieldManager=kubectl-applyset&fieldValidation=Strict&force=false <-- note the first query param. So that would necessarily double the number of patches sent during an apply, which I suspect is unacceptable / not worth the benefit.

We could override the field manager name used for the main operation to be some value related to ApplySet, but that would have some undesired effects. First, there's CSA<->SSA migration/compatibility code (including on the server) that is triggered when the default names are used, so if we override the names, then ApplySet use makes a CSA->SSA upgrade non-rollback-safe. Second, the field manager name is exposed as a flag, so the user can override it.

Another thing I realized in writing this out is that we can't use the field manager here as in the main operation if we want to leave the door open to having the parent present in the main set. Allowing the parent to be in the main set would make ApplySet easier to use with custom resources, which may have required fields we can't guess values for. And if we used the same field manager name here, we'd effectively be requesting deletion of those extra fields.

Another option that occurred to me is using the field manager name from the main operation as the prefix, instead of the tooling name. That could work for SSA and for the case where the user overrides the name, but CSA has a different default name, and we don't want a CSA->SSA migration to have any effect on this (plus the CSA name literally has '-client-side-apply` in it, which is inaccurate for us).

Good points all. I was thinking about the "main" apply. TBH my instinct was to have one field manager name for everything we apply, though I do now see that will introduce some very tricky edge cases for the parent.

I think CSA -> SSA means that we can't really mess with this, unless we were willing to make prunev2 SSA-only.
Maybe we should think about - easy enough to relax later and might avoid opening some as-yet-unknown cans of worms?

KnVerey · 2023-03-03T23:23:50Z

/unhold
Rebased and ready fo review!

/triage accepted
/priority important-longterm

KnVerey · 2023-03-06T18:54:30Z

/test pull-kubernetes-conformance-kind-ga-only-parallel

justinsb · 2023-03-08T04:23:10Z

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

@@ -2361,7 +2361,7 @@ metadata:
    applyset.k8s.io/tooling: kubectl/v0.0.0-master+$Format:%H$
  creationTimestamp: null
  labels:
-    applyset.k8s.io/id: placeholder-todo
+    applyset.k8s.io/id: bXlTZXQudGVzdC5TZWNyZXQu


This value looks pretty reasonable (but is maybe too short?). I just checked the label conditions and spotted "unless empty, must begin and end with an alphanumeric character ([a-z0-9A-Z])," https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/

So we should add a consistent prefix and suffix, or only add them when the first/last character is the one of the two "odd" base64 characters (+/ or -_)

Maybe something like "as-" or even applyset-
We could also use base32 if that's easier, but personally I prefer a constant prefix (the suffix I find less useful, but also nobody is going to notice if we put an extra a on the end).

I agree probably nobody would notice an extra "a" (or any other letter), but in the PR you made to my PR, you used -1. That looks like a potential sequence... is that intentional? Maybe another option could be a version suffix like v1 or something? Because tools are supposed to validate the values of this, we're going to have to dictate this suffix/prefix too I assume.

Yes, I was thinking if we have to add something, we might as well add some sort of version identifier. That could be "-v1", "-v2" (as I think you did), that could be -a, -b etc if we wanted to make it a little less explicit. I like -v1 and as we have to dictate what to do if the last value of the base64 encoding is not alphanumeric, so I suggest we use this for a version marker (even if we never use it)

Yep, v1 is what I went with!

justinsb · 2023-03-08T04:28:16Z

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

+	if a.parentRef.IsNamespaced() {
+		unencoded = strings.Join([]string{a.parentRef.Name, a.parentRef.Namespace, a.parentRef.GroupVersionKind.Kind, a.parentRef.GroupVersionKind.Group}, applySetIDPartDelimiter)
+	} else {
+		unencoded = strings.Join([]string{a.parentRef.Name, a.parentRef.GroupVersionKind.Kind, a.parentRef.GroupVersionKind.Group}, applySetIDPartDelimiter)


Good question. I'd argue for including the empty namespace (i.e. be consistent even if less aesthetically pleasing), because no human is going to notice the empty value once it goes through sha256/base64, and if we ever do need to parse it, then the ambiguous form is going to cause us trouble.

I think we might need to / want to parse it in our fsck command. But otherwise, yes I agree it should not be parsed.

This is not a strongly held opinion - if you want to go the other way that works too!

justinsb · 2023-03-08T04:29:45Z

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

+	encoder := base64.NewEncoder(base64.URLEncoding, &w)
+	_, err := encoder.Write([]byte(unencoded))
+	if err != nil {
+		klog.Fatalf("failed to encode parent ID %s: %w", unencoded, err)


Can we use EncodeToString? Then you avoid the issue (or rather let the base64 library make the assumption!) Your assumption is valid here though, I believe.

It looks like the sha256 is also missing BTW, and there I would similarly use Sum256 to avoid error handling

justinsb · 2023-03-08T04:34:59Z

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

@@ -384,7 +400,7 @@ func generateResourcesAnnotation(resources sets.Set[schema.GroupVersionResource]
 }

 func (a ApplySet) FieldManager() string {
-	return fmt.Sprintf("%s-applyset-%s", a.toolingID.name, a.ID()) // TODO: validate this choice
+	return fmt.Sprintf("%s-applyset", a.toolingID.name)


Agreed that we need to think about it early. I think the more valuable one to tie back to an applyset is the "child objects" - do we get to pick the field manager there (when using SSA)? Or can we annotate in some way? If we could explicitly that field manager back to say "these are the objects managed in this applyset, and these are the specific fields managed", then I think that would really be making SSA work for us...

justinsb · 2023-03-08T04:40:30Z

Looks good (though I think we need a sha256 and probably a prefix & suffix).

The question re field manager is really interesting - it would be really powerful to be able to tie the fields back to the applyset IMO. I think previously we have used a generic field manager name ("kubectl"?), which doesn't really help.

We don't have any other fields that we can (obviously) set:
https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.26/#managedfieldsentry-v1-meta

Obvious answers are something like kubectl:<applysetid> or even just applyset:<applysetid> . I guess technically we can parse the managed fields and infer which FieldManager is managing the applyset annotation, but that is a lot more complicated.

soltysh

One comment, otherwise this is good as is.

soltysh · 2023-03-13T12:44:49Z

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

+	if a.parentRef.IsNamespaced() {
+		unencoded = strings.Join([]string{a.parentRef.Name, a.parentRef.Namespace, a.parentRef.GroupVersionKind.Kind, a.parentRef.GroupVersionKind.Group}, applySetIDPartDelimiter)
+	} else {
+		unencoded = strings.Join([]string{a.parentRef.Name, a.parentRef.GroupVersionKind.Kind, a.parentRef.GroupVersionKind.Group}, applySetIDPartDelimiter)


Yeah, I agree with Justin here, just make it consistent by concatenating all the elements, empty namespace is equally valid, and this will make the code easier to read as well 😄

justinsb · 2023-03-13T20:25:42Z

Thanks - changes lgtm! Will apply lgtm whenever you're ready

KnVerey · 2023-03-14T00:35:16Z

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

@@ -419,8 +418,6 @@ func (o *ApplyOptions) Validate() error {
 				return fmt.Errorf("--selector is incompatible with --applyset")
 			} else if len(o.PruneResources) > 0 {
 				return fmt.Errorf("--prune-allowlist is incompatible with --applyset")
-			} else {
-				klog.Warning("WARNING: --prune --applyset is not fully implemented and does not yet prune any resources.")


We should have removed this in the last PR 🙈

KnVerey · 2023-03-14T00:37:54Z

/label tide/merge-method-squash

Updated, rebased and ready to go! 🤞

justinsb · 2023-03-14T09:33:06Z

Thanks @KnVerey

/lgtm

I think we can tackle the field manager owner separately.

k8s-ci-robot · 2023-03-14T09:33:14Z

LGTM label has been added.

Git tree hash: 6b621b978960ba2ccf200e536024aa44fc79325c

k8s-triage-robot · 2023-03-14T11:37:07Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

* Test for ApplySet with --dry-run=client|server * Use the real format for ApplySet ID * Incorporate feedback * Adjustments from rebase

k8s-ci-robot requested a review from justinsb March 3, 2023 23:08

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 3, 2023

k8s-ci-robot requested a review from soltysh March 3, 2023 23:08

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2023

k8s-ci-robot added area/kubectl approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 3, 2023

KnVerey commented Mar 3, 2023

View reviewed changes

KnVerey changed the title ~~Applyset dry run~~ Applyset dry run tests + ID value Mar 3, 2023

KnVerey force-pushed the applyset-dry-run branch from e4f49f3 to f18d1ba Compare March 3, 2023 23:23

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 3, 2023

justinsb reviewed Mar 8, 2023

View reviewed changes

KnVerey mentioned this pull request Mar 8, 2023

ApplySet: allow custom resources to be parent objects #116353

Merged

soltysh reviewed Mar 13, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2023

KnVerey added 4 commits March 13, 2023 20:26

Test for ApplySet with --dry-run=client|server

f01edc3

Use the real format for ApplySet ID

5f8a0f4

Incorporate feedback

ac98803

Adjustments from rebase

1a2f4e8

KnVerey force-pushed the applyset-dry-run branch from a876b29 to 1a2f4e8 Compare March 14, 2023 00:34

KnVerey commented Mar 14, 2023

View reviewed changes

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2023

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 14, 2023

k8s-ci-robot assigned justinsb Mar 14, 2023

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

k8s-ci-robot merged commit 6a31757 into kubernetes:master Mar 14, 2023

k8s-ci-robot added this to the v1.27 milestone Mar 14, 2023

KnVerey deleted the applyset-dry-run branch March 14, 2023 15:08

KnVerey mentioned this pull request Mar 15, 2023

ApplySet : kubectl apply --prune redesign and graduation strategy kubernetes/enhancements#3659

Open

8 tasks

rayowang pushed a commit to rayowang/kubernetes that referenced this pull request Feb 9, 2024

Applyset dry run tests + ID value (kubernetes#116265)

c3b61d1

* Test for ApplySet with --dry-run=client|server * Use the real format for ApplySet ID * Incorporate feedback * Adjustments from rebase

Applyset dry run tests + ID value #116265

Applyset dry run tests + ID value #116265

Uh oh!

Conversation

KnVerey commented Mar 3, 2023

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

KnVerey commented Mar 3, 2023

Uh oh!

k8s-ci-robot commented Mar 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KnVerey Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KnVerey commented Mar 3, 2023

Uh oh!

KnVerey commented Mar 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinsb commented Mar 8, 2023

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinsb commented Mar 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KnVerey commented Mar 14, 2023

Uh oh!

justinsb commented Mar 14, 2023

Uh oh!

k8s-ci-robot commented Mar 14, 2023

Uh oh!

k8s-triage-robot commented Mar 14, 2023

Uh oh!

Uh oh!

KnVerey Mar 3, 2023 •

edited

Loading