Skip to content
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.

Sign up for GitHub

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

Jump to bottom

🐛 Change delete behaviour to respect inventory #5044

Merged
merged 1 commit into from
Sep 22, 2021
Merged

🐛 Change delete behaviour to respect inventory #5044

merged 1 commit into from
Sep 22, 2021

Conversation

killianmuldoon
Copy link
Contributor

Add an option to the delete functions in clusterctl too allow user
configuration of inventory deletion. Clusterctl no longer deletes
provider inventories during an upgrade. This reduces the chance of
an unrecoverable error during clusterctl upgrade.

Signed-off-by: killianmuldoon [email protected]

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #5015

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 2, 2021
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 2, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @killianmuldoon. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 2, 2021
@killianmuldoon
Copy link
Contributor Author

I've put this in WIP until I figure out a good way to test the actual issue encountered in #5015.

@killianmuldoon killianmuldoon changed the title WIP: Change delete behaviour to respect inventory WIP: :bug Change delete behaviour to respect inventory Aug 2, 2021
@killianmuldoon killianmuldoon changed the title WIP: :bug Change delete behaviour to respect inventory WIP: 🐛 Change delete behaviour to respect inventory Aug 2, 2021
@killianmuldoon killianmuldoon changed the title WIP: 🐛 Change delete behaviour to respect inventory 🐛 WIP: Change delete behaviour to respect inventory Aug 2, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2021
@fabriziopandini
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 2, 2021
fabriziopandini
fabriziopandini reviewed Aug 3, 2021
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I know this is a WIP, feel free to ignore comments if those parts are still being worked on

cmd/clusterctl/client/cluster/components.go Show resolved Hide resolved
cmd/clusterctl/client/cluster/components.go Outdated Show resolved Hide resolved
cmd/clusterctl/client/cluster/components_test.go Show resolved Hide resolved
cmd/clusterctl/client/cluster/components_test.go Outdated
@@ -148,15 +157,17 @@ func Test_providerComponents_Delete(t *testing.T) {
{object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns2", Name: "pod3"}, deleted: false}, // this object is in another namespace, and should never be touched by delete
{object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "ns1-cluster-role"}, deleted: true}, // cluster-wide provider components should be deleted
{object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "some-cluster-role"}, deleted: false}, // other cluster-wide objects should be preserved
{object: corev1.ObjectReference{APIVersion: "clusterctl.cluster.x-k8s.io/v1alpha3", Kind: "Provider", Name: "providerOne"}, deleted: false}, // providerInventory should be preserved
Copy link
Member

Choose a reason for hiding this comment

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

Let use the GroupVersion from the clusterctl API instead of "clusterctl.cluster.x-k8s.io/v1alpha3", so it will be easier to bump our API version in future

cmd/clusterctl/client/delete.go Outdated Show resolved Hide resolved
@fabriziopandini
Copy link
Member

overall lgmt
@ykakarap PTAL

ykakarap
ykakarap reviewed Aug 8, 2021
View reviewed changes
Copy link
Contributor

@ykakarap ykakarap left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the work @killianmuldoon 🙂
PR is LGTM for me besides a nit.

The original idea behind this PR was to make the clusterclt upgrade re-entrant. Can we add some tests around this case to see if it works as expected. May be in a follow-up PR?

cmd/clusterctl/client/cluster/components.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 11, 2021
@killianmuldoon
Copy link
Contributor Author

I've added a test for create in provider components to show that an upgrade / patch of existing components happens when an older version exists at create time. I chose to just use the pod image name to mock fine-grained upgrade of the spec of a component through the Create method.

There's an existing test of this type already under providerInventory in itsTest_inventoryClient_Create.

I think these two combined are enough to show that an upgrade following non-deletion of the provider inventory should perform properly. We're still missing an e2e test design for this change though.

@killianmuldoon killianmuldoon force-pushed the fix/clusterctl-upgrade-delete-inventory branch 2 times, most recently from 7014869 to 583f616 Compare August 18, 2021 13:48
@killianmuldoon
Change delete behaviour to respect inventory
c26190e 
Add an option to the delete functions in clusterctl too allow user
configuration of inventory deletion. Clusterctl no longer deletes
provider inventories during an upgrade. This reduces the chance of
an unrecoverable error during clusterctl upgrade.

Signed-off-by: killianmuldoon <[email protected]>
@killianmuldoon killianmuldoon force-pushed the fix/clusterctl-upgrade-delete-inventory branch from 583f616 to c26190e Compare August 18, 2021 14:39
@killianmuldoon killianmuldoon changed the title 🐛 WIP: Change delete behaviour to respect inventory 🐛 Change delete behaviour to respect inventory Aug 18, 2021
vincepri
vincepri approved these changes Sep 15, 2021
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/assign @fabriziopandini
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2021
fabriziopandini
fabriziopandini reviewed Sep 22, 2021
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2021
@k8s-ci-robot k8s-ci-robot merged commit 91cc865 into kubernetes-sigs:master Sep 22, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.4 milestone Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ykakarap ykakarap ykakarap left review comments

@fabriziopandini fabriziopandini fabriziopandini left review comments

@vincepri vincepri vincepri approved these changes

@enxebre enxebre Awaiting requested review from enxebre

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

Assignees

@vincepri vincepri

@fabriziopandini fabriziopandini

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.

"clusterctl upgrade" fails during retry
5 participants
@killianmuldoon @k8s-ci-robot @fabriziopandini @vincepri @ykakarap