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

virt.pool_running: fix pool start #52341

Merged
merged 9 commits into from Apr 12, 2019

Conversation

@cbosdo
Copy link
Contributor

commented Mar 27, 2019

What does this PR do?

Building a libvirt pool starts it. When defining a new pool, we need to
let build start it or we will get libvirt errors.

What issues does this PR fix or reference?

None

Previous Behavior

When creating a pool using the virt.pool_running state, the state failed with an error:

Requested operation is not valid: storage pool 'default' is already active

New Behavior

When creating a pool using the virt.pool_running state, the state succeeds.

Tests written?

Yes

Commits signed with GPG?

Yes

@Akm0d Akm0d self-requested a review Mar 27, 2019
@dwoz

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

@cbosdo Can you write a test for this change?

@cbosdo cbosdo force-pushed the cbosdo:virt-state-fixes branch from b2ad850 to 4da5834 Apr 8, 2019
@cbosdo cbosdo requested a review from saltstack/team-core as a code owner Apr 8, 2019
@cbosdo cbosdo force-pushed the cbosdo:virt-state-fixes branch from 4da5834 to 25ba1f9 Apr 8, 2019
cbosdo added 7 commits Jun 6, 2018
As mentioned in issue 47972, applying the virt.running state doesn't
report any error if the libvirt create call actually failed.

This commit introduces proper handling of the libvirt errors to let
users see the libvirt error in case of failure.

Also add test cases for virt.running to prevent regression.

(cherry picked from commit 451e7da)
Test virt.running when the domain needs to be edited and started. This
will prevent regressions when improving virt.running to handle the new
virt.init parameters.

(cherry picked from commit 495db34)
virt.running actually calls virt.init if the virtual machine doesn't
exist. Let the user define all the virt.init possible options in his
running states too.

(cherry picked from commit cb00a5f)
So far only the virt.running state is unit tested. Add tests for the
other states. Deprecated virt states have been purposedly left untested.

(cherry picked from commit fc75872)
Avoid using kwargs to get the states parameters, perfer documented
named parameters with default value.

(cherry picked from commit c7c5d6e)
So far virt.running does nothing if the corresponding domain is already
defined. Use the new virt.update function to change the domain
configuration.

(cherry picked from commit 2a5f6ae)
Some hypervisors can handle several CPU architectures or have different
virtualization types. This is reflected in libvirt by the OS type (badly
named, indeed) and the arch value. Allow users to set them when creating
a VM using either virt.init or virt.running.

Signed-off-by: Cédric Bosdonnat <cbosdonnat@suse.com>
(cherry picked from commit 2463ebe)
@cbosdo cbosdo force-pushed the cbosdo:virt-state-fixes branch from 25ba1f9 to dd1caa4 Apr 8, 2019
@cbosdo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@dwoz For some reason the state tests that I wrote before were not in the 2019.2 branch. I cherry-picked them and adapted virt.test_pool_running() to match the fix.

Building a libvirt pool starts it. When defining a new pool, we need to
let build start it or we will get libvirt errors.
@cbosdo cbosdo force-pushed the cbosdo:virt-state-fixes branch from dd1caa4 to 25b9681 Apr 8, 2019
@dwoz dwoz added the 2019.2.1 label Apr 11, 2019
@dwoz
dwoz approved these changes Apr 12, 2019
Copy link
Contributor

left a comment

@cbosdo In the future, please also submit a bug report issue for things that you run into like this. It'll help us track the work and spot potentially duplicate issues. Thanks for the work here!

@dwoz dwoz merged commit 7a1b8ca into saltstack:2019.2 Apr 12, 2019
10 checks passed
10 checks passed
WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint Python lint test has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details
@cbosdo cbosdo referenced this pull request Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.