WINC-612: [wmco] Handle deletion of windows-instances #527

aravindhp · 2021-07-19T19:28:50Z

If the admin deletes the windows-instances ConfigMap, the operation is viewed as a call to deconfigure all BYOH Windows instances. This PR handles that scenario.

aravindhp · 2021-07-19T19:35:44Z

/approve cancel
/cc @openshift/openshift-team-windows-containers

aravindhp · 2021-07-19T19:36:01Z

/cherry-pick release-4.8

openshift-cherrypick-robot · 2021-07-19T19:36:01Z

@aravindhp: once the present PR merges, I will cherry-pick it on top of release-4.8 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.8

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.

aravindhp · 2021-07-19T19:36:16Z

/cherry-pick community-4.8

openshift-cherrypick-robot · 2021-07-19T19:36:17Z

@aravindhp: once the present PR merges, I will cherry-pick it on top of community-4.8 in a new PR and assign it to you.

In response to this:

/cherry-pick community-4.8

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.

sebsoto · 2021-07-19T20:02:10Z

controllers/configmap_controller.go

+			// If DeleteStateUnknown is true it implies that the Delete event was missed  and we can ignore it
+			if e.DeleteStateUnknown {
+				return false
+			}


In this case why should we skip the reconciliation? We should still ensure that everything is as expected should we not?

I went with the documented example. Not sure if we should be reacting to objects that have not been fully deleted.

If we are definitely going to get a second event when the object is fully deleted that makes sense to me

So should I remove this check or leave it in?

I was thinking that it would be fine to leave the check, but having looked at the controller-runtime code I am not so sure.

It being true does not mean that it hasnt been fully deleted, but that the event was missed.

// DeleteStateUnknown is true if the Delete event was missed but we identified the object // as having been deleted.

Digging more into how the delete event is actually generated:

How we create a controller: https://github.com/kubernetes-sigs/controller-runtime/blob/865d56a824cc295f4895aca40af48737e188bd0c/alias.go#L101

When we call .Watches(...) on a controller we declare that we want to watch for events for a specific object: https://github.com/kubernetes-sigs/controller-runtime/blob/865d56a824cc295f4895aca40af48737e188bd0c/pkg/source/source.go#L88

The internal.EventHandler is always used: https://github.com/kubernetes-sigs/controller-runtime/blob/865d56a824cc295f4895aca40af48737e188bd0c/pkg/source/source.go#L133

Delete events are handled and DeleteStateUnknown set: https://github.com/kubernetes-sigs/controller-runtime/blob/865d56a824cc295f4895aca40af48737e188bd0c/pkg/source/internal/eventsource.go#L101-L105

// Deal with tombstone events by pulling the object out. Tombstone events wrap the object in a // DeleteFinalStateUnknown struct, so the object needs to be pulled out. // Copied from sample-controller // This should never happen if we aren't missing events, which we have concluded that we are not // and made decisions off of this belief. Maybe this shouldn't be here?

And finally the DeleteFinalStateUnknown definition: https://github.com/kubernetes/client-go/blob/e337077829155da74648d511bfa560a148fd965d/tools/cache/delta_fifo.go#L740-L743

// DeletedFinalStateUnknown is placed into a DeltaFIFO in the case where an object // was deleted but the watch deletion event was missed while disconnected from // apiserver. In this case we don't know the final "resting" state of the object, so // there's a chance the included `Obj` is stale.

What I'm getting from this is that the issue with handling these events is the given object might be stale. That shouldn't be an issue, as we retrieve the current state of the object when handling the event. It does not look like we will get another delete event after this one. I think its safer to not filter these out.
The docs seem to be misleading. Are you getting the same conclusion as me from the code?

@sebsoto I have removed the check. PTAL.

aravindhp · 2021-07-20T21:08:12Z

/retitle WINC-612: [wmco] Handle deletion of windows-instances

If the admin deletes the "windows-instances" ConfigMap, the operation is viewed as a call to deconfigure all BYOH Windows instances. This commits handles that scenario.

aravindhp · 2021-07-21T19:37:03Z

/hold

sebsoto · 2021-07-21T19:39:34Z

/approve

openshift-ci · 2021-07-21T19:39:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sebsoto

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~~ [sebsoto]

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

alinaryan · 2021-07-21T19:58:05Z

/lgtm

mansikulkarni96 · 2021-07-21T20:19:26Z

/lgtm

aravindhp · 2021-07-21T23:40:18Z

/retest

aravindhp · 2021-07-22T00:01:39Z

/hold cancel

openshift-bot · 2021-07-22T02:12:52Z

/retest-required