Cluster-api: Realign forked third party changes

[detiber]

Previously we copied kubectl drain code into the third_party directory with the intention of eventually removing the copy and importing upstream directly (once the kubectl move to staging was complete). The kubectl code is now published at k8s.io/kubectl, however it appears that there have been changes made locally to the drain code we have copied.

These changes need to be re-aligned with upstream and we should be relying on an upstream import.

[vincepri]

/help
/milestone v0.4.0
/priority important-soon

[k8s-ci-robot]

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help
/milestone v0.4.0
/priority important-soon

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.

[neolit123]

Previously we copied kubectl drain code into the third_party directory with the intention of eventually removing the copy and importing upstream directly (once the kubeadm move to staging was complete). The kubeadm code is now published at k8s.io/kubeadm, however it appears that there have been changes made locally to the drain code we have copied.

s/kubeadm/kubectl/ :)

[detiber]

s/kubeadm/kubectl/ :)

:grimacing: fixed, thanks for catching that

[srm09]

From the code reading I did, one change that I can see is:

This particular method uses the https://github.com/kubernetes-sigs/cluster-api/blob/master/third_party/kubernetes-drain/cordon.go#L89-L94 supplied context, but the corresponding code in the upstream kubectl codebase does not use the supplied context. I am unsure how big of a deal that is, but it is different from our current functionality.

[detiber]

@srm09 indeed, it definitely complicates the re-alignment. I'm wondering if we also need an automated PR check that verifies that anything updated in third_party is done so as part of a sync from upstream vs local changes?

[ncdc]

As part of the upgrade to controller-runtime 0.7 (#3735), we modified the third party drain code to take in contexts. If we want to keep that modification, then I think we'll need to manually reconcile when updating our copy of the drain code.

Should we also submit PRs to k/k to update the drain code to take in contexts, assuming that's feasible? If we can do that, then we should be able to vendor the kubectl drain code properly, instead of having a copy of it (once we can upgrade to the newer kubectl tag).

[detiber]

Should we also submit PRs to k/k to update the drain code to take in contexts, assuming that's feasible? If we can do that, then we should be able to vendor the kubectl drain code properly, instead of having a copy of it (once we can upgrade to the newer kubectl tag).

I suspect we might need to do some refactoring and introduce new methods like blahWithContext to avoid breaking the upstream API, but seems doable.

[srm09]

I will open an issue in the k/k repo around this, and start a Slack thread about the same and see what folks have to say about that.

[srm09]

Opened a draft PR against kubectl https://github.com/kubernetes/kubernetes/pull/97078

[srm09]

kubernetes/kubernetes#97078 got merged, but these changes will be landing in 1.21
Created a backport PR kubernetes/kubernetes#98375 for the 1.20 release stream.