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

TKR webhook should not reject a paused cluster. #3724

Merged
merged 1 commit into from
Oct 21, 2022

Conversation

HanFa
Copy link
Contributor

@HanFa HanFa commented Oct 20, 2022

What this PR does / why we need it

A fix cherrypicked from #3557

Generally, the webhook should always admit a Cluster resource that is paused.

Oracle MC deletion is blocked without this change

When deleting a management cluster, the CAPI Cluster resource will be first paused an moved to the bootstrap kind cluster. During this process, we can not guarantee that the cluster's metadata is resolve-able, because its corresponding TKR might not be there yet.

In fact, the webhook is rejecting the Cluster CR when deleting the MC, if that MC created was using --additional-manifests <some-tkr-url>. This TKR from manifests won't be presented in the Bootstrap Kind cluster during deletion.

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

Webhook unit test

Release note

TKR webhook should not reject a paused cluster.

Additional information

Special notes for your reviewer

@HanFa HanFa requested a review from a team as a code owner October 20, 2022 23:25
@HanFa
Copy link
Contributor Author

HanFa commented Oct 20, 2022

@imikushin @randomvariable any thoughts on this proposed fix?

@codecov
Copy link

codecov bot commented Oct 21, 2022

Codecov Report

Merging #3724 (b6bd50c) into main (0f0df6f) will decrease coverage by 0.90%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3724      +/-   ##
==========================================
- Coverage   46.22%   45.32%   -0.91%     
==========================================
  Files         400      425      +25     
  Lines       39620    41176    +1556     
==========================================
+ Hits        18316    18661     +345     
- Misses      19617    20807    +1190     
- Partials     1687     1708      +21     
Impacted Files Coverage Δ
addons/controllers/machine_controller.go 65.65% <0.00%> (-3.04%) ⬇️
addons/controllers/clusterbootstrap_controller.go 63.85% <0.00%> (-1.02%) ⬇️
cmd/cli/plugin/cluster/set_machinehealthcheck.go 23.33% <0.00%> (ø)
cmd/cli/plugin/cluster/get_machinehealthcheck.go 11.42% <0.00%> (ø)
cmd/cli/plugin/cluster/available_upgrade.go 16.32% <0.00%> (ø)
cmd/cli/plugin/cluster/osimage_oracle.go 3.22% <0.00%> (ø)
...in/cluster/set_machinehealthcheck_control_plane.go 21.21% <0.00%> (ø)
cmd/cli/plugin/cluster/machinehealthcheck.go 100.00% <0.00%> (ø)
cmd/cli/plugin/cluster/osimage.go 100.00% <0.00%> (ø)
cmd/cli/plugin/cluster/get.go 6.27% <0.00%> (ø)
... and 18 more

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

Copy link
Contributor

@imikushin imikushin left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks @HanFa

@imikushin imikushin added the ok-to-merge PRs should be labelled with this before merging label Oct 21, 2022
@HanFa HanFa merged commit b1a728f into main Oct 21, 2022
@HanFa HanFa deleted the topic/fhan/tkr-webhook-accepts-paused-cluster branch October 21, 2022 22:03
@@ -65,6 +65,10 @@ func (cw *Webhook) Handle(ctx context.Context, req admission.Request) admission.
}

func (cw *Webhook) getClusterClass(ctx context.Context, cluster *clusterv1.Cluster) (*clusterv1.ClusterClass, *admission.Response) {
if cluster.Spec.Paused {
Copy link
Contributor Author

@HanFa HanFa Oct 25, 2022

Choose a reason for hiding this comment

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

Suggested by @imikushin

During management cluster deletion, the CLI should

  1. replicate CM that has label run.tanzu.vmware.com/additional-compatible-tkrs: "" (not needed)
  2. replicate a single TKR (specified by the cluster object label) and referenced OSimages from MC to the cleanup cluster.
  3. transplant ClusterClass and Cluster (this will be handled by the CAPI)

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants