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: more network support #59146

Merged
merged 30 commits into from
Jan 13, 2021
Merged

virt: more network support #59146

merged 30 commits into from
Jan 13, 2021

Conversation

cbosdo
Copy link
Contributor

@cbosdo cbosdo commented Dec 15, 2020

What does this PR do?

Add more parameters for virtual network creation in virt.network_defined and virt.network_running states.

What issues does this PR fix or reference?

Fixes: #59143

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

@cbosdo cbosdo requested a review from a team as a code owner December 15, 2020 12:38
@cbosdo cbosdo requested review from waynew and removed request for a team December 15, 2020 12:38
@cbosdo
Copy link
Contributor Author

cbosdo commented Dec 15, 2020

This PR is based on #59144

@cbosdo cbosdo force-pushed the virt-network branch 4 times, most recently from 132d017 to 534ae88 Compare December 16, 2020 07:30
@cbosdo
Copy link
Contributor Author

cbosdo commented Dec 16, 2020

re-run pr-macosxcatalina-py3-pytest

@waynew
Copy link
Contributor

waynew commented Dec 17, 2020

Well, this is quite the monster PR 🙃

I'm OOO through the end of the year - I don't know how antsy you are to get this in, but if nobody reviews before I get back I'll plan to take a look at this Jan 4

@cbosdo
Copy link
Contributor Author

cbosdo commented Dec 18, 2020

I'ld like to have it in Aluminium ideally, so can still wait for you to come back I guess

@cbosdo cbosdo force-pushed the virt-network branch 3 times, most recently from 8164603 to b226268 Compare December 22, 2020 16:30
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

I'm about 25% through my review on this, but I'm off for the day -- I'll pick this up tomorrow AM Central Time, most likely.

Comment on lines -2625 to -2627
iothreads
When ``True`` dedicated threads will be used for the I/O of the disk.
(Default: ``False``)
Copy link
Contributor

Choose a reason for hiding this comment

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

@cbosdo question: do we need to deprecate this old behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, there is no need since it hasn't been released yet. And this change is already part of ACKed PR #59144

salt/utils/xmlutil.py Outdated Show resolved Hide resolved
salt/modules/virt.py Outdated Show resolved Hide resolved
salt/modules/virt.py Outdated Show resolved Hide resolved
salt/modules/virt.py Outdated Show resolved Hide resolved
salt/modules/virt.py Outdated Show resolved Hide resolved
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

praise: I have to say overall, great job! This was clearly a lot of coding and testing 🙌

Most of the feedback I have are just nitpicks - I didn't see any logic failures, and only one spot that had me :suspect:

Once you've addressed the feedback (either 👍 the suggestions or 🗑️), I'm 👍 on this. Thank you so much, and again, great job!

salt/modules/virt.py Outdated Show resolved Hide resolved
tests/pytests/unit/states/virt/test_domain.py Outdated Show resolved Hide resolved
salt/modules/virt.py Outdated Show resolved Hide resolved
salt/modules/virt.py Outdated Show resolved Hide resolved
salt/modules/virt.py Outdated Show resolved Hide resolved
salt/modules/virt.py Outdated Show resolved Hide resolved
salt/modules/virt.py Outdated Show resolved Hide resolved
salt/modules/virt.py Outdated Show resolved Hide resolved
salt/modules/virt.py Outdated Show resolved Hide resolved
Comment on lines 4234 to 4236
if "errors" not in status:
status["errors"] = []
status["errors"] += errors
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: setdefault would work here, too:

if errors:
    status.setdefault("errors",[]).extend(errors)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed all the comments. Thanks for the thorough review!

@cbosdo cbosdo force-pushed the virt-network branch 2 times, most recently from e487008 to b846e6b Compare January 7, 2021 14:12
cbosdo added 11 commits January 8, 2021 13:34
io_uring has recently been added as another IO policy on virtual disks.
Keep the parameter opened for changes. Also add the optional iothread ID
in case the user wants to pin a disk to some IO thread (and thus to a
CPU).
Stripping spaces and indentation from XML could also be useful in other
places, moving to xmlutil to help reuse.
The network_update code will be rather similar to the pool_update one.
In order to share the XML tree cleanup for easier comparisons, create a
helper function in the virt module.
In order to let users define more types of virtual networks, expose more
of the libvirt virtual network properties.
cbosdo added 9 commits January 8, 2021 13:34
While converting the virt domain-related states to pytest I realized
the __opts__["test"] == False case was not handled in some of them.
This commit also fixes the return code for virt.shutdown,
virt.powered_off, virt.snapshot and virt.rebooted states. It also
prevents the actual call to be issued in test mode.
Expose the new host_devices parameter to the virt.running and
virt.defined states.
On a running guest, libvirt changes the XML definition of the network
interfaces of type "network" to the type of the network (for instance
bridge). In such a case the virt.update() function will find the two
NICs different even if they may not be... so we need to try harder to
compare.
A network with hostdev forward mode has no bridge and no mac. So we need
to handle this in a few places in the virt module.
In order to help reusing the device changes computing code and avoid
getting a giant virt.update(), move the live update code of it into a
specific internal function.
When a domain has a NIC of type network pointing to a network with hostdev
forward, libvirt changes its running XML definition with a hostdev
interface with a PCI address from those in the network.

Handle this case to avoid useless interface detaching / attaching.
Libvirt adds the PCI addresses of the SR-IOV device virtual functions
when only providing the physical function. Those need to be removed in
order to avoid network changes for no reason in virt.network_update()
@cbosdo
Copy link
Contributor Author

cbosdo commented Jan 8, 2021

Just fixed the rebase on latest master to get the new test pass again.

@cbosdo
Copy link
Contributor Author

cbosdo commented Jan 8, 2021

re-run pr-amazon2-py3-pytest

@cbosdo
Copy link
Contributor Author

cbosdo commented Jan 11, 2021

re-run freebsd122-py3-pytest

Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

🚀

@cbosdo
Copy link
Contributor Author

cbosdo commented Jan 12, 2021

re-run freebsd122-py3-pytest

@cbosdo
Copy link
Contributor Author

cbosdo commented Jan 12, 2021

re-run ubuntu2004-py3-pytest

@cbosdo
Copy link
Contributor Author

cbosdo commented Jan 12, 2021

@dwoz could you merge this PR while it's Green and ACKed?

@cbosdo
Copy link
Contributor Author

cbosdo commented Jan 13, 2021

re-run freebsd122-py3-pytest

@cbosdo cbosdo mentioned this pull request Jan 13, 2021
3 tasks
@sagetherage sagetherage added the Aluminium Release Post Mg and Pre Si label Jan 13, 2021
@dwoz dwoz merged commit 3c1eaca into saltstack:master Jan 13, 2021
@cbosdo cbosdo deleted the virt-network branch January 14, 2021 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow more virtual network types in virt states
4 participants