🌱 Remove handling for orphaned VSphereMachines #2429

killianmuldoon · 2023-10-12T17:00:42Z

Remove special handling for orphaned VSphereMachines. VSphereMachines should no longer be orphaned by CAPI MachineSets on creation so this safeguard is no longer needed.

Fixes #2054

codecov · 2023-10-13T11:28:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@a1db12e). Click here to learn what that means.
Report is 62 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2429   +/-   ##
=======================================
  Coverage        ?   63.93%           
=======================================
  Files           ?      123           
  Lines           ?     8731           
  Branches        ?        0           
=======================================
  Hits            ?     5582           
  Misses          ?     2741           
  Partials        ?      408

Files	Coverage Δ
controllers/vspherecluster_reconciler.go	`67.76% <ø> (ø)`
controllers/vspheremachine_controller.go	`77.38% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

killianmuldoon · 2023-10-13T11:37:47Z

/retest

sbueringer · 2023-10-23T12:11:21Z

/lgtm

/assign @chrischdi

@killianmuldoon Would it be possible to test this once manually or with the clusterctl upgrade test? (Just to get some additional confidence)

k8s-ci-robot · 2023-10-23T12:11:27Z

LGTM label has been added.

Git tree hash: 547100069e49269d72c4bd3262499a6ddcd8b4bf

sbueringer · 2023-10-23T12:12:59Z

controllers/vspherecluster_reconciler.go

@@ -158,12 +154,6 @@ func (r *clusterReconciler) reconcileDelete(ctx context.Context, clusterCtx *cap
 		log := log.WithValues("VSphereMachine", klog.KObj(vsphereMachine))
 		ctx := ctrl.LoggerInto(ctx, log)



Missed this before. Shouldn't we be able to drop the entire finalizer handling here?

xref: https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/pull/1284/files

cc @chrischdi

You're right - we can get rid of the entire block that deals with deleting Machines here.

sbueringer · 2023-10-23T12:18:01Z

cc @srm09

killianmuldoon · 2023-10-23T12:45:39Z

/hold

Will test this manually

sbueringer · 2023-10-24T10:01:00Z

Will test this manually

WDYT about #2429 (comment)?

killianmuldoon · 2023-11-01T15:53:01Z

/test

k8s-ci-robot · 2023-11-01T15:53:04Z

@killianmuldoon: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-vsphere-conformance-main
/test pull-cluster-api-provider-vsphere-e2e-full-main
/test pull-cluster-api-provider-vsphere-e2e-main
/test pull-cluster-api-provider-vsphere-test-integration-main
/test pull-cluster-api-provider-vsphere-test-main
/test pull-cluster-api-provider-vsphere-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-vsphere-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-vsphere-apidiff-main
pull-cluster-api-provider-vsphere-e2e-main
pull-cluster-api-provider-vsphere-test-integration-main
pull-cluster-api-provider-vsphere-test-main
pull-cluster-api-provider-vsphere-verify-main

In response to this:

/test

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 · 2023-11-02T11:52:11Z

LGTM label has been added.

Git tree hash: 9b2b2e4f1ef12d07edfb17895569cc981a719577

killianmuldoon · 2023-11-02T11:57:28Z

One questions: This PR builds up on the base, that a user did first upgraded to a CAPV version >= v1.8.x , so owner refs are all okay, right?

Yeah - exactly we should consider this directly a part of the ownerReferences fixes.

chrischdi · 2023-11-02T12:05:42Z

Sounds great!

/approve

There's a still a hold on it :-)

chrischdi

one left nit

/approve cancel

chrischdi · 2023-11-02T12:07:29Z

controllers/vspheremachine_controller.go

+	// If the VSphereCluster has been previously set as an ownerReference remove it. This may have been set in an older
+	// version of CAPV to prevent VSphereMachines from being orphaned, but is no longer needed.
+	// For more info see: https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/issues/2054
+	// TODO (killianmuldoon): Decide when this can be removed.


Should we remove this comment or code? :-)

This was meant to be a bit longer term - i.e. this is migration code and eventually when all existing Clusters have passed through this migration we shouldn't need it any more. I've updated this to be a bit clearer, without asserting when it should be removed.

chrischdi · 2023-11-02T12:10:01Z

controllers/vspherecluster_reconciler.go

-		machineDeletionCount++
-		// Remove the finalizer since VM creation wouldn't proceed
-		log.Info("Removing finalizer from VSphereMachine")
-		ctrlutil.RemoveFinalizer(vsphereMachine, infrav1.MachineFinalizer)


Don't we still need something which removes this finalizer? Or should this already have happened a longer time ago?

You're right - let me put this on [WIP] - I'm still trying to test it properly.

This finalizer is going to be removed in the vspheremachine_controller at

cluster-api-provider-vsphere/controllers/vspheremachine_controller.go

Lines 285 to 289 in 959691c

if apierrors.IsNotFound(err) {

// The VM is deleted so remove the finalizer.

ctrlutil.RemoveFinalizer(machineCtx.GetVSphereMachine(), infrav1.MachineFinalizer)

return reconcile.Result{}, nil

}

Previously the VSphereCluster was able to do a full cleanup in case VSphereMachines were orphaned. They shouldn't be able to become orphaned now as CAPI ensure InfrastructureMachines should always have an ownerReference.

killianmuldoon · 2023-11-07T16:28:27Z

One questions: This PR builds up on the base, that a user did first upgraded to a CAPV version >= v1.8.x , so owner refs are all okay, right?

One comment on this - contradicting what I said above 😅

The migration here is independent from the owner reference migration in v1.8.2 as it doesn't deal with versions. In this case we should keep the owner reference migration code - which removes the VSphereCluster owner reference from the VSphereMachine - for as long as we'd like to to ensure all Clusters are migrated.

controllers/vspheremachine_controller.go

sbueringer · 2023-11-09T14:25:11Z

One final comment

Signed-off-by: killianmuldoon <[email protected]>

killianmuldoon · 2023-11-09T15:19:17Z

/retest

sbueringer · 2023-11-09T23:14:37Z

Thx!

/lgtm
/approve

Feel free to hold cancel

k8s-ci-robot · 2023-11-09T23:14:42Z

LGTM label has been added.

Git tree hash: c049b412588caf3dacf7f3350536139e7aec5bc1

k8s-ci-robot · 2023-11-09T23:14:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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:

~~OWNERS~~ [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

killianmuldoon · 2023-11-10T09:46:37Z

/hold cancel

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 12, 2023

killianmuldoon changed the title ~~🌱 Remove handling for orphaned VSphereMachines~~ [WIP] 🌱 Remove handling for orphaned VSphereMachines Oct 12, 2023

k8s-ci-robot requested review from fabriziopandini and gab-satchi October 12, 2023 17:00

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 12, 2023

killianmuldoon changed the title ~~[WIP] 🌱 Remove handling for orphaned VSphereMachines~~ 🌱 Remove handling for orphaned VSphereMachines Oct 13, 2023

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 Oct 13, 2023

killianmuldoon force-pushed the pr-remove-orphaned-machine-handling branch from 93511ad to c242fe5 Compare October 13, 2023 10:59

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 Oct 13, 2023

killianmuldoon force-pushed the pr-remove-orphaned-machine-handling branch from c242fe5 to 004b849 Compare October 13, 2023 11:22

k8s-ci-robot assigned chrischdi and sbueringer Oct 23, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2023

sbueringer reviewed Oct 23, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 23, 2023

killianmuldoon force-pushed the pr-remove-orphaned-machine-handling branch from 004b849 to 959691c Compare November 1, 2023 15:51

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2023

k8s-ci-robot requested review from chrischdi and sbueringer November 1, 2023 15:51

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2023

chrischdi reviewed Nov 2, 2023

View reviewed changes

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2023

chrischdi reviewed Nov 2, 2023

View reviewed changes

killianmuldoon changed the title ~~🌱 Remove handling for orphaned VSphereMachines~~ [WIP] 🌱 Remove handling for orphaned VSphereMachines Nov 2, 2023

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 2, 2023

killianmuldoon force-pushed the pr-remove-orphaned-machine-handling branch from 959691c to f4c6b2d Compare November 7, 2023 16:23

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2023

k8s-ci-robot requested a review from chrischdi November 7, 2023 16:23

killianmuldoon changed the title ~~[WIP] 🌱 Remove handling for orphaned VSphereMachines~~ 🌱 Remove handling for orphaned VSphereMachines Nov 7, 2023

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 Nov 7, 2023

sbueringer reviewed Nov 9, 2023

View reviewed changes

controllers/vspheremachine_controller.go Outdated Show resolved Hide resolved

Remove handling for orphaned VSphereMachines

c82f792

Signed-off-by: killianmuldoon <[email protected]>

killianmuldoon force-pushed the pr-remove-orphaned-machine-handling branch from f4c6b2d to c82f792 Compare November 9, 2023 14:54

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2023

k8s-ci-robot merged commit 0ba8082 into kubernetes-sigs:main Nov 10, 2023
17 checks passed

k8s-ci-robot added this to the v1.9 milestone Nov 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Remove handling for orphaned VSphereMachines #2429

🌱 Remove handling for orphaned VSphereMachines #2429

killianmuldoon commented Oct 12, 2023

codecov bot commented Oct 13, 2023 •

edited

Loading

killianmuldoon commented Oct 13, 2023

sbueringer commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

sbueringer Oct 23, 2023

sbueringer Oct 23, 2023

killianmuldoon Nov 1, 2023

sbueringer commented Oct 23, 2023

killianmuldoon commented Oct 23, 2023

sbueringer commented Oct 24, 2023

killianmuldoon commented Nov 1, 2023

k8s-ci-robot commented Nov 1, 2023

k8s-ci-robot commented Nov 2, 2023

killianmuldoon commented Nov 2, 2023

chrischdi commented Nov 2, 2023

chrischdi left a comment

chrischdi Nov 2, 2023

killianmuldoon Nov 7, 2023

chrischdi Nov 2, 2023

killianmuldoon Nov 2, 2023

killianmuldoon Nov 7, 2023 •

edited

Loading

killianmuldoon commented Nov 7, 2023

sbueringer commented Nov 9, 2023

killianmuldoon commented Nov 9, 2023

sbueringer commented Nov 9, 2023

k8s-ci-robot commented Nov 9, 2023

k8s-ci-robot commented Nov 9, 2023

killianmuldoon commented Nov 10, 2023

		@@ -158,12 +154,6 @@ func (r clusterReconciler) reconcileDelete(ctx context.Context, clusterCtx cap
		log := log.WithValues("VSphereMachine", klog.KObj(vsphereMachine))
		ctx := ctrl.LoggerInto(ctx, log)

	if apierrors.IsNotFound(err) {
	// The VM is deleted so remove the finalizer.
	ctrlutil.RemoveFinalizer(machineCtx.GetVSphereMachine(), infrav1.MachineFinalizer)
	return reconcile.Result{}, nil
	}

🌱 Remove handling for orphaned VSphereMachines #2429

🌱 Remove handling for orphaned VSphereMachines #2429

Conversation

killianmuldoon commented Oct 12, 2023

codecov bot commented Oct 13, 2023 • edited Loading

Codecov Report

killianmuldoon commented Oct 13, 2023

sbueringer commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

sbueringer Oct 23, 2023

Choose a reason for hiding this comment

sbueringer Oct 23, 2023

Choose a reason for hiding this comment

killianmuldoon Nov 1, 2023

Choose a reason for hiding this comment

sbueringer commented Oct 23, 2023

killianmuldoon commented Oct 23, 2023

sbueringer commented Oct 24, 2023

killianmuldoon commented Nov 1, 2023

k8s-ci-robot commented Nov 1, 2023

k8s-ci-robot commented Nov 2, 2023

killianmuldoon commented Nov 2, 2023

chrischdi commented Nov 2, 2023

chrischdi left a comment

Choose a reason for hiding this comment

chrischdi Nov 2, 2023

Choose a reason for hiding this comment

killianmuldoon Nov 7, 2023

Choose a reason for hiding this comment

chrischdi Nov 2, 2023

Choose a reason for hiding this comment

killianmuldoon Nov 2, 2023

Choose a reason for hiding this comment

killianmuldoon Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

killianmuldoon commented Nov 7, 2023

sbueringer commented Nov 9, 2023

killianmuldoon commented Nov 9, 2023

sbueringer commented Nov 9, 2023

k8s-ci-robot commented Nov 9, 2023

k8s-ci-robot commented Nov 9, 2023

killianmuldoon commented Nov 10, 2023

codecov bot commented Oct 13, 2023 •

edited

Loading

killianmuldoon Nov 7, 2023 •

edited

Loading