CPIConfigController also Enqueue cluster event #3318

lubronzhan · 2022-09-12T23:32:42Z

What this PR does / why we need it

VSphereCPIConfigController also relies on underlying Cluster object. The controller should watch on Cluster's event as well
Otherwise, it will cause delay when creating the cluster.
Currently it will just silently exit and might cause 20 minutes delay when creating management cluster, if the VSphereCPIConfig is created before the Cluster object

Which issue(s) this PR fixes

Fixes ##3317

Describe testing done for PR

Enqueue corresponding VSphereCPIConfig when there is Cluster event.
Also error out when VSphereCPIConfig's owner Clusteris not found

Release note

VSphereCPIConfig Controller watch on Cluster event now

Additional information

Special notes for your reviewer

addons/controllers/cpi/vspherecpiconfig_controller.go

addons/controllers/cpi/vspherecpiconfig_utils.go

github-actions · 2022-09-13T18:10:35Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3318/20220913181033/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 · 2022-09-13T20:25:23Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3318/20220913202520/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 · 2022-09-13T21:10:20Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3318/20220913211018/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 · 2022-09-13T22:21:15Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3318/20220913222114/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 · 2022-09-14T02:26:35Z

Codecov Report

Merging #3318 (a685a40) into main (c9c5555) will not change coverage.
The diff coverage is n/a.

❗ Current head a685a40 differs from pull request most recent head 975271d. Consider uploading reports for the commit 975271d to get more accurate results

@@           Coverage Diff           @@
##             main    #3318   +/-   ##
=======================================
  Coverage   46.63%   46.63%           
=======================================
  Files         281      281           
  Lines       29654    29654           
=======================================
  Hits        13828    13828           
  Misses      14569    14569           
  Partials     1257     1257

Impacted Files	Coverage Δ
addons/pkg/util/cluster_util.go	`4.34% <0.00%> (ø)`
addons/webhooks/cluster_pause_webhook.go	`80.00% <0.00%> (ø)`
addons/controllers/clusterbootstrap_controller.go	`63.52% <0.00%> (ø)`
...ons/controllers/packageinstallstatus_controller.go	`79.15% <0.00%> (ø)`
addons/pkg/webhooks/webhooks.go
addons/pkg/webhooks/certificates.go
pkg/v1/webhooks/certificates.go	`15.95% <0.00%> (ø)`
pkg/v1/webhooks/webhooks.go	`0.00% <0.00%> (ø)`

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

DanielXiao · 2022-09-15T06:23:48Z

Not related to this PR, but I think we should only use 1 way either name or ownerreference to match its owner cluster. If 2 configs belong to the same cluster, how should we handle it?

lubronzhan · 2022-09-15T17:18:27Z

Not related to this PR, but I think we should only use 1 way either name or ownerreference to match its owner cluster. If 2 configs belong to the same cluster, how should we handle it?

That's an issue with current design. I created a Jira for it

github-actions · 2022-09-15T18:09:05Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3318/20220915180903/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 · 2022-09-15T18:10:41Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3318/20220915181039/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 · 2022-09-15T18:11:11Z

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3318/20220915181109/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.

HanFa

lgtm

Error out when the owner cluster of a VSphereCPIConfig is not found Fix logger Resolve conmment Enqueue VSphereCluster intead of Cluster Fix readme for lint Rmove log Fix the cluster type Skip cpiconfig if it's template Add

haoranleo

Looks good, thanks for adding filters for template config in the event handler!

lubronzhan · 2022-09-20T18:09:02Z

Thanks @HL-EverGreen could you take a look at this simialr PR as well? https://github.com/vmware-tanzu/tanzu-framework/pull/3377/files

12345lcr · 2022-09-21T10:32:43Z

addons/controllers/cpi/vspherecpiconfig_utils.go

+				Name:       cluster.Name,
+				UID:        cluster.UID,
+			}
+			if clusterapiutil.HasOwnerRef(config.OwnerReferences, ownerReference) || config.Name == fmt.Sprintf("%s-%s-package", cluster.Name, constants.CPIAddonName) {


I am confused with fmt.Sprintf("%s-%s-package", cluster.Name, constants.CPIAddonName) as package name and cluster name as package name.

Here we are filtering the cpiConfig to enqueue those who has ownRef to an existing clusters, or who has the "clustername"-"package-prefix"-package name where the cluster is exist.

But in below we only reconcile those who has ownRef to an existing clusters, or who has the name of an existing cluster.

https://github.com/lubronzhan/tanzu-framework/blob/975271dee7d6ecb0c60fc271d1d29805ecf6f18c/addons/controllers/cpi/vspherecpiconfig_utils.go#L403-L427

And ALL the AddonConfig test cases are based on cluster name as cpiConfig name for controller testing. Like

https://github.com/lubronzhan/tanzu-framework/blob/975271dee7d6ecb0c60fc271d1d29805ecf6f18c/addons/controllers/testdata/test-vsphere-cpi-non-paravirtual.yaml

This means after this fix, the user customized AddonConfig which has name of the cluster name will never be enqueued base on the cluster event. But also this cluster name based reconcile seems useless now, since it will be copied to "cluster name"-"package prefix"-package for actual use.

The confusion between cluster name as AddonConfig name and "cluster name"-"package prefix"-package as AddonConfig name has bring a lot of unsync, I think we really should make this clear.

Per my understanding, the name format of "cluster name"-"package prefix"-package was never a part of the design doc, yet originally generated based on the copy of the ClusterBootstrapTemplate. All other customized cases are following the cluster name as AddonConfig name. Now this "cluster name"-"package prefix"-package naming conversion becomes primary, that the Bootstrap Controller will copy both the customized and the default provider configs as this pattern. I think maybe we should eliminate the concept of one of them totally.

cc @vijaykatam @DanielXiao @HanFa

We are relying on "cluster name"-"package shortName"-package now. The original design was for "cluster name" but a refactor(I think cloning for webhook) changed it. I would like us to continue down this path of updated name because this has already shipped with vSphere 8.0 integration.

Edit: Docs were updated to reflect this.https://github.com/vmware-tanzu/tanzu-framework/tree/main/addons#pre-create-specific-config-cr

lubronzhan requested review from a team as code owners September 12, 2022 23:32

vmwclabot added the cla-not-required label Sep 12, 2022

lubronzhan force-pushed the enqueue_cluster_changes branch from 111fb84 to 93f4e3d Compare September 12, 2022 23:37

HanFa reviewed Sep 13, 2022

View reviewed changes

addons/controllers/cpi/vspherecpiconfig_controller.go Outdated Show resolved Hide resolved

HanFa reviewed Sep 13, 2022

View reviewed changes

addons/controllers/cpi/vspherecpiconfig_utils.go Outdated Show resolved Hide resolved

HanFa reviewed Sep 13, 2022

View reviewed changes

addons/controllers/cpi/vspherecpiconfig_utils.go Show resolved Hide resolved

DanielXiao reviewed Sep 13, 2022

View reviewed changes

addons/controllers/cpi/vspherecpiconfig_utils.go Outdated Show resolved Hide resolved

lubronzhan force-pushed the enqueue_cluster_changes branch 2 times, most recently from 7d05c22 to 89ae2a4 Compare September 13, 2022 17:58

lubronzhan requested review from a team as code owners September 13, 2022 17:58

lubronzhan force-pushed the enqueue_cluster_changes branch from 6f367f1 to e736d07 Compare September 13, 2022 21:00

lubronzhan force-pushed the enqueue_cluster_changes branch 2 times, most recently from f7e72b2 to 0b2ebb4 Compare September 15, 2022 17:56

lubronzhan force-pushed the enqueue_cluster_changes branch 2 times, most recently from 7deb5d9 to 3beb3f5 Compare September 16, 2022 00:11

lubronzhan mentioned this pull request Sep 16, 2022

Enqueue AddonConfig if Cluster event happens #3377

Merged

lubronzhan force-pushed the enqueue_cluster_changes branch 2 times, most recently from a765d3c to 50458d5 Compare September 19, 2022 01:00

HanFa approved these changes Sep 19, 2022

View reviewed changes

CPIConfigController also Enqueue cluster event

975271d

Error out when the owner cluster of a VSphereCPIConfig is not found Fix logger Resolve conmment Enqueue VSphereCluster intead of Cluster Fix readme for lint Rmove log Fix the cluster type Skip cpiconfig if it's template Add

lubronzhan force-pushed the enqueue_cluster_changes branch from 50458d5 to 975271d Compare September 20, 2022 17:40

haoranleo approved these changes Sep 20, 2022

View reviewed changes

haoranleo added the ok-to-merge PRs should be labelled with this before merging label Sep 20, 2022

lubronzhan merged commit 331b259 into vmware-tanzu:main Sep 20, 2022

lubronzhan deleted the enqueue_cluster_changes branch September 20, 2022 23:41

12345lcr reviewed Sep 21, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPIConfigController also Enqueue cluster event #3318

CPIConfigController also Enqueue cluster event #3318

lubronzhan commented Sep 12, 2022 •

edited

Loading

github-actions bot commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

codecov bot commented Sep 14, 2022 •

edited

Loading

DanielXiao commented Sep 15, 2022

lubronzhan commented Sep 15, 2022

github-actions bot commented Sep 15, 2022

github-actions bot commented Sep 15, 2022

github-actions bot commented Sep 15, 2022

HanFa left a comment

haoranleo left a comment

lubronzhan commented Sep 20, 2022

12345lcr Sep 21, 2022 •

edited

Loading

12345lcr Sep 21, 2022 •

edited

Loading

vijaykatam Sep 21, 2022 •

edited

Loading

CPIConfigController also Enqueue cluster event #3318

CPIConfigController also Enqueue cluster event #3318

Conversation

lubronzhan commented Sep 12, 2022 • edited Loading

What this PR does / why we need it

Which issue(s) this PR fixes

Describe testing done for PR

Release note

Additional information

Special notes for your reviewer

github-actions bot commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

codecov bot commented Sep 14, 2022 • edited Loading

Codecov Report

DanielXiao commented Sep 15, 2022

lubronzhan commented Sep 15, 2022

github-actions bot commented Sep 15, 2022

github-actions bot commented Sep 15, 2022

github-actions bot commented Sep 15, 2022

HanFa left a comment

Choose a reason for hiding this comment

haoranleo left a comment

Choose a reason for hiding this comment

lubronzhan commented Sep 20, 2022

12345lcr Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

12345lcr Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

vijaykatam Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

lubronzhan commented Sep 12, 2022 •

edited

Loading

codecov bot commented Sep 14, 2022 •

edited

Loading

12345lcr Sep 21, 2022 •

edited

Loading

12345lcr Sep 21, 2022 •

edited

Loading

vijaykatam Sep 21, 2022 •

edited

Loading