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 init improvements #48261

Merged
merged 14 commits into from Jun 26, 2018

Conversation

Projects
None yet
3 participants
@cbosdo
Copy link
Contributor

commented Jun 22, 2018

What does this PR do?

This PR makes virt.init more flexible for users. Rather than forcing all domain configuration in the minion configuration, virt.init now allows defining and/or overriding default disks and network interfaces. It also allows more graphic types than the VNC.

As an effort to understand virt.init before modifying it, the virt documentation has been improved a lot.

Note: this PR has been split in two: the first commits that are more independent have been submitted in PR #48262

What issues does this PR fix or reference?

Issue #48085 is also fixed in this PR.

Previous Behavior

See description.

New Behavior

See description

Tests written?

Yes

Commits signed with GPG?

Yes

@rallytime rallytime requested review from gtmanfred and rallytime Jun 22, 2018

@rallytime
Copy link
Contributor

left a comment

Since I merged your other PR, this will need a rebase first.

@cbosdo cbosdo force-pushed the cbosdo:virt-init branch from 87c37ff to d3256f9 Jun 25, 2018

@cbosdo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2018

@rallytime Thanks: rebase just done

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2018

@cbosdo There's a test failure here that needs to be addressed:

  • unit.modules.test_virt.VirtTestCase.test_parse_qemu_img_info

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu14-n/23911/

Can you take a look?

@cbosdo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2018

hum... locale problems, I'll make the test more robust

@cbosdo cbosdo force-pushed the cbosdo:virt-init branch from d3256f9 to 4cc56a3 Jun 25, 2018

cbosdo added some commits Jun 13, 2018

Refactoring virt.get_disks
virt.get_disks does not need to depend on libvirt:hypervisor value to
decice whether to extract data using qemu-img info on a disk image.
This needs to be run on all qemu images... and those can be also used
by a Xen VM nowadays.

The refactoring removes that dependency on the deprecated configuration
libvirt:hypervisor and uses the value for the <driver> type attribute
in the disk XML configuration.

Enhance the get_disk unit test to make sure that the qemu-img info is
parsed for a qemu image and not for others
Fix qemu-img infos output parsing
As mentioned in issue #48085 qemu-img infos parsing is really fragile.
In order to fix the weaknesses of that parsing use the --output json
parameter of qemu-img infos and parse this rather than the human output.

Note that the following properties in virt.get_disks output have a
different format:

  * disk size, virtual size and snapshot vmsize are now in bytes, rather
    than in a human-friendly format
  * date is now complete and in iso format, but that won't bother
    anybody since that was broken before (only had the time part)

Since the image property was a duplicate of the file one, they have been
consolidated into a single file property.

All format-specific values are simply not provided, but as those were
in the snapshots list before it's rather likely no one will care.

Also write the parsed data into a dictionary rather than writing it as
YAML in a string and parse it later on.

This commit also adds a unit test for _parse_qemu_img_info() function.
virt.get_disks provides complete backing chain
As mentioned in issue #48085, if a disk image has a backing file,
_parse_qemu_img_info() only returns the path to the backing file.
From a user point of view this is rather limited since there is no way
to know more on that file.

virt.get_disks now uses --backing-chain parameter to get the disk data
for all disks in the backing chain.
Finish removing the use of libvirt:hypervisor
Storing the hypervisor type in a configuration value doesn't make sense
since the libvirt host tells us what it is capable of. Instead use
the values from the guest domains provided by the virt.capabilities.

This is also the occasion to remove the use of the 'esxi' hypervisor in
as many places as possible since this is a synonym of 'vmware' and
'vmware' is the value provided by the libvirt esx driver.
Support more display types in virt.init
virt.init's enable_vnc is too limiting for the current possibilities of
the libvirt stack. Deprecate it in favor of a graphics dictionary to
allow creating VMs with VNC, Spice, RDP or no graphics. This design
helps keeping the structure opened to support new parameters in the
future.
Allow overriding NICs profile in virt.init
virt.init just allows the user to pass in a profile name to get the
list of network interfaces to create. This is rather good for simple
cases, but requires the profile to be stored in the minion
configuration.

From that commit on, the user can use an interfaces parameter to
customize the NICs from the template or add other ones. This could be
handy for the virt.running state for example.
Allow overriding disks profile in virt.init
virt.init just allows the user to pass in a profile name to get the
list of disk devices to create. This is rather good for simple
cases, but requires the profile to be stored in the minion
configuration.

From that commit on, the user can use a disks parameter to
customize the disks from the template or add other ones. This could be
handy for the virt.running state for example.

This new parameter makes the image parameter useless: deprecating it.
virt.init: move enable_qcow to disks parameter
enable_qcow is rather badly named since it doesn't tell the user what
that actually does. Thanks to the new disks parameter, this option can
now be set on a per-disk basis in the disks structure using a new
overlay_image property.

enable_qcow is now marked as deprecated
Fix virt images default location in docs
virt images are not stored in /srv/salt/salt-images, but in
/srv/salt-images. Fix the documentation to reflect the truth.
virt: move all disks computations in _disk_profile
Disk profile structure wasn't containing the image filename and path.
These were computed in two different places: one in _qemu_create_image()
and one in _gen_xml.

This commit moves all disks list computations in _disk_profile to get:
 - default values on both disk profile and user disks definitions
 - one place to compute all values
This should reduce error risks in future disk-related changes.
Don't create subfolders within virt:images
Disk images are currently created at a patch matching this rule:

    <virt:images>/<vmname>/<diskname>

In the future we want the user to be able to define in which libvirt
pool to create the image, rather than always in the default virt:images
folder. As libvirt doesn't allow volume names with a / in them, create
the qemu disk images in:

    <virt:images>/<vmname>_<diskname>
Simplify virt._disk_profile return value
Rather than returning a list of {name: dict_of_disk_props}, return
include the name as a property in each disk dictionary and remove one
level. This aligns disks with the structure returned for networks and
allows usage of list comrehensions to slightly simplify the calling code.
Add target path and type to virt.pool_info result
At times we need to know where the storage pool is pointing to and of
what type it is. virt.pool_info is a nice place for this.
Virt disk profile handles pool for kvm
So far _disk_profile uses the pool property only for vmware
hypervisors. Use it also for KVM/QEMU for users to be able to choose
where to create their images.

The pool property can be either the path to a local folder or an storage
pool name.

@cbosdo cbosdo force-pushed the cbosdo:virt-init branch from 4cc56a3 to d7c4f3d Jun 25, 2018

@cbosdo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2018

@rallytime Should be fixed by now the expected string is now computed based on the locale...

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2018

re-run py

@rallytime rallytime merged commit 6d5e6e7 into saltstack:develop Jun 26, 2018

7 of 12 checks passed

jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10983 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #20066 — ABORTED
Details
codeclimate 18 issues to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #6013 — FAILURE
Details
WIP ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #26217 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #18258 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23941 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22893 — SUCCESS
Details
jenkins/pr/lint The lint job has passed
Details

@cbosdo cbosdo deleted the cbosdo:virt-init branch Aug 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.