Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Support adding node labels for control plane nodes when cluster creation for vsphere/aws/azure #3818

Merged
merged 5 commits into from
Nov 23, 2022
Merged

Support adding node labels for control plane nodes when cluster creation for vsphere/aws/azure #3818

merged 5 commits into from
Nov 23, 2022

Conversation

ydp
Copy link

@ydp ydp commented Nov 2, 2022 •
edited
Loading

What this PR does / why we need it

Add CONTROL_PLANE_NODE_LABELS param to support adding labels for control plane nodes upon cluster creation.

Which issue(s) this PR fixes

Fixes #3616

Describe testing done for PR

Manually created cluster with below variables set:

CONTROL_PLANE_NODE_LABELS: 'key1=value1,key2=value2'

The result on vSphere:

root@jammy:~# kubectl get no
NAME                               STATUS   ROLES                  AGE   VERSION
mgmt-97zkp-ns97s                   Ready    control-plane,master   47m   v1.23.8+vmware.2
mgmt-md-0-bvwq2-6b8fcb8848-sgjwx   Ready    <none>                 45m   v1.23.8+vmware.2
root@jammy:~# kubectl get no mgmt-97zkp-ns97s -o jsonpath='{.metadata.labels}'
{"beta.kubernetes.io/arch":"amd64","beta.kubernetes.io/instance-type":"vsphere-vm.cpu-2.mem-8gb.os-photon","beta.kubernetes.io/os":"linux","image-type":"ova","key1":"value1","key2":"value2","kubernetes.io/arch":"amd64","kubernetes.io/hostname":"mgmt-97zkp-ns97s","kubernetes.io/os":"linux","node-role.kubernetes.io/control-plane":"","node-role.kubernetes.io/master":"","node.cluster.x-k8s.io/esxi-host":"10.206.189.22","node.kubernetes.io/exclude-from-external-load-balancers":"","node.kubernetes.io/instance-type":"vsphere-vm.cpu-2.mem-8gb.os-photon","os-name":"photon","os-type":"linux","run.tanzu.vmware.com/tkr":"v1.23.8---vmware.2-tkg.2-zshippable"}

The result on AWS:

~/ ❯ kubectl get no
NAME                                       STATUS   ROLES                  AGE   VERSION
ip-10-0-5-127.us-east-2.compute.internal   Ready    <none>                 11m   v1.23.8+vmware.2
ip-10-0-5-143.us-east-2.compute.internal   Ready    control-plane,master   12m   v1.23.8+vmware.2
~/ ❯ kubectl get no ip-10-0-5-143.us-east-2.compute.internal -o jsonpath='{.metadata.labels}'
{"ami-id":"ami-010edf5fc7bb6f7a3","ami-region":"us-east-2","beta.kubernetes.io/arch":"amd64","beta.kubernetes.io/instance-type":"c5.xlarge","beta.kubernetes.io/os":"linux","failure-domain.beta.kubernetes.io/region":"us-east-2","failure-domain.beta.kubernetes.io/zone":"us-east-2b","image-type":"ami","key1":"value1","key2":"value2","kubernetes.io/arch":"amd64","kubernetes.io/hostname":"ip-10-0-5-143.us-east-2.compute.internal","kubernetes.io/os":"linux","node-role.kubernetes.io/control-plane":"","node-role.kubernetes.io/master":"","node.kubernetes.io/exclude-from-external-load-balancers":"","node.kubernetes.io/instance-type":"c5.xlarge","os-arch":"amd64","os-name":"ubuntu","os-type":"linux","os-version":"2004","run.tanzu.vmware.com/os-image":"v1.23.8---vmware.2-ami-010edf5fc7bb6f7a3","run.tanzu.vmware.com/tkr":"v1.23.8---vmware.2-tkg.2-zshippable","topology.kubernetes.io/region":"us-east-2","topology.kubernetes.io/zone":"us-east-2b"}%

The result on Azure:

~/ ❯ kubectl get no
NAME                            STATUS   ROLES                  AGE   VERSION
dp2-control-plane-dqttd-qfcgd   Ready    control-plane,master   20m   v1.23.8+vmware.2
dp2-md-0-infra-bkb7d-lncf7      Ready    <none>                 18m   v1.23.8+vmware.2
~/ ❯ kubectl get no dp2-control-plane-dqttd-qfcgd -o jsonpath='{.metadata.labels}'
{"azure-offer":"tkg-capi-2022-06-24","azure-publisher":"vmware-inc","azure-sku":"k8s-1dot23dot8-ubuntu-2004","azure-thirdPartyImage":"true","azure-version":"2022.07.20","beta.kubernetes.io/arch":"amd64","beta.kubernetes.io/instance-type":"Standard_D2s_v3","beta.kubernetes.io/os":"linux","failure-domain.beta.kubernetes.io/region":"westus2","failure-domain.beta.kubernetes.io/zone":"westus2-1","image-type":"azure","key1":"value1","key2":"value2","kubernetes.io/arch":"amd64","kubernetes.io/hostname":"dp2-control-plane-dqttd-qfcgd","kubernetes.io/os":"linux","node-role.kubernetes.io/control-plane":"","node-role.kubernetes.io/master":"","node.kubernetes.io/exclude-from-external-load-balancers":"","node.kubernetes.io/instance-type":"Standard_D2s_v3","os-arch":"amd64","os-name":"ubuntu","os-type":"linux","os-version":"2004","run.tanzu.vmware.com/os-image":"v1.23.8---vmware.2-tkg.1-4fe60a764820f694f40a06fb273edc96","run.tanzu.vmware.com/tkr":"v1.23.8---vmware.2-tkg.2-zshippable","topology.kubernetes.io/region":"westus2","topology.kubernetes.io/zone":"westus2-1"}%

Release note

Add `CONTROL_PLANE_NODE_LABELS` param to support adding labels for control plane nodes upon cluster creation.

Additional information

Special notes for your reviewer

@ydp ydp requested review from a team as code owners November 2, 2022 07:07
@github-actions
Copy link

github-actions bot commented Nov 2, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3818/20221102071743/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@codecov
Copy link

codecov bot commented Nov 2, 2022 •
edited
Loading

Codecov Report

Merging #3818 (8deb9f2) into main (f06167f) will decrease coverage by 0.84%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3818      +/-   ##
==========================================
- Coverage   47.64%   46.79%   -0.85%     
==========================================
  Files         420      445      +25     
  Lines       42080    43669    +1589     
==========================================
+ Hits        20051    20437     +386     
- Misses      20112    21306    +1194     
- Partials     1917     1926       +9
Impacted Files Coverage Δ
cmd/cli/plugin/cluster/get.go 6.27% <0.00%> (ø)
...md/cli/plugin/cluster/delete_machinehealthcheck.go 19.23% <0.00%> (ø)
cmd/cli/plugin/cluster/osimage.go 100.00% <0.00%> (ø)
cmd/cli/plugin/cluster/get_node_pools.go 10.52% <0.00%> (ø)
cmd/cli/plugin/cluster/main.go 0.00% <0.00%> (ø)
...in/cluster/get_machinehealthcheck_control_plane.go 11.11% <0.00%> (ø)
cmd/cli/plugin/cluster/osimage_oracle.go 3.20% <0.00%> (ø)
...i/plugin/cluster/delete_machinehealthcheck_node.go 16.66% <0.00%> (ø)
...cluster/delete_machinehealthcheck_control_plane.go 16.66% <0.00%> (ø)
cmd/cli/plugin/cluster/machinehealthcheck.go 100.00% <0.00%> (ø)
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ydp ydp force-pushed the topic/dingpingy/nodelabels branch from 8e7853d to c720ce6 Compare November 4, 2022 09:35
@github-actions
Copy link

github-actions bot commented Nov 4, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3818/20221104094500/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

tenczar
tenczar approved these changes Nov 8, 2022
View reviewed changes
Copy link
Contributor

@tenczar tenczar left a comment

Choose a reason for hiding this comment

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

This will still break the node pools cli implementation. This fix will either be to get tkgs on board with this style of adding nodeLabels or have the node pool cil implementation handle two sets of nodeLabels/nodePoolLabels. Either way, I won't have time to implement this before Friday. I'm hoping to have some help from you in updating the implementation. Please file a P0 bug to track this for FC

@ydp ydp force-pushed the topic/dingpingy/nodelabels branch from c720ce6 to a2e8819 Compare November 8, 2022 12:39
@ydp
Copy link
Author

ydp commented Nov 8, 2022 •
edited
Loading

Latest test result after commit Ignore worker.nodeLabels if nodePoolLabels is set

root@jammy:~# kubectl --kubeconfig wl.kubeconfig get no
NAME                             STATUS   ROLES                  AGE     VERSION
wl-md-0-h8fbb-57ffff5b47-4jsq2   Ready    <none>                 3m31s   v1.23.8+vmware.2
wl-wt97n-94jpn                   Ready    control-plane,master   5m15s   v1.23.8+vmware.2
root@jammy:~# kubectl --kubeconfig wl.kubeconfig get no wl-md-0-h8fbb-57ffff5b47-4jsq2 -o jsonpath='{.metadata.labels}'
{"beta.kubernetes.io/arch":"amd64","beta.kubernetes.io/instance-type":"vsphere-vm.cpu-2.mem-8gb.os-photon","beta.kubernetes.io/os":"linux","image-type":"ova","key3":"value3","key4":"value4","kubernetes.io/arch":"amd64","kubernetes.io/hostname":"wl-md-0-h8fbb-57ffff5b47-4jsq2","kubernetes.io/os":"linux","node.cluster.x-k8s.io/esxi-host":"10.206.187.19","node.kubernetes.io/instance-type":"vsphere-vm.cpu-2.mem-8gb.os-photon","os-name":"photon","os-type":"linux","run.tanzu.vmware.com/tkr":"v1.23.8---vmware.2-tkg.2-zshippable"}root@jroot@jammy:~# tanzu cl node-pool set wl -f ./node-pool.yaml
Cluster update for node pool 'md-0' completed successfully
root@jammy:~# tanzu cl node-pool list wl
  NAME  NAMESPACE  PHASE    REPLICAS  READY  UPDATED  UNAVAILABLE
  md-0  default    Running  2         1      1        1
root@jammy:~# tanzu cl node-pool list wl
  NAME  NAMESPACE  PHASE    REPLICAS  READY  UPDATED  UNAVAILABLE
  md-0  default    Running  1         1      1        0
root@jammy:~# kubectl --kubeconfig wl.kubeconfig get no
NAME                             STATUS   ROLES                  AGE     VERSION
wl-md-0-h8fbb-6dbfd99466-vnhwn   Ready    <none>                 3m19s   v1.23.8+vmware.2
wl-wt97n-94jpn                   Ready    control-plane,master   10m     v1.23.8+vmware.2
root@jammy:~# kubectl --kubeconfig wl.kubeconfig get no wl-md-0-h8fbb-6dbfd99466-vnhwn -o jsonpath='{.metadata.labels}'
{"beta.kubernetes.io/arch":"amd64","beta.kubernetes.io/instance-type":"vsphere-vm.cpu-2.mem-8gb.os-photon","beta.kubernetes.io/os":"linux","image-type":"ova","key5":"value5","key6":"value6","kubernetes.io/arch":"amd64","kubernetes.io/hostname":"wl-md-0-h8fbb-6dbfd99466-vnhwn","kubernetes.io/os":"linux","node.cluster.x-k8s.io/esxi-host":"10.206.189.41","node.kubernetes.io/instance-type":"vsphere-vm.cpu-2.mem-8gb.os-photon","os-name":"photon","os-type":"linux","run.tanzu.vmware.com/tkr":"v1.23.8---vmware.2-tkg.2-zshippable"}root@jammy:~#

The worker.nodeLabels "key3":"value3","key4":"value4" is replaced by nodePoolLabels "key5":"value5","key6":"value6" after node-pool command is executed.

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3818/20221108125100/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@ydp
Copy link
Author

ydp commented Nov 8, 2022

Summarize a discussion with @tenczar offline, Nick mentioned that in legacy cluster mode nodePoolLabels will replace worker labels set by ytt overlay, but what I thought those 2 types of labels are to be merged, which is wrong, to maintain backward compatibility, I add commit Ignore worker.nodeLabels if nodePoolLabels is set.
This also bring another benefit that Nick mentioned, customer will not confuse with those labels. In my previous implementation, since labels can be added both ways, customer will be easily confused with those features, and ended with wrong labels in their cluster. The commit addresses this concern.

tenczar
tenczar reviewed Nov 9, 2022
View reviewed changes
Copy link
Contributor

@tenczar tenczar left a comment

Choose a reason for hiding this comment

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

I would love to set up a chat with a PM about this, who is it and when can we have this meeting?

providers/infrastructure-vsphere/v1.4.1/cconly/base.yaml Outdated Show resolved Hide resolved
@ydp ydp added the do-not-merge/hold Some fixes necessary, hold for merging label Nov 9, 2022
@ydp
Copy link
Author

ydp commented Nov 9, 2022

Hi @tenczar , just let you know, I raised the issue in our scrum, and we prefer to hold the PR since we don't want to touch TKGs.

@ydp ydp force-pushed the topic/dingpingy/nodelabels branch from a2e8819 to f43a7b4 Compare November 10, 2022 00:53
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3818/20221110010353/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@ydp
Copy link
Author

ydp commented Nov 10, 2022

After discuss with @DanielXiao , we think it's better to drop this, since control plane node label seems non-trivial, and worker node label is equivalent to nodePoolLabels, as PM also want to keep consistent between TKGm and TKGs. No need to add this PR to increase complexity. Hence close it now.

@ydp ydp closed this Nov 10, 2022
@ydp ydp reopened this Nov 18, 2022
@ydp ydp force-pushed the topic/dingpingy/nodelabels branch from f43a7b4 to f236302 Compare November 18, 2022 03:02
@ydp
Copy link
Author

ydp commented Nov 18, 2022

Reopen it since Timmy explicitly require to add the control plane node labels back, the worker.Labels will be just use nodePoolLabels as alternative.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3818/20221118031024/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3818/20221118031119/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@ydp ydp removed the do-not-merge/hold Some fixes necessary, hold for merging label Nov 18, 2022
@ydp ydp force-pushed the topic/dingpingy/nodelabels branch from f236302 to 2e75bea Compare November 18, 2022 09:28
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3818/20221118093408/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@ydp ydp changed the title Support adding node labels when cluster creation for vsphere/aws/azure Support adding node labels for control plane nodes when cluster creation for vsphere/aws/azure Nov 18, 2022
@ydp ydp force-pushed the topic/dingpingy/nodelabels branch from 2e75bea to 566b666 Compare November 21, 2022 00:38
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3818/20221121004624/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@ydp ydp force-pushed the topic/dingpingy/nodelabels branch from 566b666 to 8deb9f2 Compare November 23, 2022 02:16
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3818/20221123022543/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@DanielXiao DanielXiao added the ok-to-merge PRs should be labelled with this before merging label Nov 23, 2022
@ydp
Copy link
Author

ydp commented Nov 23, 2022 •
edited
Loading

Just finished manually testing for AWS cluster creation with latest code, though the cluster cannot be created due to feature-gate issue, I could see the labels are correct on KCP:

~/work/github/tanzu-framework  ❯ kubectl get kcp peter-zm98w -n tkg-system -o jsonpath='{.spec.kubeadmConfigSpec.initConfiguration.nodeRegistration.kubeletExtraArgs.node-labels}'
ami-id=ami-010edf5fc7bb6f7a3,ami-region=us-east-2,image-type=ami,os-arch=amd64,os-name=ubuntu,os-type=linux,os-version=2004,run.tanzu.vmware.com/os-image=v1.23.8---vmware.2-ami-010edf5fc7bb6f7a3,run.tanzu.vmware.com/tkr=v1.23.8---vmware.2-tkg.2-zshippable,key1=value1,key2=value2
~/work/github/tanzu-framework  ❯
~/work/github/tanzu-framework  ❯ kubectl get kcp peter-zm98w -n tkg-system -o jsonpath='{.spec.kubeadmConfigSpec.joinConfiguration.nodeRegistration.kubeletExtraArgs.node-labels}'
ami-id=ami-010edf5fc7bb6f7a3,ami-region=us-east-2,image-type=ami,os-arch=amd64,os-name=ubuntu,os-type=linux,os-version=2004,run.tanzu.vmware.com/os-image=v1.23.8---vmware.2-ami-010edf5fc7bb6f7a3,run.tanzu.vmware.com/tkr=v1.23.8---vmware.2-tkg.2-zshippable,key1=value1,key2=value2

@ydp ydp merged commit ed4efe6 into main Nov 23, 2022
@ydp ydp deleted the topic/dingpingy/nodelabels branch November 23, 2022 06:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tenczar tenczar tenczar approved these changes

@DanielXiao DanielXiao DanielXiao approved these changes

Assignees
No one assigned
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support officially documented ytt overlay customizations in the default clusterclass
4 participants
@ydp @DanielXiao @tenczar @vmwclabot