Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update VM phase by checking domain status and reason #149

Merged
merged 3 commits into from
Mar 28, 2017

Conversation

mpolednik
Copy link
Contributor

Second attempt since the event changes. The only way to really see the phase change is to actually crash the domain - there is no phase change when VM TPR is deleted (so is the domain, but at that point there is nothing to update).

@rmohr Any idea how to nicely test this? Currently experimenting with pkill -9 qemu, which may be a bit strong. Pretty much need anything to send kill -9 or equivalent.

Closes #75

@kubevirt-bot
Copy link
Contributor

Can one of the admins verify this patch?

@rmohr
Copy link
Member

rmohr commented Mar 16, 2017

I could think of a privileged pod which runs on the host pid namespace and just greps for the qemu processes for the vm (something like ps aux | grep qemu | grep and sends kill -9 to that process.

We can then just launch that pod on the node of the VM. That would also be useful for later, for random kill, to verify everything is working ...

You can have a look at the libvirt daemon set to see how to run a privileged pod with host pidnamespace,

@mpolednik
Copy link
Contributor Author

@rmohr Cool idea, like the future extensibility. Looking into it.

@rmohr
Copy link
Member

rmohr commented Mar 27, 2017

@mpolednik looks good so far. Could you rebase it? @admiyo changed the method calls on the controllers a little bit. Should not be hard to update.

@mpolednik
Copy link
Contributor Author

@rmohr noticed when working on it today :(

I'd love to add the "crash" test in the same PR, update coming today at night or someday tomorrow.

Copy link
Member

@rmohr rmohr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

One check is missing. Once that is posted I will merge it asap!

}
obj, vmExists, err := d.vmStore.GetByKey(key.(string))
if err != nil {
queue.AddRateLimited(key)
} else if !vmExists || obj.(*v1.VM).GetObjectMeta().GetUID() != domain.GetObjectMeta().GetUID() {
// The VM is not in the vm cache, or is a VM with a differend uuid, tell the VM controller to investigate it
d.vmQueue.Add(key)
} else {
setVmPhaseForStatusReason(domain, obj.(*v1.VM), d.restClient)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also have to check if the VM has the same uid like the Domain, otherwise you are setting a potentially new posted VM to stopped, although it was not even scheduled ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and one more thing, you have to check for the error. If an error is returned, you have to do a queue.AddRateLimited(key).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the first comment: shouldn't that be handled by else-if clause above?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes right.

@rmohr
Copy link
Member

rmohr commented Mar 28, 2017

ok to test

@rmohr rmohr merged commit 301b847 into kubevirt:master Mar 28, 2017
kubevirt-bot pushed a commit to kubevirt-bot/kubevirt that referenced this pull request Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants