make deprovisioning an instance asynchronously not fall-through to synchronous deprovision

[kibbles-n-bytes]

No description provided.

[pmorie]

Can we get a test for this?

[kibbles-n-bytes]

@pmorie Yup, working on that now!

[kibbles-n-bytes]

@pmorie Yup, working on that now!

[kibbles-n-bytes]

I made a few design choices, would like feedback as to whether they're valid.

Currently in master, asynchronously deprovisioning sets an async-only condition, and then falls back onto the regular deprovision logic, where the condition is set as successfully deprovisioned, a successful deprovision event is recorded, and the finalizer is cleared.

This seemed strange to me. The instance hasn't actually been completely prepped for K8s-side deletion, so clearing the finalizer seems wrong. And setting the InstanceConditionReady as false already occurs in the async-only step, so doing a second condition seemed odd. And I'm not entirely sure of when it's appropriate to record an event, but recording a successful deprovision event also seemed misleading. So I added a return in the async section that does none of them. Is this alright? Should we be recording a different event type here?

[kibbles-n-bytes]

I made a few design choices, would like feedback as to whether they're valid.

Currently in master, asynchronously deprovisioning sets an async-only condition, and then falls back onto the regular deprovision logic, where the condition is set as successfully deprovisioned, a successful deprovision event is recorded, and the finalizer is cleared.

This seemed strange to me. The instance hasn't actually been completely prepped for K8s-side deletion, so clearing the finalizer seems wrong. And setting the InstanceConditionReady as false already occurs in the async-only step, so doing a second condition seemed odd. And I'm not entirely sure of when it's appropriate to record an event, but recording a successful deprovision event also seemed misleading. So I added a return in the async section that does none of them. Is this alright? Should we be recording a different event type here?

[kibbles-n-bytes]

It seems like async provisioning does record an event, so I'll record one for deprovisioning too.

[kibbles-n-bytes]

It seems like async provisioning does record an event, so I'll record one for deprovisioning too.

[kibbles-n-bytes]

Since #1074 is now in, the scope of this PR changes. Now it is only preventing the asynchronous deprovision logic from falling through to the rest of the synchronous deprovision logic, and adds a test (and typo fix).

[kibbles-n-bytes]

Since #1074 is now in, the scope of this PR changes. Now it is only preventing the asynchronous deprovision logic from falling through to the rest of the synchronous deprovision logic, and adds a test (and typo fix).

[MHBauer]

seems okay.

I am not liking this "gotta make sure we check in the right order".

LGTM

[kibbles-n-bytes]

@MHBauer Not sure if there's a clean way around it. There's no single thing you can look at to see what should be done to reconcile the instance. We could specify the different actions to take as:

shouldPoll = instance.Status.AsyncOpInProgress
shouldDelete = !instance.Status.AsyncOpInProgress && instance.ObjectMeta.DeletionTimestamp != nil
shouldAddOrUpdate = !instance.StatusAsyncOpInProgress && instance.ObjectMeta.DeletionTimestamp == nil

But that's pretty ugly IMO. I'm planning to work on refactoring the polling queue logic anyway though, so I'll try to see if there's a better place we can place the poll/not poll logic at least. But that'll have to be in a followup.

[kibbles-n-bytes]

@MHBauer Not sure if there's a clean way around it. There's no single thing you can look at to see what should be done to reconcile the instance. We could specify the different actions to take as:

shouldPoll = instance.Status.AsyncOpInProgress
shouldDelete = !instance.Status.AsyncOpInProgress && instance.ObjectMeta.DeletionTimestamp != nil
shouldAddOrUpdate = !instance.StatusAsyncOpInProgress && instance.ObjectMeta.DeletionTimestamp == nil

But that's pretty ugly IMO. I'm planning to work on refactoring the polling queue logic anyway though, so I'll try to see if there's a better place we can place the poll/not poll logic at least. But that'll have to be in a followup.

[MHBauer]

That's why I keep trying to push this state machine idea. We should know, coming in, which fork we're going down, and go down only that fork, before moving to the next state. I'm not sure if it can be done by sub parts of the condition or what. What makes the controller adjustments painful is we have to make sure to return if we enter one of these blocks, make sure to change something (or not) so we don't enter the same block the next go around, make sure sure that nothing in one block can effect any other block in the same invocation, etc. Maybe extracting will fix it. I don't know.

[MHBauer]

That's why I keep trying to push this state machine idea. We should know, coming in, which fork we're going down, and go down only that fork, before moving to the next state. I'm not sure if it can be done by sub parts of the condition or what. What makes the controller adjustments painful is we have to make sure to return if we enter one of these blocks, make sure to change something (or not) so we don't enter the same block the next go around, make sure sure that nothing in one block can effect any other block in the same invocation, etc. Maybe extracting will fix it. I don't know.

[pmorie]

One nit - what do you think?

[pmorie]

if you move this up to L328, we can remove a level of nesting in the checksum check

[kibbles-n-bytes]

Ah yeah, good catch!

[kibbles-n-bytes]

@MHBauer Can you write up an issue describing a bit of what your state machine concept would look like? I'm curious and would like to wrap my head around it, so if you're serious about it I'd like to get the ball rolling so we can discuss pros/cons/implementation/etc.

[kibbles-n-bytes]

@MHBauer Can you write up an issue describing a bit of what your state machine concept would look like? I'm curious and would like to wrap my head around it, so if you're serious about it I'd like to get the ball rolling so we can discuss pros/cons/implementation/etc.

[kibbles-n-bytes]

FYI @ALL: @pmorie gave me a verbal LGTM to this PR, but said because he was on a plane about to take off, he wasn't able to add the badge and merge on GitHub. I'm going to go ahead and do so as a proxy.

[kibbles-n-bytes]

FYI @ALL: @pmorie gave me a verbal LGTM to this PR, but said because he was on a plane about to take off, he wasn't able to add the badge and merge on GitHub. I'm going to go ahead and do so as a proxy.

[nilebox]

@kibbles-n-bytes just found this bug myself, and was happy to find that it has already been fixed in the latest release :) 👍

[nilebox]

@kibbles-n-bytes just found this bug myself, and was happy to find that it has already been fixed in the latest release :) 👍