Make sure old VMs are deleted if UIDs don't match #218

rmohr · 2017-05-17T15:04:52Z

WIP because: Not yet sure how to write a functional test which covers that.

If virt-handler is under load, it can miss a delete and instead see a
replaced VM with the same name. Check for the UIDs to detect such
mismatches and delete the old VM before starting the new one.

Fixes #193

stu-gott · 2017-05-26T20:54:20Z

EDIT: this has been addressed.

@rmohr There appears to be a problem with the code in manager.go. It appears that due to the changes that were incorporated for getActualOrDeleteOutdatedDomain, that dom.GetState() crashes with a nil pointer reference on line 380.

I'm posting the test in case you get a chance to look at it before I do.

davidvossel · 2017-06-12T17:45:20Z

pkg/virt-handler/virtwrap/manager.go

+		if string(vm.GetObjectMeta().GetUID()) != uid {
+			vm := v1.NewVMReferenceFromName(vm.ObjectMeta.Name)
+			vm.GetObjectMeta().SetUID(types.UID(uid))
+			if err = l.killDomain(vm, dom); err != nil {


If nothing is being logged here, this if statement isn't really necessary.

Maybe such a defer:

defer func (dom) { if dom != nil { dom.Free() } }(dom)

would be a solution to make the if unnecessary.

I think there was an issue here where dom was technically an interface, so comparing against nil didn't do something expected.

davidvossel · 2017-06-12T17:49:18Z

pkg/virt-handler/virtwrap/manager.go

+		logging.DefaultLogger().Object(vm).Error().Reason(err).Msg("Getting the domain failed.")
+		return nil, false, err
+	}
+	if exists {


you could reduce indentation by returning early here if you wanted.

if !exists { return nil, false, nil }

rmohr · 2017-06-13T10:59:20Z

tests/virt_handler_test.go

+
+			uuid := obj.(*v1.VM).ObjectMeta.UID
+
+			deleteVirtHandler(coreClient, namespace)


Expect(deleteVirtHandler(...)).To(Succeed())

rmohr · 2017-06-13T11:01:54Z

tests/virt_handler_test.go

+
+			// Ensure the event log shows that a VM restart took place
+			select {
+			case <-time.After(120 * time.Second):


I would rather try to give the whole test an overall timeout ...

rmohr · 2017-06-16T10:54:26Z

tests/virt_handler_test.go

+	}, 60*time.Second).Should(Equal(false), "Timed out waiting for virt-handler to stop")
+
+	// Double check that virt-handler daemonset is not present
+	obj, err := betaRestClient.Get().Resource("daemonsets").Namespace(namespace).Name("virt-handler").Do().Get()


Can't you move that check into the isVirtHandlerRunning function?

rmohr · 2017-06-16T11:00:26Z

pkg/virt-handler/virtwrap/manager.go

+		if string(vm.GetObjectMeta().GetUID()) != uid {
+			vm := v1.NewVMReferenceFromName(vm.ObjectMeta.Name)
+			vm.GetObjectMeta().SetUID(types.UID(uid))
+			if err = l.killDomain(vm, dom); err != nil {


Maybe such a defer:

defer func (dom) { if dom != nil { dom.Free() } }(dom)

would be a solution to make the if unnecessary.

If virt-handler is under load, it can miss a delete and instead see a replaced VM with the same name. Check for the UIDs to detect such mismatches and delete the old VM before starting the new one.

Signed-off-by: Stu Gott <sgott@redhat.com>

davidvossel · 2017-06-28T14:04:22Z

retest this please

davidvossel · 2017-06-28T14:07:15Z

ci is failing

davidvossel

Trying to detect this situation is weird. We shouldn't have to care about this.

We should put something that uniquely defines the VM domain in the actual domain name passed to libvirt to prevent us from having to think about this condition. I don't know what the vm "GetUID()" function's results look like, but if that's something we can include in the domain name, that seems valuable.

davidvossel · 2017-06-28T14:14:40Z

pkg/virt-handler/virtwrap/manager.go

+	if string(vm.GetObjectMeta().GetUID()) != uid {
+		vm := v1.NewVMReferenceFromName(vm.ObjectMeta.Name)
+		vm.GetObjectMeta().SetUID(types.UID(uid))
+		if err = l.killDomain(vm, dom); err != nil {


We're about to have the ability for vms with identical names to live in different namespaces. This function just looks up domains by name. We'll want to make sure to update this when the namespace change occurs.

I think it is better to just completely ignore the k8s object name when setting the guest name. This avoids problems with clashes in set of valid characters for names between libvirt & kubevirt, or length limits, or other similar things which could trip us up. Use some kubevirt controlled naming convention, so we can reliably identify kubevirt controlled guests. A simple option is just 'kubevirt-" prefix + the result of GetUID() again. Ideally, we would use UID for all object lookups too, since UUID is the primary unique identifier libvirt recommends apps work with, as it has the strongest uniqueness guarantees.

Sounds good.

We didn't do it so far, because by default the controllers use namespaces and object names to create keys for lookups. So if we don't do it this way, we could miss the deletion of a VM and only see the updated object.

I wanted to avoid copying client-go code to use another identify function, but by going over it again, it might not be that much copying. I will replace the DeletionHandlingMetaNamespaceKeyFunc in the Shared informer for virt-handler in the two controllers there with a DeletionHandlingUUIDKeyFunc if possible. Then we can track deletions based on uuids.

Hm, I would have to do a complete copy of the cache.sharedIndexerInformer. I can't replace the DeletionHandlingMetaNamespaceKeyFunc otherwise ...

I will see if we can do something upstream in client-go, to not have to copy all that ...

kubernetes/client-go#229

@berrange, like mentioned further down below in the PR, I don't think we have a much better option than what we do here right now. Regarding valid character clashes, I looked up the allowed chars in Kubernetes:

By convention, the names of Kubernetes resources should be up to maximum length of 253 characters and consist of lower case alphanumeric characters, -, and ., but certain resources have more specific restrictions. [1]

I think there should be no issues, since this is a subset of what libvirt supports. What do you think?

fabiand · 2017-07-10T09:34:18Z

Anything that is missing to move this PR forward?

davidvossel · 2017-07-10T14:32:48Z

@fabiand the patch as it is now means we're inheriting some technical debt. The "correct" fix is for us to be able to make use of informers in a way where we don't miss deletions. As long as we've investigated that there's not a low friction way for us to resolve the issue using informer logic right now, my opinion is that we should accept the debt and retroactively detect the UUID mismatch like the proposed patch is doing.

rmohr · 2017-07-11T09:28:55Z

Follow up on kubernetes/client-go#229: It turns out, that it would not really help us, to set the KeyFunc.
While we would be able to "optimize" the extra uid checks away, and not miss the delete, we would still have to take care of the situation, where we would be informed about the new VM with the new UID while the delete might still not be processed.
We could handle that, but I have the suspicion, that it makes things even worse.

davidvossel · 2017-10-26T20:30:56Z

This issue cannot occur anymore. Maybe it was possible in the past, but there are safeguards in place now that prevent it.

If an active VM is deleted and then immediately created again, the newly created version of the VM will move to the "Pending" phase.
If a previous instance of this VM is still active somewhere in the cluster, then that instances's virt-launcher pod will still be active while the new VM is in "Pending".
Virt-controller will not attempt to schedule the new VM while an active virt-launcher pod already exists for the VM. Look for this log message logger.Error("VM Pods do already exist, will clean up before continuing.") All existing virt-launcher pods must terminate before the VM can move from "pending" to "scheduling"

By virt-controller blocking the scheduling of the VM until the previous virt-launcher pod termintates, it's not possible for virt-handler to know about the newly created VM's existence until after cleanup of the previous VM instance occurs.

I'm closing this for now. Feel free to reopen it if someone wants to discuss it further.

[WIP] add unit test for pkg/controller util_test.go and import_controller_test.go

Add ROADMAP document

fix the typo of macvlan and also modify documents to meet the current plugins.

Signed-off-by: Quique Llorente <ellorent@redhat.com>

rmohr requested review from admiyo and stu-gott May 17, 2017 15:04

rmohr force-pushed the fix-delete branch from 368c746 to 4fb6847 Compare May 17, 2017 15:05

stu-gott force-pushed the fix-delete branch 3 times, most recently from a305fc9 to 40e86e1 Compare June 7, 2017 21:38

admiyo changed the title ~~[DO NOT MERGE] Make sure old VMs are deleted if UIDs don't match~~ Make sure old VMs are deleted if UIDs don't match Jun 12, 2017

davidvossel reviewed Jun 12, 2017

View reviewed changes

rmohr commented Jun 16, 2017

View reviewed changes

rmohr and others added 4 commits June 27, 2017 12:58

Make sure old VMs are deleted if UIDs don't match

5840ff5

If virt-handler is under load, it can miss a delete and instead see a replaced VM with the same name. Check for the UIDs to detect such mismatches and delete the old VM before starting the new one.

Functional test verifying that VMs are re-created when needed

47b219c

Signed-off-by: Stu Gott <sgott@redhat.com>

Refactor manager.go for readability

2c2b095

Signed-off-by: Stu Gott <sgott@redhat.com>

Add missing error check to WaitForSuccessfulVMStart

2cfa908

Signed-off-by: Stu Gott <sgott@redhat.com>

stu-gott force-pushed the fix-delete branch from 40e86e1 to 2cfa908 Compare June 27, 2017 18:30

davidvossel requested changes Jun 28, 2017

View reviewed changes

davidvossel closed this Oct 26, 2017

kubevirt-bot pushed a commit to kubevirt-bot/kubevirt that referenced this pull request Nov 6, 2020

Merge pull request kubevirt#218 from screeley44/unit-test-controller

7c9faad

[WIP] add unit test for pkg/controller util_test.go and import_controller_test.go

mzzgaopeng pushed a commit to mzzgaopeng/kubevirt that referenced this pull request Mar 8, 2021

Merge pull request kubevirt#218 from steveeJ/roadmap

4a292cd

Add ROADMAP document

mzzgaopeng pushed a commit to mzzgaopeng/kubevirt that referenced this pull request Mar 8, 2021

Merge pull request kubevirt#218 from hwchiu/fix-readme

5fc8209

fix the typo of macvlan and also modify documents to meet the current plugins.

kubevirt-bot pushed a commit to kubevirt-bot/kubevirt that referenced this pull request Dec 7, 2021

Move okd hashes to images.sh (kubevirt#218)

e5eaa1a

Signed-off-by: Quique Llorente <ellorent@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure old VMs are deleted if UIDs don't match #218

Make sure old VMs are deleted if UIDs don't match #218

rmohr commented May 17, 2017

stu-gott commented May 26, 2017 •

edited

Loading

davidvossel Jun 12, 2017

rmohr Jun 16, 2017

stu-gott Jun 27, 2017

davidvossel Jun 12, 2017

rmohr Jun 13, 2017

rmohr Jun 13, 2017

rmohr Jun 16, 2017

rmohr Jun 16, 2017

davidvossel commented Jun 28, 2017

davidvossel commented Jun 28, 2017

davidvossel left a comment

davidvossel Jun 28, 2017

berrange Jun 28, 2017

rmohr Jun 29, 2017

rmohr Jun 29, 2017

rmohr Jun 29, 2017 •

edited

Loading

rmohr Jun 29, 2017

rmohr Jul 11, 2017

fabiand commented Jul 10, 2017

davidvossel commented Jul 10, 2017

rmohr commented Jul 11, 2017

davidvossel commented Oct 26, 2017


		uuid := obj.(*v1.VM).ObjectMeta.UID

		deleteVirtHandler(coreClient, namespace)

Make sure old VMs are deleted if UIDs don't match #218

Make sure old VMs are deleted if UIDs don't match #218

Conversation

rmohr commented May 17, 2017

stu-gott commented May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidvossel commented Jun 28, 2017

davidvossel commented Jun 28, 2017

davidvossel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmohr Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiand commented Jul 10, 2017

davidvossel commented Jul 10, 2017

rmohr commented Jul 11, 2017

davidvossel commented Oct 26, 2017

stu-gott commented May 26, 2017 •

edited

Loading

rmohr Jun 29, 2017 •

edited

Loading