Salt: virt.running: not idempotent (fails if VM already running)

Created on 17 May 2019 · 14Comments · Source: saltstack/salt

Description of Issue/Question

salt.states.virt.running calls virt.vm_state to determine if the VM is already running, and if the result is anything other than the literal string running, it then updates it (if update: True is set in the state), and then tries to start the VM.

But salt.states.modules.vm_state always returns a dict, whch means the return value will never be the literal string running. (Possibly this once worked, but the definition of vm_state was changed?)

This dict return can also be seen testing virt.vm_state from the command line:

ewen@noc:/etc/salt/pillar$ sudo salt 'naosr620.naos.co.nz' virt.vm_state debian_unstable
naosr620.naos.co.nz:
    ----------
    debian_unstable:
        running
ewen@noc:/etc/salt/pillar$

From a quick glance it appears that salt.states.virt.running needs to index into the dict it gets back to get the state it is looking for (at https://github.com/saltstack/salt/blob/develop/salt/states/virt.py#L405; I'm unclear what the purpose of the call on the previous line is, since it seems like it would happen a second time on line 405 anyway, and the result from the call in line 404 is ignored?!).

Setup

Create a simple libvirt VM with virt.running:

debian_unstable:
    virt.running:
      - cpu: 1
      - mem: 512
      - vm_type: kvm
      - disk_profile: 
      - disks:
          - name: vda
            source_file: /dev/ssd/debian_unstable_vda
            format: raw
            model: virtio
      - install: False
      - nic_profile: 
      - interfaces:
          - name: eth0
            type: bridge
            source: virbr0
            mac: 00:01:02:03:04:05
            model: virtio

Steps to Reproduce Issue

Define a VM with the config above, do state.highstate which results in it running again. Do state.highstate a second time, and get the report:

ewen@noc:/etc/salt/pillar$ ths naosr620.naos.co.nz
naosr620.naos.co.nz:
----------
          ID: debian_unstable
    Function: virt.running
      Result: False
     Comment: Requested operation is not valid: domain is already running
     Started: 17:04:23.748135
    Duration: 15.583 ms
     Changes:   

Summary for naosr620.naos.co.nz
--------------
Succeeded: 105
Failed:      1
--------------
Total states run:     106
Total run time:     5.662 s
ERROR: Minions returned with non-zero exit code
ewen@noc:/etc/salt/pillar$

(ths is a shell script that does sudo salt "$@" state.highstate test=True --state-output=changes --output-diff, because that's a lot to type :-) Note that salt.states.virt.running happens even in test=True mode, which seems like a separate bug.)

Versions Report

salt master is currently on Debian Jessie (8), with Salt 2019.2.0 on Python 2.7; salt minions are all Salt 2019.2.0, and in the case of the minion where I ran into this issue the salt minion is on Ubuntu 18.04 LTS with Salt 20.19.2.0 on Python 2.7.

ewen@noc:~$ salt --versions-report
Salt Version:
           Salt: 2019.2.0

Dependency Versions:
           cffi: 0.8.6
       cherrypy: Not Installed
       dateutil: 2.2
      docker-py: Not Installed
          gitdb: 0.5.4
      gitpython: 0.3.2 RC1
          ioflo: Not Installed
         Jinja2: 2.9.4
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.2
   mysql-python: Not Installed
      pycparser: 2.10
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.9 (default, Sep 25 2018, 23:32:58)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 14.4.0
           RAET: Not Installed
          smmap: 0.8.2
        timelib: Not Installed
        Tornado: 4.4.3
            ZMQ: 4.0.5

System Versions:
           dist: debian 8.11 
         locale: ANSI_X3.4-1968
        machine: i686
        release: 4.9.0-0.bpo.9-686-pae
         system: Linux
        version: debian 8.11 

ewen@noc:~$

Bug Confirmed severity-medium

Source

ewenmcneill

All 14 comments

With the patch below (applied on the minion and the salt-minion service restarted), virt.running appears to be fine with a VM already running, and just reports success.

ewen@noc:/etc/salt/pillar$ hs naosr620.naos.co.nz
[...]
  Name: libvirtd - Function: service.running - Result: Clean Started: - 17:20:55.952533 Duration: 79.735 ms
  Name: debian_unstable - Function: virt.running - Result: Clean Started: - 17:20:56.032958 Duration: 8.045 ms
[...]
Summary for naosr620.naos.co.nz
--------------
Succeeded: 106
Failed:      0
--------------
Total states run:     106
Total run time:     5.170 s
ewen@noc:/etc/salt/pillar$

and if the VM is not defined, then it is defined and started as expected:

ewen@noc:/etc/salt/pillar$ hs naosr620.naos.co.nz
[...]
  Name: libvirtd - Function: service.running - Result: Clean Started: - 17:23:57.312997 Duration: 79.445 ms
----------
          ID: debian_unstable
    Function: virt.running
      Result: True
     Comment: Domain debian_unstable defined and started
     Started: 17:23:57.393027
    Duration: 1160.685 ms
     Changes:   
              ----------
              debian_unstable:
                  Domain defined and started
[...]
Summary for naosr620.naos.co.nz
--------------
Succeeded: 106 (changed=1)
Failed:      0
--------------
Total states run:     106
Total run time:     6.918 s
ewen@noc:/etc/salt/pillar$

and if the VM is defined, but not started:

virsh # list --inactive
 Id    Name                           State
----------------------------------------------------
 -     debian_unstable                shut off

virsh #

then the VM is just started as expected:

ewen@noc:/etc/salt/pillar$ hs naosr620.naos.co.nz
[...]
  Name: libvirtd - Function: service.running - Result: Clean Started: - 17:26:24.663625 Duration: 83.959 ms
----------
          ID: debian_unstable
    Function: virt.running
      Result: True
     Comment: Domain debian_unstable started
     Started: 17:26:24.748305
    Duration: 1191.459 ms
     Changes:   
              ----------
              debian_unstable:
                  Domain started
[...]
Summary for naosr620.naos.co.nz
--------------
Succeeded: 106 (changed=1)
Failed:      0
--------------
Total states run:     106
Total run time:     6.440 s
ewen@noc:/etc/salt/pillar$

So AFAICT this patch properly detects a running VM and does the right thing, but the shipped 2019.2 code is not able to do so.

Ewen

PS: hs is another shell script convenience wrapper which does sudo salt "$@" state.highstate --state-output=changes.

ewen@naosr620:~$ diff -u /usr/lib/python2.7/dist-packages/salt/states/virt.py-old2 /usr/lib/python2.7/dist-packages/salt/states/virt.py
--- /usr/lib/python2.7/dist-packages/salt/states/virt.py-old2   2019-05-17 16:14:04.272620242 +1200
+++ /usr/lib/python2.7/dist-packages/salt/states/virt.py    2019-05-17 17:19:49.960989983 +1200
@@ -396,7 +396,9 @@
     try:
         try:
             __salt__['virt.vm_state'](name)
-            if __salt__['virt.vm_state'](name) != 'running':
+            # 2019-05-17 - virt.vm_state always returns a dict
+            # See: https://github.com/saltstack/salt/issues/53107
+            if __salt__['virt.vm_state'](name).get(name, None) != 'running':
                 action_msg = 'started'
                 if update:
                     status = __salt__['virt.update'](name,
ewen@naosr620:~$

ewenmcneill on 17 May 2019

@ewenmcneill Thanks for the report. Looking at the code for virt.vm_state, it appears that this function has always returned a dictionary. Looks like you've got a solution, can you submit a PR with the above fixes and some tests to ensure this doesn't result in a regression later.

garethgreenaway on 17 May 2019

Thanks for the analysis that this has "always" been a problem.

I'm busy with server migrations, etc, for the next few weeks (and have enough of a work around for my migration purposes now), but if no one else gets to it before I get some free time then yes, I'll figure out how to add some tests to salt for this, and submit a PR. I'm happy if someone else with more existing knowledge of how to add tests to salt wants to do it first.

Ewen

ewenmcneill on 18 May 2019

@garethgreenaway feel free to ping me on the virt.* issues. Since I am not actively monitoring the salt issues, that would fasten the process.

cbosdo on 13 Aug 2019

Just hit this myself. Status?

brianthelion on 4 Sep 2019

@brianthelion the PR has been ACKed but not merged yet... waiting

cbosdo on 5 Sep 2019

👍1

@garethgreenaway Anything concerning with the PR above?

brianthelion on 9 Sep 2019

the PR has been ACKed but not merged yet... waiting

Unfortunately looks like this didn't make it into 2019.2.2 either. So I'm still carrying my local patch for this issue.

Per https://github.com/saltstack/salt/issues/53107#issuecomment-493527083, it looks like it "never worked", and @cbosdo's patch looks pretty small/safe, so it'd be helpful to see https://github.com/saltstack/salt/pull/54196 get merged. (Particuarly since the main code base seems to be moving ahead with other unrelated changes to the same files.)

Ewen

ewenmcneill on 10 Dec 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

stale[bot] on 10 Jan 2020

Looks like the PR to fix this has been merged (https://github.com/saltstack/salt/pull/54196). But I don't think it's released yet. For now I think it'd help to keep this issue open until there's a release including that PR.

(Commenting to keep open -- stale after 1 month of inactivity, especially when that inactivity is "PR hasn't been merged in months", seems a pretty aggressive "stale bot" setting...)

Ewen

ewenmcneill on 10 Jan 2020

Thank you for updating this issue. It is no longer marked as stale.

stale[bot] on 10 Jan 2020

Looks like the PR to fix this has been merged (#54196). But I don't think it's released yet.

FTR, it seems like the merged fix didn't make it into the 2019.2.3 release, so I'm still patching my states/virt.py by hand.

ewen@naosr620:~$ dpkg -l salt-common | grep salt
ii  salt-common    2019.2.3+ds-1 all          shared libraries that salt requires for all packages
ewen@naosr620:~$ dpkg -L salt-common | grep states/virt.py
/usr/lib/python2.7/dist-packages/salt/states/virt.py
ewen@naosr620:~$ grep "!= 'running'" $(dpkg -L salt-common | grep states/virt.py)
            if __salt__['virt.vm_state'](name) != 'running':
ewen@naosr620:~$

(compare that code with the pre-patched version in https://github.com/saltstack/salt/issues/53107#issuecomment-493324963, and the fix in https://github.com/saltstack/salt/pull/54196/files#diff-082ca74577bb246c4f2d5b5d24722339; it's the pre-fixed version.)

Ewen

ewenmcneill on 12 Feb 2020

😕1

@ewenmcneill That would be correct - starting with Neon (3000) we changed our branching and release strategy. 2019.2.3 was a CVE release only.

You can read the text of the SEP for more info, but the summary is that Salt is following a more typical release strategy, rather than maintaining a half-dozen branches for releases.