feat: Delete machine #40

Callisto13 · 2021-11-30T12:24:01Z

What this PR does / why we need it:

Implements MicroVMMachine deletion reconcile.
Calls the flintlock API.

LMK if I have missed any CAPI-type things, was
mostly guessing.

Also removes finalizers from the Cluster controller
as we do not do any supporting infrastructure
creation there. We can add this back later if it changes.

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 #13

Special notes for your reviewer:

Successfully ran a manual create+delete on my local machine.

Checklist:

squashed commits into logical changes
includes documentation
adds unit tests
adds or updates e2e tests

richardcase

This looks good and its good to remove the cluster finalizers.

api/v1alpha1/condition_consts.go

controllers/microvmmachine_controller.go

richardcase · 2021-12-06T16:52:06Z

controllers/microvmmachine_controller.go

+			machineScope.SetNotReady(infrav1.MicrovmDeleteFailedReason, clusterv1.ConditionSeverityError, "")
+
+			return ctrl.Result{}, err
+		}


After the call to Delete should we requeue and on the next reconciliation check the status? Flintlock may have an issue itself reconciling the delete and so we should wait until its actually been deleted. wdyt?

We should check in flintlock that the Get has distinct state detrmination for deleting vs deleted.

And then only remove the finalizer when it gets to a deleted / not found state ?

👍 makes sense

Flintlock does not have any state of deleted or deleting. "Deleted" is unnecessary, because where would we record that after state is deleted. We could introduce a "deleting" phase and check that here, only issuing a Delete call if not already processing.

Flintlock does do its own "should I delete" check of course, but it's good to avoid redundant API calls if we can.

For now I am just going to requeue after the Delete call. If the FL delete fails and the requeue comes around again, flintlock should not make a second attempt in the plan.

I will add a ticket to Flintlock to discuss a "deleting" phase on the state.

jmickey

My understanding of CAPI stuff is pretty limited still, but this looks ok to me.

Callisto13 · 2021-12-09T14:07:13Z

Ty @jmickey. @yitsushi do you want to have a look (now that you have spent some time with capmvm)?

Not too bothered if we are all a bit " 🤷‍♀️ yolo" with this one, I mainly want to get the core lifecycle unblocked so we can work a bit more fluidly here.

yitsushi

lgtm (only that nested if that hurts my precious eyes a bit)

controllers/microvmmachine_controller.go

We do not currently create any supporting infrastructure for our machines. We can add this back if we need it.

github-actions bot added the size/s label Nov 30, 2021

Callisto13 added the kind/feature New feature or request label Nov 30, 2021

richardcase force-pushed the machine_controller branch 8 times, most recently from 3bad20a to ad1bbb3 Compare December 6, 2021 15:02

Base automatically changed from machine_controller to main December 6, 2021 15:08

Callisto13 force-pushed the delete-machine branch from 5014a02 to 0aeab0e Compare December 6, 2021 15:34

Callisto13 changed the title ~~[WIP] feat: Delete machine~~ feat: Delete machine Dec 6, 2021

Callisto13 force-pushed the delete-machine branch from 0aeab0e to 777c22c Compare December 6, 2021 15:39

richardcase reviewed Dec 6, 2021

View reviewed changes

Callisto13 force-pushed the delete-machine branch 2 times, most recently from 4a55a12 to 26bbef2 Compare December 7, 2021 10:28

github-actions bot added size/m and removed size/s labels Dec 7, 2021

Callisto13 force-pushed the delete-machine branch from 26bbef2 to 55fa34a Compare December 9, 2021 13:43

jmickey previously approved these changes Dec 9, 2021

View reviewed changes

Callisto13 dismissed jmickey’s stale review via e0ca8a6 December 9, 2021 15:47

Callisto13 force-pushed the delete-machine branch from 55fa34a to e0ca8a6 Compare December 9, 2021 15:47

yitsushi previously approved these changes Dec 9, 2021

View reviewed changes

controllers/microvmmachine_controller.go Outdated Show resolved Hide resolved

Callisto13 added 2 commits December 10, 2021 10:14

feat: Microvm deletion

2879582

Remove finalizers from Cluster

cdae6cd

We do not currently create any supporting infrastructure for our machines. We can add this back if we need it.

Callisto13 dismissed yitsushi’s stale review via cdae6cd December 10, 2021 10:14

Callisto13 force-pushed the delete-machine branch from e0ca8a6 to cdae6cd Compare December 10, 2021 10:14

yitsushi approved these changes Dec 10, 2021

View reviewed changes

Callisto13 merged commit cbc4042 into main Dec 10, 2021

Callisto13 deleted the delete-machine branch December 10, 2021 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Delete machine #40

feat: Delete machine #40

Callisto13 commented Nov 30, 2021 •

edited

Loading

richardcase left a comment

richardcase Dec 6, 2021

richardcase Dec 6, 2021

Callisto13 Dec 6, 2021

Callisto13 Dec 7, 2021

jmickey left a comment

Callisto13 commented Dec 9, 2021 •

edited

Loading

yitsushi left a comment

feat: Delete machine #40

feat: Delete machine #40

Conversation

Callisto13 commented Nov 30, 2021 • edited Loading

richardcase left a comment

Choose a reason for hiding this comment

richardcase Dec 6, 2021

Choose a reason for hiding this comment

richardcase Dec 6, 2021

Choose a reason for hiding this comment

Callisto13 Dec 6, 2021

Choose a reason for hiding this comment

Callisto13 Dec 7, 2021

Choose a reason for hiding this comment

jmickey left a comment

Choose a reason for hiding this comment

Callisto13 commented Dec 9, 2021 • edited Loading

yitsushi left a comment

Choose a reason for hiding this comment

Callisto13 commented Nov 30, 2021 •

edited

Loading

Callisto13 commented Dec 9, 2021 •

edited

Loading