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: allow defining the VM type and arch when creating it #49510

Merged
merged 2 commits into from
Sep 6, 2018

Conversation

cbosdo
Copy link
Contributor

@cbosdo cbosdo commented Sep 5, 2018

What does this PR do?

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.

What issues does this PR fix or reference?

None

Previous Behavior

virt.init and virt.running where forcing x86_64 HVM virtual machines when creating them.

New Behavior

User can choose the architecture and VM type to be created

Tests written?

Yes

Commits signed with GPG?

Yes

@cbosdo
Copy link
Contributor Author

cbosdo commented Sep 5, 2018

@cachedout @rallytime this one PR should be fairly straight forward

Copy link
Contributor

@rallytime rallytime left a comment

Choose a reason for hiding this comment

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

Thanks @cbosdo! I have some small requests.

type of virtualization as found in the ``//os/type`` element of the libvirt definition.
The default value is taken from the host capabilities, with a preference for ``hvm``.

.. versionadded:: Fluorine
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be Neon now.

architecture of the virtual machine. The default value is taken from the host capabilities,
but ``x86_64`` is prefed over ``i686``.

.. versionadded:: Fluorine
Copy link
Contributor

Choose a reason for hiding this comment

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

Neon here as well. :)

@@ -1200,6 +1197,8 @@ def _get_merged_nics(hypervisor, profile, interfaces=None, dmac=None):
def init(name,
cpu,
mem,
os_type=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

These new parameters need to go at the end of the list so we don't break things for people if they are using positional arguments.

salt/states/virt.py Outdated Show resolved Hide resolved
@cbosdo cbosdo force-pushed the virt-arch-ostype branch 2 times, most recently from 5488afa to fd6404d Compare September 5, 2018 14:57
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>
Not all disk target devices should start with 'vd' for non-vmware
hypervisors. Map the bus type to the right device name prefix. While at
it, also make sure the disks are created right for xen.
@cbosdo
Copy link
Contributor Author

cbosdo commented Sep 5, 2018

The second commit being a bug fix, may be backported to Fluorine.

@meaksh
Copy link
Contributor

meaksh commented Sep 5, 2018

Hey @rallytime , the changes on this PR are part of @cbosdo's work around the virt module which has been already included in fluorine.

Some of the related PRs are #48736, #48421, #48261, #48262, etc. which are all included on fluorine.

Since this PR is still related with the previous ones and also contains a bugfix, it would be really nice to get this PR also into fluorine.

Would that be even possible at this point in time? Thanks!

@cachedout
Copy link
Contributor

@meaksh and @cbosdo Do you anticipate the need for additional backports beyond this or would this be the only one?

@cbosdo
Copy link
Contributor Author

cbosdo commented Sep 6, 2018

@cachedout I don't expect any other backports: the other ones can wait for a future release

@rallytime
Copy link
Contributor

@cbosdo That sounds good. In the future let's keep the feature additions and bug fixes in separate PRs so it's easier to get the bug fixes in the right place. I am OK with backporting this one, but just in the future to make it simpler. :D

@cbosdo
Copy link
Contributor Author

cbosdo commented Sep 6, 2018

@rallytime noted

@rallytime rallytime added the bugfix-bckport will be be back-ported to an older release branch by creating a PR against that branch label Sep 6, 2018
@rallytime
Copy link
Contributor

Thanks! I'll get this backported right now.

@rallytime rallytime merged commit 5e7bbb4 into saltstack:develop Sep 6, 2018
@meaksh
Copy link
Contributor

meaksh commented Sep 6, 2018

Thanks @rallytime ! 👍

@rallytime
Copy link
Contributor

@cbosdo Actually, this isn't backporting cleanly and is including more changed files than it should because of some additional work it's pulling in from a kwarg addition around the loader. Can you make this change against the fluorine branch directly instead of me back-porting it?

@rallytime rallytime removed the bugfix-bckport will be be back-ported to an older release branch by creating a PR against that branch label Sep 6, 2018
@cbosdo
Copy link
Contributor Author

cbosdo commented Sep 6, 2018

@rallytime sure

@cbosdo
Copy link
Contributor Author

cbosdo commented Sep 6, 2018

@rallytime Should I change the versionadded back to Fluorine, then?

@rallytime
Copy link
Contributor

@cbosdo Yeah! Thanks for catching that.

@cbosdo
Copy link
Contributor Author

cbosdo commented Sep 6, 2018

@rallytime I changed them in the backport PR, how should I handle that for develop? open a new PR?

@rallytime
Copy link
Contributor

@cbosdo No need to do anything for develop. I will merge forward and there will be a conflict on those lines. I'll take the Fluorine version. Thank you for following up!

rallytime pushed a commit that referenced this pull request Sep 7, 2018
@cbosdo cbosdo deleted the virt-arch-ostype branch October 15, 2018 06:50
@waynew waynew removed the master-port label Dec 2, 2019
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.

6 participants