Remove experimentalRetryJoin

[killianmuldoon]

experimentalRetryJoin is deprecated as of CAPI release 1.2. Removal is expected to happen in release 1.3 - but before proceeding we should have high confidence that CAPI + Kubeadm are capable today of solving the problem that experimentalRetryJoin was designed to fix.

In order to increase confidence in the removal we should:

1. Ask providers which, if any, are still using experimentalRetryJoin, to what extent, and to solve what problem.
2. Figure out a testing strategy for providers that would trigger the problem experimentalRetryJoin was designed to fix.
3. Ensure these tests are passing and we have high confidence in the current retryJoin behaviour of Kubeadm.

Once we're confident that we can proceed with removal we need to:

1. Remove the field from the bootstrap KubeadmConfig types + regenerate CRDs
2. Stop embedding the kubeadm-bootsrap-script in bootstrap/kubeadm/internal/cloudinit/cloudinit.go
3. Update cloudinit.go, removing functions and variables related to the embedded script e.g. generateBootstrapScript, kubeadmBootstrapScript
4. Remove the KubeadmBootstrap script
5. Add a note to the migration guide that the

/area bootstrap

[sbueringer]

In my opinion we should never remove a field from a v1beta1 apiVersion without an apiVersion bump as it's a breaking change to v1beta1 otherwise.

[killianmuldoon]

In my opinion we should never remove a field from a v1beta1 apiVersion without an apiVersion bump as it's a breaking change to v1beta1 otherwise.

Good point, but I suppose that doesn't necessarily preclude us from changing the behaviour. It's not exactly a feature gate, but the field is prefixed with experimental

[sbueringer]

So we would keep the field but disable its behavior? I think that's a very dangerous precedent and ~ the same as dropping the field.

[killianmuldoon]

So we would keep the field but disable its behavior? I think that's a very dangerous precedent and ~ the same as dropping the field.

This is done regularly for experimental features as I understand it. The key question is whether we consider this experimental or not given the prefix. I think the most important thing here is whether the actual behaviour is fixed rather than the API fields and behaviour though.

[killianmuldoon]

Maybe an alternative path to deprecating this is to add a feature flag and a binary flag to enable this behaviour while deprecating and eventually removing the experimental API field. This would bring the way we enable the feature in line with our other experimental features like ClusterClass and MachinePools.

[vincepri]

/kind api-change

As mentioned above, we need to deprecate now (just a comment on the field would suffice) + remove in the next API version

[vincepri]

/triage accepted

[fabriziopandini]

/milestone v1.3

we should get this done before the release

[fabriziopandini]

/good-first-issue

[sbueringer]

If I'm not mistaken deprecation was done in these PRs:

• 🌱 Deprecate experimentalRetryJoin #6938
• [release-1.2] 🌱 Deprecate experimentalRetryJoin #6941

It was already part of v1.2.0 (https://github.com/kubernetes-sigs/cluster-api/releases/tag/v1.2.0).

I wonder a bit about removal though.

I don't have background information for what exactly this feature was needed and why we can drop it now (either because it's not needed anymore or we have a replacement I guess (?)).

Additional indication for that:

@yastij: why did we drop UseExperimentalRetryJoin it's still needed in slow/flaky envs
kubernetes-sigs/cluster-api-provider-vsphere#1602 (comment)

[killianmuldoon]

I think we should remove the good first issue and help wanted labels, as this removal isn't something easily implemented. @fabriziopandini WDYT?

[fabriziopandini]

If I got this right what we are doing now is just applying a deprecated label on the API field (I have updated the issue title)
actual deprecation will happen in the next cycle

[sbueringer]

If I'm not mistaken deprecation was done in these PRs:

• seedling Deprecate experimentalRetryJoin #6938
• [release-1.2] seedling Deprecate experimentalRetryJoin #6941

It was already part of v1.2.0 (https://github.com/kubernetes-sigs/cluster-api/releases/tag/v1.2.0).

I wonder a bit about removal though.

I don't have background information for what exactly this feature was needed and why we can drop it now (either because it's not needed anymore or we have a replacement I guess (?)).

Additional indication for that:

@yastij: why did we drop UseExperimentalRetryJoin it's still needed in slow/flaky envs
kubernetes-sigs/cluster-api-provider-vsphere#1602 (comment)

I'm slightly confused. As mentioned, I think we did this already?

[fabriziopandini]

Ok I have missed this. reset.

[fabriziopandini]

/remove-good-first-issue

[fabriziopandini]

Dropping to the milestone
@yastij @randomvariable what is your opinion about making this happen in 1.4 timeframe (target date ~march 2023)

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[sbueringer]

/remove-lifecycle stale

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[fabriziopandini]

/priority important-longterm

[sbueringer]

Just a note. Just checked linked issues and talked to some folks. We should be really able to remove the field as there were various improvements to kubeadm.

Also in the worst case were folks still need it they can remove the kubeadm binary through a script and implement their own retry handling there.

[neolit123]

Just a note. Just checked linked issues and talked to some folks. We should be really able to remove the field as there were various improvements to kubeadm.

Also in the worst case were folks still need it they can remove the kubeadm binary through a script and implement their own retry handling there.

+1, i though this was removed already. :\

[neolit123]

i see there are steps in the OP, i can give this a shot.
/assign

[sbueringer]

We have to wait for the next apiVersion (it's deprecated already)

I just additionally talked to some folks if they still depend on it

[fabriziopandini]

based on the comment above
/triage accepted

[chrischdi]

Note: #10983 gives another indicator that we should not re-add this in v1beta2.