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

CA-369446: Do not ignore ovs-vsctl failure during VM-start #4828

Merged

Conversation

jameshensmancitrix
Copy link
Contributor

Change the use of doexec in add_vswitch_port to run using the ON_ERROR_FAIL handler. This will prevent the VM from starting in the case of ovs-vsctl failing.

Signed-off-by: jameshensmancitrix james.hensman@citrix.com

@lindig
Copy link
Contributor

lindig commented Oct 21, 2022

I think the commit subject could be shorter (to avoid overflow) and be more descriptive: "exit with error if add_vswitch_port fails" or similar. The body could explain how this is implemented (but it is also quite obvious) once you see the diff

Signed-off-by: jameshensmancitrix <james.hensman@citrix.com>
@robhoes
Copy link
Member

robhoes commented Oct 21, 2022

Did you manage to test this?

@jameshensmancitrix
Copy link
Contributor Author

@robhoes What would need to be done to have ovs-vsctl fail on vm start?

@robhoes
Copy link
Member

robhoes commented Oct 21, 2022

Firstly, you need to check that the normal case still works by starting a VM. To test the failure case, you could modify the code to call a fake version of ovs-vsctl that exits with an error.

@lindig lindig requested a review from robhoes October 24, 2022 08:27
Copy link
Member

@robhoes robhoes left a comment

Choose a reason for hiding this comment

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

Testing has now happened. All good.

@robhoes robhoes merged commit 91f0d15 into xapi-project:master Oct 24, 2022
@psafont psafont changed the title CA-369446 CA-369446: Do not ignore ovs-vsctl failure during VM-start Feb 13, 2023
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.

3 participants