kubectl prunev2: Refactor the applyset to be more reusable #116580

justinsb · 2023-03-14T12:12:32Z

This enables sharing with diff.

/kind cleanup

NONE

KnVerey · 2023-03-14T16:28:28Z

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

 	}
 	return nil
 }

-func (p *applySetPruner) delete(ctx context.Context, namespace string, name string, mapping *meta.RESTMapping) error {
-	return runDelete(ctx, namespace, name, mapping, p.dynamicClient, p.cascadingStrategy, p.gracePeriod, p.dryRunStrategy == cmdutil.DryRunServer)
+func (a *ApplySet) delete(ctx context.Context, dynamicClient dynamic.Interface, namespace string, name string, mapping *meta.RESTMapping, o *ApplySetDeleteOptions) error {


nit: would this make more sense as a method on ApplySetDeleteOptions? It uses three of its values, and doesn't interact with ApplySet at all. Or maybe just call runDelete directly from deleteObjects unless I'm missing some value in the indirection here.

My 2c is that Options types should be POGO (like POJOs, but for Go!), but I do agree that we didn't need the indirection so I inlined delete 👍

KnVerey · 2023-03-14T16:30:32Z

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

+
+		if opt.DryRunStrategy != cmdutil.DryRunClient {
+			if err := a.delete(ctx, dynamicClient, namespace, name, mapping, opt); err != nil {
+				return fmt.Errorf("pruning %v %s/%s: %w", pruneObject.Mapping.GroupVersionKind.String(), namespace, name, err)


nit: maybe make a pruneObject.String() than could prevent the slash for non-namespaced objects? (e.g. Kind=Namespace /bar in the test)

KnVerey · 2023-03-14T16:35:26Z

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

-					return err
-				}
-				if err := pruner.pruneAll(ctx, o.ApplySet); err != nil {
+				if err := o.ApplySet.Prune(ctx, o); err != nil {


Should o.ApplySet.UpdateParent be merged into o.ApplySet.Prune as well (and then UpdateParent made private)? IIRC I originally had it that way but moved it out because the parent update went in before pruning was implemented at all. I don't think there's a conceivable case where pruning was completed but parent update should not be attempted?

Good call - done!

KnVerey · 2023-03-14T16:39:32Z

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

+
+// FindObjectsToPrune returns the list of objects that will be pruned.
+// Calling this instead of Prune can be useful for dry-run / diff behaviour.
+func (a *ApplySet) FindObjectsToPrune(ctx context.Context, dynamicClient dynamic.Interface, visitedUids sets.Set[types.UID]) ([]PruneObject, error) {


Why have FindObjectsToPrune that is public wrapping the very similarly named findAllObjectsToPrune that is private?

Good point, removed the extra layer.

KnVerey · 2023-03-14T16:43:37Z

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

@@ -457,3 +459,58 @@ func ParseApplySetParentRef(parentRefStr string, mapper meta.RESTMapper) (*Apply
 	}
 	return &ApplySetParentRef{Name: name, RESTMapping: mapping}, nil
 }
+
+// Prune deletes any objects from the apiserver that are no longer in the applyset.
+func (a *ApplySet) Prune(ctx context.Context, o *ApplyOptions) error {


Same question as below about having a Prune and a pruneAll on the same object (though this one does more than just wrap). Also, now that applySetPruner isn't a separate object, should that file's content move here? Or if we're collecting the pruning related functionality in that file, shouldn't this and the method below be there?

This one I think is more defensible... it does a lot more (particularly now we've merged in updateParent). Also we're mapping from ApplyOptions which is a CLI concern (and specific to kubectl apply, not just kubectl) to ApplySetDeleteOptions which is more of an applyset library concern.

The overall motivation for the PR here is that we need to create a reusable "applyset" library, if we are to support diff as well without too much code duplication.

I think in terms of code organization, we should try to create a new package (probably applyset) which just has the applyset logic in it, and does not have kubectl apply specific types (like ApplyOptions). I just didn't want to do that here because moving code around at the same time as doing some separable refactors would make it harder to review..

Separate package in a later refactor SGTM!

KnVerey · 2023-03-14T16:45:56Z

/test pull-kubernetes-unit
/kind cleanup
/approve
/triage accepted
/priority important-longterm

k8s-ci-robot · 2023-03-14T16:46:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, 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-14T16:47:58Z

/hold
in case you want to address any of the nits on this PR (but feel free to do it in a follow up)
/lgtm

k8s-ci-robot · 2023-03-14T16:48:06Z

LGTM label has been added.

Git tree hash: 7b6746856106a425b7e42d152dc567be0c11ff81

justinsb · 2023-03-14T17:01:29Z

Thanks for review - addressed the nits (which I agree with, though I think we should do the refactor in a separate PR once we have this code shared with diff)

KnVerey · 2023-03-14T17:07:17Z

/lgtm
/unhold

k8s-ci-robot · 2023-03-14T17:07:24Z

LGTM label has been added.

Git tree hash: dc30d37deb76dfd1d59c50cfd1a60f72cbe30b31

KnVerey · 2023-03-14T17:21:44Z

/lgtm

k8s-ci-robot · 2023-03-14T17:21:51Z

LGTM label has been added.

Git tree hash: e18fd4e02ca5a55ddedf037f2707afca7fb1ed5f

k8s-ci-robot · 2023-03-14T17:48:18Z

PR needs rebase.

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.

k8s-ci-robot · 2023-03-14T17:48:47Z

PR needs rebase.

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.

k8s-ci-robot · 2023-03-14T17:49:17Z

PR needs rebase.

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.

This enables sharing with diff.

KnVerey · 2023-03-14T18:07:16Z

/lgtm

k8s-ci-robot · 2023-03-14T18:07:23Z

LGTM label has been added.

Git tree hash: f03f89c5f7659f8147fb432624706a67eed34b49

k8s-ci-robot requested review from KnVerey and seans3 March 14, 2023 12:13

justinsb force-pushed the applyset_refactor branch 2 times, most recently from e381732 to 70a02c0 Compare March 14, 2023 13:02

KnVerey reviewed Mar 14, 2023

View reviewed changes

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

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 14, 2023

k8s-ci-robot assigned KnVerey 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

justinsb force-pushed the applyset_refactor branch from 70a02c0 to 6620e5c Compare March 14, 2023 17:00

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

k8s-ci-robot requested a review from KnVerey March 14, 2023 17:00

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 14, 2023

justinsb force-pushed the applyset_refactor branch from 6620e5c to f68a546 Compare March 14, 2023 17:15

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label 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 added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2023

kubectl prunev2: Refactor the applyset to be more reusable

d016fdc

This enables sharing with diff.

justinsb force-pushed the applyset_refactor branch from f68a546 to d016fdc Compare March 14, 2023 17:57

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels 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 3c6ad6d into kubernetes:master Mar 14, 2023

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

KnVerey mentioned this pull request Mar 15, 2023

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

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubectl prunev2: Refactor the applyset to be more reusable #116580

kubectl prunev2: Refactor the applyset to be more reusable #116580

justinsb commented Mar 14, 2023 •

edited

KnVerey Mar 14, 2023

justinsb Mar 14, 2023

KnVerey Mar 14, 2023

KnVerey Mar 14, 2023

justinsb Mar 14, 2023

KnVerey Mar 14, 2023

justinsb Mar 14, 2023

KnVerey Mar 14, 2023

justinsb Mar 14, 2023

KnVerey Mar 14, 2023

KnVerey commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

KnVerey commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

justinsb commented Mar 14, 2023

KnVerey commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

KnVerey commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

KnVerey commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

kubectl prunev2: Refactor the applyset to be more reusable #116580

kubectl prunev2: Refactor the applyset to be more reusable #116580

Conversation

justinsb commented Mar 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnVerey commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

KnVerey commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

justinsb commented Mar 14, 2023

KnVerey commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

KnVerey commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

KnVerey commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

justinsb commented Mar 14, 2023 •

edited