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 nets pools #47907

Merged
merged 14 commits into from Jun 13, 2018
Merged

Virt nets pools #47907

merged 14 commits into from Jun 13, 2018

Conversation

@cbosdo
Copy link
Collaborator

@cbosdo cbosdo commented May 31, 2018

What does this PR do?

Add functions to the virt execution module to manage libvirt networks and storage pools.
As a follow up, a future PR will add functions to manage storage volumes.

What issues does this PR fix or reference?

None

Tests written?

Yes, but no unit tests were written for the state changing functions since these wouldn't be super useful.

Note: the virt module unit tests are no pylint-clean!

Commits signed with GPG?

Yes

@cbosdo cbosdo force-pushed the cbosdo:virt-nets-pools branch from 2a631ab to 4953c2e Jun 6, 2018
}
finally:
conn.close()
return result # pylint: disable=lost-exception

This comment has been minimized.

@isbm

isbm Jun 11, 2018
Contributor

Shouldn't this to be indented one block earier like so?

    finally:
        conn.close()

return result
ret = net.setAutostart(1) == 0

elif state == 'off':
ret = net.setAutostart(0) == 0

This comment has been minimized.

@isbm

isbm Jun 11, 2018
Contributor

ret = net.setAutostart(state == 'on' and 1 or 0) == 0
}
finally:
conn.close()
return result # pylint: disable=lost-exception

This comment has been minimized.

@isbm

isbm Jun 11, 2018
Contributor

Return statement indent one block earlier, perhaps?

'''
conn = __get_conn(**kwargs)
pool = conn.storagePoolLookupByName(name)
ret = pool.create() == 0

This comment has been minimized.

@isbm

isbm Jun 11, 2018
Contributor

You use this foo == 0 all over the place. Unless 0 means something (like a constant, say error code or so) then it has to be aligned to it. Otherwise I would use just ret = bool(pool.create()).

ret = pool.setAutostart(1) == 0

elif state == 'off':
ret = pool.setAutostart(0) == 0

This comment has been minimized.

@isbm

isbm Jun 11, 2018
Contributor

Same here, this can be just one line:

ret = bool(not pool.setAutostart(int(bool(state == 'on)))

Or the example above in other comments to a similar place. 😉


class libvirtError(Exception):
'''
libvirtError mock
'''
pass

This comment has been minimized.

@isbm

isbm Jun 11, 2018
Contributor

The pass is not needed when docstring is present.

@cbosdo cbosdo force-pushed the cbosdo:virt-nets-pools branch from 4953c2e to fbf028e Jun 12, 2018
@cbosdo
Copy link
Collaborator Author

@cbosdo cbosdo commented Jun 12, 2018

@isbm All requested changes have been pushed

@isbm
isbm approved these changes Jun 12, 2018
net = conn.networkLookupByName(name)
ret = not bool(net.create())
conn.close()
return ret

This comment has been minimized.

@isbm

isbm Jun 12, 2018
Contributor

JFYI, in Python you can do something like this:

def network_start(name, **kwargs):
    '''
    your docstring
    '''
    conn = __get_conn(**kwargs)
    try:
        return not bool(conn.networkLookupByName(name).create())
    finally:
        conn.close()

Which is still final return, just conn.close() guaranteed happening after. So that minimises the code as well. You can reuse this all around the place, if you like.

This comment has been minimized.

@cbosdo

cbosdo Jun 12, 2018
Author Collaborator

Changed it since it also protects against unexpected errors.

@isbm
Copy link
Contributor

@isbm isbm commented Jun 12, 2018

@rallytime we're good to go here in terms of reviews from our side. 😉 Please add reviewers or proceed as normal.

@cbosdo cbosdo force-pushed the cbosdo:virt-nets-pools branch 2 times, most recently from 3cd5c3b to 1418498 Jun 12, 2018
@rallytime rallytime requested a review from gtmanfred Jun 12, 2018
Copy link
Contributor

@gtmanfred gtmanfred left a comment

These need to have the name and any other argument documented for each function.

Copy link
Contributor

@isbm isbm left a comment

@cbosdo translated, please go over the each function documentation and make sure each explicitly named parameter as well as whatever is supposed to be found in kwargs is properly documented in the docstring of the corresponding function. This will appear as help string in command line interface too.

cbosdo added 14 commits May 31, 2018
Users want to know what are the virtual networks available on their
hypervisor. Provide a list_networks function for that purpose.
User need to get basic informations on how a virtual network looks like.
For example if it autostarts, its current state, the bridge associated
with it, etc. Provide these using a new virt.network_info function.
From this commit on, users can start and stop their virtual networks
using virt.network_start and virt.network_stop functions.
Let user undefine virtual networks.
Let users change the autostart flag of virtual networks.
Users need to figure out what libvirt storage pools are defined. Provide
a new virt.list_pools function for that.
Users need to get some data from the libvirt storage pools, like its
state, allocation / free sizes, etc. Provide them using the new
virt.pool_info function.
Let user start, stop and build libvirt storage pools by adding the
virt.pool_start, virt.pool_stop and virt.pool_build functions.
Let users undefine libvirt storage pools.
Let users delete libvirt storage pool resources.
Let users refresh the libvirt storage pools status.
Let user change the autostart flag of libvirt storage pools.
Let users list the volumes contained in a libvirt storage pool.
@cbosdo cbosdo force-pushed the cbosdo:virt-nets-pools branch from 1418498 to 1293f7d Jun 12, 2018
@cbosdo
Copy link
Collaborator Author

@cbosdo cbosdo commented Jun 12, 2018

all parameters documented now

@rallytime
Copy link
Contributor

@rallytime rallytime commented Jun 12, 2018

re-run py3

@rallytime rallytime merged commit 41bf828 into saltstack:develop Jun 13, 2018
5 of 10 checks passed
5 of 10 checks passed
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10653 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19731 — ABORTED
Details
codeclimate 13 issues to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5682 — FAILURE
Details
@wip
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25881 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17939 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23607 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22570 — SUCCESS
Details
@cbosdo cbosdo deleted the cbosdo:virt-nets-pools branch Aug 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants