kubeadm: annotate CRI socket on kubelet-finalize phase

[ereslibre]

What type of PR is this?
/kind bug

What this PR does / why we need it:
When running kubeadm init phase upload-config kubelet the current
logic will try to read the config file and patch the node annotation
for the CRI socket.

Ideally, this logic should not be triggered under this phase, because
uploading the kubelet configuration to the cluster and patching the
current node CRI socket annotation are two different things.

Now that we introduced the kubelet-finalize phase, make this logic
happen under this phase on init. The rest (join, upgrade) are left
untouched.

Which issue(s) this PR fixes:
Fixes kubernetes/kubeadm#1924

Does this PR introduce a user-facing change?:

NONE

/priority backlog
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ereslibre

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:

• cmd/kubeadm/OWNERS [ereslibre]

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

[ereslibre]

After some discussion with @neolit123 we might want to take different approaches here:

• Keep as it is, annotate-cri-socket, however this creates some future expectations on keeping this subphase (keep reading).

• If we want to keep the CRI annotation, maybe make this phase experimental-annotate-cri-socket so we can remove it altogether in the future, since CRI annotation has been mentioned several times in the past to not be really required.

• If we decide to remove CRI annotations altogether, then this PR needs to be adapted to remove the annotation logic. This would imply that kubeadm would need to autodetect the CRI socket on every execution.

I think it's reasonable to delegate this discussion post KubeCon, so folks can give their opinion.

[k8s-ci-robot]

@ereslibre: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-integration	8f40148	link	/test pull-kubernetes-integration

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[rosti]

Thanks @ereslibre !
Code-wise it looks good. However, hiding the CRI annotation in another phase was deliberate when designing all the phases. This is a technical detail that can change.
Even now the CRI can be obtained from the kubelet command line parameters. In the future it is best to obtain it from the local kubeadm component config file (provided that a new field for the CRI socket is introduced).
Is re-annotation such a big problem for you? Let's discuss this on the next office hours.
/hold

[ereslibre]

Is re-annotation such a big problem for you? Let's discuss this on the next office hours.

Let's discuss during next office hours. It's not a big deal, but it feels a bit arbitrary that we are patching the CRI socket during the upload config phase. In my opinion it would be better to extract it (even if we prefix the new phase with experimental-, so we are free to remove it in the future when we plan to get rid of this annotation completely).

[ereslibre]

Also, using experimental- for something that is not experimental but knowlingly will be removed might seem a bit "rule bending"... let's discuss it.

[rosti]

The idea is that at some point the kubelet component config will have a CRI socket field in it. Thus the action of "storing kubelet's CRI socket" will take place during the upload config phase.

[neolit123]

@rosti @ereslibre

However, hiding the CRI annotation in another phase was deliberate when designing all the phases

we can still add it as a separate phase, but make the phase hidden. the phase runner supports this.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.