No pruning with NewDirectApplier

[wscheep]

What happened:
The kubectl --prune flag is set as extraOpts in reconciler.go.

However, since the NewDirectApplier is used (#101), the prune option is not propagated to the applyOpts.Prune boolean of direct.go.

What you expected to happen:
The prune option to be propagated so that pruning happens as expected.

How to reproduce it (as minimally and precisely as possible):
Putting a breakpoint on direct.go#L80 clearly shows that the Prune flag is not set on the applyOpts object.

Anything else we need to know?:
There currently is also no support for the --prune-whitelist flag, which we need for our use case.
Happy to create a PR for this, but I think it's better to solve this issue first.

Is there a specific reason for keeping the old ExecApplier around?
If not, I guess the ApplierOptions from the library could be directly replaced by ApplyOptions from the k8s.io./kubectl module.

Environment:

• Kubernetes version (use kubectl version):

Client Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.5", GitCommit:"aea7bbadd2fc0cd689de94a54e5b7b758869d691", GitTreeState:"clean", BuildDate:"2021-09-15T21:10:45Z", GoVersion:"go1.16.8", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.1", GitCommit:"5e58841cce77d4bc13713ad2b91fa0d961e69192", GitTreeState:"clean", BuildDate:"2021-05-21T23:01:33Z", GoVersion:"go1.16.4", Compiler:"gc", Platform:"linux/amd64"}

• OS (e.g: cat /etc/os-release): macOS Big Sure 11.6.2
• Kernel (e.g. uname -a):

Darwin C02YK2Z2JHD2 20.6.0 Darwin Kernel Version 20.6.0: Wed Nov 10 22:23:07 PST 2021; root:xnu-7195.141.14~1/RELEASE_X86_64 x86_64 i386 MacBookPro15,2 Darwin

• Go version (e.g. go version): go version go1.17.6 darwin/amd64
• Others:

[justinsb]

Thanks for figuring this out and for sending the PR - I'm going to look at why it isn't passing...

Is there a specific reason for keeping the old ExecApplier around?
There's not a strong reason, other than avoiding breaking people. However, extraArgs is pretty specific to the kubectl, and maybe we should replace that with more specific options instead. (i.e. drop extraArgs, but expose Prune, ForceConflicts, Selector etc).

This feels like a great time to introduce stronger versioning into this library and maybe start cleaning up some of the older code. Once we introduce versioning, we can then drop support for the exec applier at a version boundary (clearly announcing it). (Versioning and clearer announcements there will also probably better let us find out if anyone wants to keep the execApplier!)

[wscheep]

Sounds good, let me know if there's anything I can do to help with introducing stronger versioning.

In the mean time, we've used a nicer workaround where we convert ApplierOptions into the latest ApplyOptions from the k8s.io./kubectl module.
I'll create a PR once the versioning is in place.

[xuzhenglun]

pruning seems still broken, because some attributes of applyOpts is nil.

To make pruning work normally, below attributes must be set before calling applyOpts.Run().

	applyOpts.Mapper = opt.RESTMapper
	applyOpts.DynamicClient, _ = dynamic.NewForConfig(opt.RESTConfig)
	applyOpts.DeleteOptions.CascadingStrategy = "Background"
	applyOpts.DeleteOptions.GracePeriod = -1
	applyOpts.PostProcessorFn = applyOpts.PrintAndPrunePostProcessor()

[atoato88]

Recently, some PRs related with prune functionality are created.
I think we should reopen this issue.

[xuzhenglun]

DirectApplier is implemented very tricky in the current code. IMHO, some reason can blame that k/kubectl is lack of a good interface for other people to reuse it easily.

To solve this issue, I have 3 ideas:

1. After 1.16, ServerSide apply is available, It allows client just send resources to kube-apiserver, and kube-apiserver will figure out how to deal with apply progress. The good side is this way can get rid of k/kubectl problems. But something bad is some function like prune is missing and we should implement it by self.
2. Reuse k/kubectl at a higher level, just like running kubectl apply in higher level #211 did. But there are some codes which still have to be copy & paste. I know this is not a good way in practice, but I really did not find a way to reuse that code completely. Something detail can see in the discussion in that PR.
3. Or we can reuse k/kubectl in the highest level: binary level. It is exactly what ExecApplier did. So is there any way to enable it? I know that will introduce a binary dependency but also got a good side effect: users can use any version of kubectl if they want to. It can be a good thing because something in k/kubectl is version-related. such as the default value of --prune-whitelist.

[atoato88]

/reopen

@justinsb
WDYT about comment above?

[k8s-ci-robot]

@atoato88: Reopened this issue.

In response to this:

/reopen

@justinsb
WDYT about comment above?

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.

[xuzhenglun]

@justinsb

Any comment on this issue?

[xuzhenglun]

I believe that #217 fixes #201 too, and we can close this issue now.

WDYT @atoato88

[atoato88]

/close
Yes, as of now this issue is fixed, I think.
I close this.

[k8s-ci-robot]

@atoato88: Closing this issue.

In response to this:

/close
Yes, as of now this issue is fixed, I think.
I close this.

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.

[JeffLuoo]

Do we have tests that will cover the prune?

[atoato88]

We don't have cover tests to prune in DirectApplier.
We need more coverage on unit test for preventing some degrades. 😅