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

Replicate TKR and OSImage to cleanup cluster during management cluster deletion. #3755

Merged
merged 1 commit into from
Nov 1, 2022

Conversation

HanFa
Copy link
Contributor

@HanFa HanFa commented Oct 26, 2022

What this PR does / why we need it

A follow-up of #3724, cherrypicked from #3557

During the management cluster deletion, if the user has brought their own TKR and OSImages, then in the cleanup cluster, the moved Cluster CR will be rejected by the TKR cluster webhook. A sample error looks like,

error: clusters.cluster.x-k8s.io "test-lb-cluster" could not be patched: admission webhook "tkr-resolver-cluster-webhook.tanzu.vmware.com" denied the request: could not resolve TKR/OSImage for controlPlane, machineDeployments: [md-0], q
uery: {controlPlane: {k8sVersionPrefix: 'v1.23.8+vmware.2', tkrSelector: '', osImageSelector: 'oci-region=us-phoenix-1,os-name=ubuntu'}, machineDeployments: [{k8sVersionPrefix: 'v1.23.8+vmware.2', tkrSelector: '', osImageSelector: 'oci-
region=us-phoenix-1,os-name=ubuntu'}]}, result: {controlPlane: {k8sVersion: '', tkrName: '', osImagesByTKR: map[]}, machineDeployments: [{k8sVersion: '', tkrName: '', osImagesByTKR: map[]}]}
You can run `kubectl replace -f /tmp/kubectl-edit-1292888427.yaml` to try this update again.

This is because the TKR Cluster webhook rejects the moved Cluster CR because of lacking the self-brought TKR and OSImages in the cleanup cluster.

Therefore, when performing region deletion, the tanzu CLI should be responsible for replicating the TKR and OSImages that is associated with the moved management cluster. This PR adds this extra step.

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

With this fix presented at #3557, the Oracle management cluster is able to be deleted successfully and the webhook no longer rejects with moved Cluster.

Release note

package-based-lcm: Replicate TKR and OSImage to cleanup cluster during management cluster deletion.

Additional information

Special notes for your reviewer

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3755/20221026030629/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 HanFa force-pushed the topic/fhan/mc-deletion-replicate-tkr branch from 817c634 to 87f3f63 Compare October 26, 2022 18:23
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3755/20221026183017/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 Oct 26, 2022

Codecov Report

Merging #3755 (e1f3059) into main (7fc5101) will decrease coverage by 0.87%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #3755      +/-   ##
==========================================
- Coverage   46.58%   45.70%   -0.88%     
==========================================
  Files         400      425      +25     
  Lines       39710    41336    +1626     
==========================================
+ Hits        18497    18891     +394     
- Misses      19523    20737    +1214     
- Partials     1690     1708      +18     
Impacted Files Coverage Δ
tkg/client/delete_region.go 0.00% <0.00%> (ø)
tkg/client/init.go 0.00% <0.00%> (ø)
tkg/clusterclient/clusterclient.go 48.49% <0.00%> (-0.85%) ⬇️
tkg/client/machine_deployment_cc.go 70.61% <0.00%> (-0.67%) ⬇️
cmd/cli/plugin/package/main.go 0.00% <0.00%> (ø)
cmd/cli/plugin/cluster/delete.go 12.50% <0.00%> (ø)
cmd/cli/plugin/cluster/main.go 0.00% <0.00%> (ø)
cmd/cli/plugin/cluster/kubeconfig_get.go 8.82% <0.00%> (ø)
...i/plugin/cluster/delete_machinehealthcheck_node.go 16.66% <0.00%> (ø)
cmd/cli/plugin/cluster/get.go 6.27% <0.00%> (ø)
... and 25 more

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

@HanFa HanFa requested a review from XudongLiuHarold October 27, 2022 00:37
@HanFa HanFa force-pushed the topic/fhan/mc-deletion-replicate-tkr branch from 87f3f63 to f5aca00 Compare October 27, 2022 05:07
@github-actions
Copy link

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

tkg/client/delete_region.go Outdated Show resolved Hide resolved
tkg/client/init.go Outdated Show resolved Hide resolved
tkg/client/init.go Outdated Show resolved Hide resolved
tkg/client/init.go Outdated Show resolved Hide resolved
tkg/client/init.go Outdated Show resolved Hide resolved
tkg/clusterclient/clusterclient.go Outdated Show resolved Hide resolved
@HanFa HanFa force-pushed the topic/fhan/mc-deletion-replicate-tkr branch from f5aca00 to f505bb4 Compare October 28, 2022 21:38
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3755/20221028214725/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 HanFa force-pushed the topic/fhan/mc-deletion-replicate-tkr branch from f505bb4 to beaeb21 Compare October 31, 2022 01:01
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3755/20221031011219/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 HanFa force-pushed the topic/fhan/mc-deletion-replicate-tkr branch from beaeb21 to e1f3059 Compare October 31, 2022 04:40
@github-actions
Copy link

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

Copy link
Member

@XudongLiuHarold XudongLiuHarold left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM

@randomvariable randomvariable added the ok-to-merge PRs should be labelled with this before merging label Nov 1, 2022
@randomvariable randomvariable merged commit bb7611b into main Nov 1, 2022
@randomvariable randomvariable deleted the topic/fhan/mc-deletion-replicate-tkr branch November 1, 2022 22:08
# 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.

5 participants