Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Changes to controlplane spec should trigger a rollout #114

Merged
merged 5 commits into from
May 20, 2024

Conversation

wikoion
Copy link
Contributor

@wikoion wikoion commented May 14, 2024

This PR addresses #83, changes to the controlplane spec should trigger a rollout of controlplane machines.
I've tried to mirror the approach used in kubeadm as much as possible while adapting for k3s

@wikoion wikoion force-pushed the wikoion/rollout-after-spec-change branch 2 times, most recently from 44be031 to 4a4e8d2 Compare May 14, 2024 12:26
…d, mirroring the approach used by kubeadm

Signed-off-by: Richard Draycott <[email protected]>
@wikoion wikoion force-pushed the wikoion/rollout-after-spec-change branch from 4a4e8d2 to 9b1f81b Compare May 14, 2024 12:27
@wikoion wikoion force-pushed the wikoion/rollout-after-spec-change branch from bf39bea to 81d4839 Compare May 14, 2024 14:20
@wikoion wikoion marked this pull request as ready for review May 14, 2024 14:20
// If the annotation is not present (machine is either old or adopted), we won't roll out on any possible changes
// made in KCP's KThreesServerConfig given that we don't have enough information to make a decision.
// Users should use KCP.Spec.RolloutAfter field to force a rollout in this case.
func matchKThreesServerConfig(kcp *controlplanev1.KThreesControlPlane, machine *clusterv1.Machine) bool {
Copy link
Collaborator

@mogliang mogliang May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least for controlplane, init and join cp node has same bootstrap config, this is different from how kubeadm works

// InitialControlPlaneConfig returns a new KThreesConfigSpec that is to be used for an initializing control plane.
func (c *ControlPlane) InitialControlPlaneConfig() *bootstrapv1.KThreesConfigSpec {
bootstrapSpec := c.KCP.Spec.KThreesConfigSpec.DeepCopy()
return bootstrapSpec
}
// JoinControlPlaneConfig returns a new KThreesConfigSpec that is to be used for joining control planes.
func (c *ControlPlane) JoinControlPlaneConfig() *bootstrapv1.KThreesConfigSpec {
bootstrapSpec := c.KCP.Spec.KThreesConfigSpec.DeepCopy()
return bootstrapSpec
}

I'm not very sure about worker node, you may provision a k3s cluster and check the bootstrap config.

Let's try if we can simplify this code

Copy link
Contributor Author

@wikoion wikoion May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you mean. The init and join config is just the kcp KThreesConfigSpec, we do still compare this here, how do you suggest we should simplify the code?

Copy link
Collaborator

@mogliang mogliang May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can simply compare kcp config and machine bootstrap config directly, no need to compare serverConfig separately

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in the latest commit

@wikoion wikoion force-pushed the wikoion/rollout-after-spec-change branch from 998fb1d to 9caad96 Compare May 16, 2024 11:53
@mogliang mogliang merged commit 7b104e6 into k3s-io:main May 20, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants