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

runtime-rs: ch: Change state when VM stopped #8630

Merged

Conversation

jodh-intel
Copy link
Contributor

Make the CH (Cloud Hypervisor) stop_vm() method check the VM state before attempting to stop the VM, and update the state once the VM has stopped.

This avoids the method failing if called multiple times which will happen if the workload exits before the container manager requests that the container stop.

This change ensures the CH driver finishes cleanly.

Fixes: #8629.

Copy link
Contributor

@dborquez dborquez left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jodh-intel

@amshinde amshinde self-requested a review December 11, 2023 22:15
// which results in this method being called potentially a second
// time. Without this check, we'll return an error representing EPIPE
// since the CH API socket is at that point invalid.
if self.state != VmmState::VmRunning {
Copy link
Member

Choose a reason for hiding this comment

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

I was going to suggest to explicitly check for Stopped State here, but looks like there are only 3 states defined. I would eventually like to explicitly distinguish between NotReady and Stopped, but this would work for now.

@amshinde
Copy link
Member

/test

@jodh-intel
Copy link
Contributor Author

/retest-arm

Copy link
Contributor

@cmaf cmaf left a comment

Choose a reason for hiding this comment

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

@jodh-intel is there a reason we don't update the state within cloud_hypervisor_shutdown?

Also I wanted to check I'm understanding this correctly, if cloud_hypervisor_shutdown returns an error the state will remain VmRunning?

@jodh-intel
Copy link
Contributor Author

@jongwu - Is there a known problem with the ARM jenkins CI as I'm seeing lots of errors like this:

host system doesn't support vsock: stat /dev/vhost-vsock: no such file or directory

Move the state setting to the `Hypervisor` trait calls. This makes the
code clearer.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@jodh-intel jodh-intel force-pushed the runtime-rs-ch-set-state-on-vm-stop branch from 3dbcf9a to 24d1eae Compare December 12, 2023 15:34
@jodh-intel
Copy link
Contributor Author

is there a reason we don't update the state within cloud_hypervisor_shutdown

@cmaf - I've moved the state handling to the Hypervisor API calls now for clarity and consistency (see new commit).

Also I wanted to check I'm understanding this correctly, if cloud_hypervisor_shutdown returns an error the state will remain VmRunning?

@cmaf - Good catch. I've updated the branch to set the state to NotReady before that call as, conveniently, that's the state we want on success and failure ;)

@jodh-intel
Copy link
Contributor Author

/test

Make the CH (Cloud Hypervisor) `stop_vm()` method check the VM state before
attempting to stop the VM, and update the state once the VM has stopped.

This avoids the method failing if called multiple times which will
happen if the workload exits before the container manager requests that
the container stop.

This change ensures the CH driver finishes cleanly.

Fixes: kata-containers#8629.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@jodh-intel jodh-intel force-pushed the runtime-rs-ch-set-state-on-vm-stop branch from 24d1eae to 2a518f0 Compare December 12, 2023 18:25
@jodh-intel
Copy link
Contributor Author

/test

@jodh-intel jodh-intel merged commit d7c6219 into kata-containers:main Dec 13, 2023
171 of 181 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test size/small Small and simple task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

runtime-rs: ch: Change state when VM stopped
6 participants