Add virt.*defined states #55814

cbosdo · 2020-01-08T14:24:24Z

What does this PR do?

This PR extracts the code defining and/or updating the VM, storage pool or network from the corresponding virt.*running state into virt.*defined states.

This allows ensuring a VM, storage pool or network is defined without needing to care about it's status.

While at it, implement the __opts__['test'] support for those states. The returned values from the running states have been keep intact as much as possible.

Since the running states are using the new defined ones, the existing running tests are enough to ensure the defined states are working fine.

What issues does this PR fix or reference?

Tests written?

Yes

Commits signed with GPG?

Yes

cbosdo · 2020-01-09T16:47:00Z

@Ch3LL I suspect the failing test to be unrelated to the PR

cbosdo · 2020-01-09T16:47:20Z

BTW, this PR is ready for review

cbosdo · 2020-01-09T19:37:56Z

oops... just found out a regression here. Will submit an update soon

cbosdo · 2020-01-09T19:50:26Z

Fixed now :)

tests/unit/states/test_virt.py

salt/states/virt.py

cbosdo · 2020-01-27T12:39:57Z

@Ch3LL comments addressed

cbosdo · 2020-02-01T17:05:33Z

rebased on master

cbosdo · 2020-02-03T21:24:26Z

@Akm0d unlike what I suspected, the bug I saw in the demo is not related to this PR at all since the resume action doesn't involve the virt.running state at all. I couldn't even reproduce the error after the demo... definitely need to do some sacrifice to the demo kami next time

Ch3LL · 2020-02-10T15:27:26Z

salt/states/virt.py

-           }
-
-    try:
+    if update:


It seems that someone still cannot use the update kwarg to change behavior if i'm reading it correctly. What if they want to ensure an update does not occur? This would be changing behavior without proper deprecation still. Can you elaborate why you are changing this behavior?

If using the update parameter to ensure an update doesn't occur, this means that users have to set the deprecated parameter to True in order to be able to use the new behavior... which doesn't make sense at all. And changing the default value for update is disruptive as well.

The option that brings the least issues is to leave the update parameter but warn that it is not used: the VM will only be updated if there is a difference in the configuration. This fits more the Salt states spirit and in all cases the update doesn't stop the VM to update it: it tries as much as possible to apply live changes.

users have to set the deprecated parameter to True in order to be able to use the new behavior... which doesn't make sense at all.

Why does this not make sense? This would allow this to be an opt in feature while not changing behavior for users for another couple releases so they are prepared.

salt/states/virt.py

salt/modules/virt.py

salt/states/virt.py

Ch3LL · 2020-03-09T14:27:52Z

salt/states/virt.py

-           }
-
-    try:
+    if update:


users have to set the deprecated parameter to True in order to be able to use the new behavior... which doesn't make sense at all.

Why does this not make sense? This would allow this to be an opt in feature while not changing behavior for users for another couple releases so they are prepared.

cbosdo · 2020-03-09T14:41:39Z

Why does this not make sense? This would allow this to be an opt in feature while not changing behavior for users for another couple releases so they are prepared.

Well having to set a deprecated parameter in order to use a new feature sounds awkward to me.
And setting update=True doesn't necessarily mean there will be an update: if there was no change there will be no update.
And that forces to add a deprecated parameter to the new virt.defined state.

cbosdo · 2020-04-14T07:18:42Z

@cbosdo Black needs to be run on this PR

@dwoz done

cbosdo · 2020-04-14T18:38:18Z

argh! I broke a test while rebasing... will fix tomorrow

cbosdo · 2020-04-15T07:12:11Z

Fixed! I also cherry picked a commit from PR #56414 to fix the test on py3.8 which is what I am running locally

cbosdo · 2020-04-20T08:48:13Z

rebased on master and resolved the conflicts after the deprecated removal in virt states

Users may want to use states to ensure a virtual storage pool is defined and not enforce it to be running. Extract the code that performs the pool definition / update from virt.pool_running state into a virt.pool_defined. Obviously the virt.pool_running state calls the virt.pool_defined one. In such a case no additionnal test is needed for virt.pool_defined since this is already tested with virt.pool_running.

In order to allow running dry-runs of virt.update module add a test parameter. This will later be used by the virt states.

In order to ensure a virtual guest is defined independently of its status, extract the corresponding code from the virt.running state. This commit also handles the __opts__['test'] for the running state. Since the update call only performs changes if needed, deprecate the update parameter.

Just like domains and storage pools, users may want to ensure a network is defined without influencing it's status. Extract the code from network_running state defining the network into a network_defined state. While at it, support __opt__['test'] == True in these states. Updating the network definition in the pool_defined state will come in a future PR.

virt.running state now may call virt.update with None mem and cpu parameters. This was not handled in _gen_xml(). Also add some more tests cases matching this for virt.update.

cbosdo requested a review from a team as a code owner January 8, 2020 14:24

cbosdo force-pushed the virt-defined-states branch from fe16e44 to fc8080e Compare January 8, 2020 15:32

ghost requested a review from Ch3LL January 8, 2020 19:16

cbosdo force-pushed the virt-defined-states branch 3 times, most recently from 73f3df8 to b21a93b Compare January 9, 2020 08:20

cbosdo force-pushed the virt-defined-states branch from a50af64 to 9c5b016 Compare January 9, 2020 19:49

cbosdo mentioned this pull request Jan 13, 2020

openSUSE 2019.2.0 - virt defined states openSUSE/salt#201

Closed

Ch3LL suggested changes Jan 21, 2020

View reviewed changes

tests/unit/states/test_virt.py Outdated Show resolved Hide resolved

Ch3LL suggested changes Jan 21, 2020

View reviewed changes

salt/states/virt.py Outdated Show resolved Hide resolved

cbosdo force-pushed the virt-defined-states branch from 9c5b016 to cb8c210 Compare January 27, 2020 12:29

cbosdo force-pushed the virt-defined-states branch 2 times, most recently from 41b8b42 to 3be80eb Compare February 1, 2020 16:59

Ch3LL suggested changes Feb 10, 2020

View reviewed changes

salt/states/virt.py Outdated Show resolved Hide resolved

salt/states/virt.py Outdated Show resolved Hide resolved

cbosdo force-pushed the virt-defined-states branch 2 times, most recently from 9552ea7 to dbd1d49 Compare March 4, 2020 10:09

cbosdo mentioned this pull request Mar 5, 2020

Add virtual storage pool edit action uyuni-project/uyuni#1166

Merged

15 tasks

cbosdo force-pushed the virt-defined-states branch from dbd1d49 to 49ab152 Compare March 9, 2020 13:15

cbosdo mentioned this pull request Mar 9, 2020

Open suse 2019.2.3 virt defined states openSUSE/salt#219

Merged

Ch3LL suggested changes Mar 9, 2020

View reviewed changes

cbosdo force-pushed the virt-defined-states branch from 49ab152 to 9d172b4 Compare March 9, 2020 16:36

cbosdo mentioned this pull request Apr 14, 2020

virt._get_domain: don't raise an exception if there is no VM #56392

Merged

cbosdo dismissed Ch3LL’s stale review via f35fb97 April 14, 2020 07:17

cbosdo force-pushed the virt-defined-states branch from 8c4abae to f35fb97 Compare April 14, 2020 07:17

cbosdo force-pushed the virt-defined-states branch from f35fb97 to bc113be Compare April 15, 2020 07:10

Ch3LL removed Merge Ready Feature new functionality including changes to functionality and code refactors, etc. Salt-Cloud labels Apr 17, 2020

Ch3LL previously approved these changes Apr 17, 2020

View reviewed changes

cbosdo dismissed Ch3LL’s stale review via fdd8937 April 20, 2020 08:46

cbosdo force-pushed the virt-defined-states branch 2 times, most recently from fdd8937 to 660147d Compare April 20, 2020 08:47

cbosdo force-pushed the virt-defined-states branch from 660147d to 9bb5167 Compare April 20, 2020 09:03

cbosdo added 5 commits April 21, 2020 09:10

Add virt.update test parameter

72ab311

In order to allow running dry-runs of virt.update module add a test parameter. This will later be used by the virt states.

Fix virt.update to handle None mem and cpu

4e6ea37

virt.running state now may call virt.update with None mem and cpu parameters. This was not handled in _gen_xml(). Also add some more tests cases matching this for virt.update.

cbosdo force-pushed the virt-defined-states branch from 9bb5167 to 4e6ea37 Compare April 21, 2020 07:11

Merge branch 'master' into virt-defined-states

c882782

Ch3LL approved these changes Apr 22, 2020

View reviewed changes

Ch3LL added the Merge Ready label Apr 22, 2020

dwoz merged commit 1317142 into saltstack:master Apr 22, 2020

Core Open PRs automation moved this from In progress to Done Apr 22, 2020

cbosdo deleted the virt-defined-states branch April 23, 2020 08:23

sagetherage added the ZRelease-Sodium retired label label May 18, 2020

sagetherage added this to Done in Sodium May 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add virt.*defined states #55814

Add virt.*defined states #55814

cbosdo commented Jan 8, 2020

cbosdo commented Jan 9, 2020

cbosdo commented Jan 9, 2020

cbosdo commented Jan 9, 2020

cbosdo commented Jan 9, 2020

cbosdo commented Jan 27, 2020

cbosdo commented Feb 1, 2020

cbosdo commented Feb 3, 2020

Ch3LL Feb 10, 2020

cbosdo Mar 4, 2020

Ch3LL Mar 9, 2020

Ch3LL Mar 9, 2020

cbosdo commented Mar 9, 2020 •

edited

cbosdo commented Apr 14, 2020

cbosdo commented Apr 14, 2020

cbosdo commented Apr 15, 2020

cbosdo commented Apr 20, 2020

Add virt.*defined states #55814

Add virt.*defined states #55814

Conversation

cbosdo commented Jan 8, 2020

What does this PR do?

What issues does this PR fix or reference?

Tests written?

Commits signed with GPG?

cbosdo commented Jan 9, 2020

cbosdo commented Jan 9, 2020

cbosdo commented Jan 9, 2020

cbosdo commented Jan 9, 2020

cbosdo commented Jan 27, 2020

cbosdo commented Feb 1, 2020

cbosdo commented Feb 3, 2020

Ch3LL Feb 10, 2020

Choose a reason for hiding this comment

cbosdo Mar 4, 2020

Choose a reason for hiding this comment

Ch3LL Mar 9, 2020

Choose a reason for hiding this comment

Ch3LL Mar 9, 2020

Choose a reason for hiding this comment

cbosdo commented Mar 9, 2020 • edited

cbosdo commented Apr 14, 2020

cbosdo commented Apr 14, 2020

cbosdo commented Apr 15, 2020

cbosdo commented Apr 20, 2020

cbosdo commented Mar 9, 2020 •

edited