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 states improvements #48421

Merged
merged 8 commits into from Jul 9, 2018
Merged

Virt states improvements #48421

merged 8 commits into from Jul 9, 2018

Conversation

cbosdo
Copy link
Contributor

@cbosdo cbosdo commented Jul 3, 2018

What does this PR do?

This PR improves the virt states by using the new virt.init parameters in virt.running, rewriting virt.pool_define to handle more pool types, complete unit tests and documentation as well as some cleanup like some kwargs removal.

What issues does this PR fix or reference?

None

Tests written?

Yes

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

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.
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.
Since the virt module allows the user to override the connection
parameters, bubble these changes to the non-deprecated virt states.
Rename execution module virt.net_define in virt.network_define and
state virt.network_define into virt.network_running. Also fix the
network_running state to match the virt.running implementation style.
The virt.pool_define template is way too limited to handle all possible
libvirt pool types. Add some more parameters to cover them all.
Change the function name to pool_running since that state ensures the
pool is running, rather than just defining it. Also change the
implementation to match the one from virt.running() to keep consistent
across the module.
So far only the virt.running state is unit tested. Add tests for the
other states. Deprecated virt states have been purposedly left untested.
Avoid using kwargs to get the states parameters, perfer documented
named parameters with default value.
@cbosdo
Copy link
Contributor Author

cbosdo commented Jul 3, 2018

@rallytime @gtmanfred Could someone have a look at this PR?

'''
Create libvirt pool.

:param name: Pool name
:param ptype: Pool type
:param target: Pool path target
:param source: Pool dev source
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the idea of removing source.

At the very least, we should go through a deprecation process of this, and allow all of the other source_* things to still be defined in source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gtmanfred I know, but I removed that one without deprecating because it hasn't been released yet.

@homolkad could you check that these changes too since you are the one who introduced the pool and net define functions?

Copy link
Contributor

Choose a reason for hiding this comment

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

cool, if it hasn't been released yet, then I am ok with it if @homolkad is.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is ok. Nice job.

vport = kwargs.pop('vport', None)
tag = kwargs.pop('tag', None)
autostart = kwargs.pop('autostart', True)
start = kwargs.pop('start', True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does start need to be deprecated? I don't see this option used below and people might be passing this through still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rallytime This one and the other one you've spot are two states that haven't been published yet... that's why start hasn't been deprecated and removed directly. And after all, what's the point in telling we want the network / pool to be started in the *_running state?

target = kwargs.pop('target', None)
source = kwargs.pop('source', None)
autostart = kwargs.pop('autostart', True)
start = kwargs.pop('start', True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here.

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

4 participants