-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
⚠️ KCP: Stop updating and using Kubeadm's ClusterStatus with Kubernetes v1.22 #4643
Conversation
/hold |
@fabriziopandini I'm not sure if you're aware so I thought I just mention it. Locally you're testing exactly this branch. Prow tests the commit which is created after this branch is merged into the base branch. Maybe that explains the error. It only really leads to differing results in rare cases. (but we had a few of them on-prem) |
Agree, rebase is usually the first things to do.... |
&actualConfig, | ||
)).To(Succeed()) | ||
|
||
g.Expect(actualConfig.Data[clusterStatusKey]).To(Equal("apiEndpoints:\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be worth to switch to multiline strings here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO multiline strings makes the code less readable when using yaml, because the indentation gets abruptly interrupted (and this is even worst if we are in a middle in of table tests / a series of test cases).
Also, we already did this for recent changes in KCP (e.g. all the refactor on the kubeadm config update)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you provide an example? I'm not sure how this is better than a straight multiline string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to change if you feel this is better, but here it is an example of indentation hard to read due to multiline strings
cluster-api/controlplane/kubeadm/internal/workload_cluster_test.go
Lines 809 to 821 in 7478817
{ | |
name: "updates the config map", | |
scheduler: kubeadmv1beta1.ControlPlaneComponent{ExtraArgs: map[string]string{"foo": "bar"}}, | |
objs: []client.Object{kubeadmConfig}, | |
expectErr: false, | |
expectedChanged: true, | |
expectedScheduler: `apiVersion: kubeadm.k8s.io/v1beta2 | |
kind: ClusterConfiguration | |
scheduler: | |
extraArgs: | |
foo: bar | |
`, | |
}, |
This is ( much more complex) an example without multiline strings; indentation is consisted and easier to read
cluster-api/controlplane/kubeadm/internal/workload_cluster_test.go
Lines 986 to 1018 in 5c19ea1
{ | |
name: "it should set the scheduler config", | |
clusterConfigurationData: "apiVersion: kubeadm.k8s.io/v1beta2\n" + | |
"kind: ClusterConfiguration\n", | |
newScheduler: bootstrapv1.ControlPlaneComponent{ | |
ExtraArgs: map[string]string{ | |
"bar": "baz", | |
"someKey": "someVal", | |
}, | |
ExtraVolumes: []bootstrapv1.HostPathMount{ | |
{ | |
Name: "mount2", | |
HostPath: "/bar/baz", | |
MountPath: "/foo/bar", | |
}, | |
}, | |
}, | |
wantClusterConfiguration: "apiServer: {}\n" + | |
"apiVersion: kubeadm.k8s.io/v1beta2\n" + | |
"controllerManager: {}\n" + | |
"dns: {}\n" + | |
"etcd: {}\n" + | |
"kind: ClusterConfiguration\n" + | |
"networking: {}\n" + | |
"scheduler:\n" + | |
" extraArgs:\n" + | |
" bar: baz\n" + | |
" someKey: someVal\n" + | |
" extraVolumes:\n" + | |
" - hostPath: /bar/baz\n" + | |
" mountPath: /foo/bar\n" + | |
" name: mount2\n", | |
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could start the multi-line string with a new line and use TrimSpace https://play.golang.org/p/YShA372kY82 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's tackle this in a new PR?
bindPort: 6443 | ||
apiVersion: kubeadm.k8s.io/v1beta2 | ||
kind: ClusterStatus`, | ||
clusterStatusKey: "apiEndpoints:\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, re:multiline strings
machine: machine, | ||
objs: []client.Object{kubeadmConfig}, | ||
expectErr: false, | ||
expectedEndpoints: "apiEndpoints:\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, re:multiline strings
Why is this PR marked as a breaking change? |
6d60650
to
76e4cf8
Compare
I made this breaking change to raise the attention on the fact the there ClusterStatus is going away, in case some tools on top of CAPI is not paying attention to kubeadm changes. |
/retitle |
/milestone v0.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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:
Approvers can indicate their approval by writing |
@CecileRobertMichon over to you |
is the hold still in place intentionally @fabriziopandini ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold cancel |
/retest |
76e4cf8
to
40e737b
Compare
rebased |
/retest |
/retitle |
/lgtm |
What this PR does / why we need it:
This PR makes it possible for CAPI 0.4.x to work with Kubernetes/kubeadm v1.22.0, where the ClusterStatus field inside the Kubeadm-config has been dropped.
Which issue(s) this PR fixes:
Fixes #4423