Additional target states for virt module: "destroyed" and "paused" #5079

Merged
merged 1 commit into from Mar 11, 2014

Conversation

Projects
None yet
4 participants
@candlerb
Contributor

candlerb commented Nov 27, 2013

It's more convenient to do:

virt: name=foo state=destroyed

than to first test the current state and then do:

virt: name=foo command=destroy
@mpdehaan

This comment has been minimized.

Show comment Hide comment
@mpdehaan

This comment has been minimized.

Show comment Hide comment
@mpdehaan

mpdehaan Nov 27, 2013

Contributor

When adding new arguments that are choices, it's neccessary to indicate in the docs when the new choices were added or people will try them on old versions.

Contributor

mpdehaan commented Nov 27, 2013

When adding new arguments that are choices, it's neccessary to indicate in the docs when the new choices were added or people will try them on old versions.

@mpdehaan

This comment has been minimized.

Show comment Hide comment
@mpdehaan

mpdehaan Nov 27, 2013

Contributor

Logic review:

 if state == 'running':
        if v.status(guest) is 'paused':
            res['changed'] = True
            res['msg'] = v.unpause(guest)
        elif v.status(guest) is not 'running':
            res['changed'] = True
            res['msg'] = v.start(guest)

unpause looks fine here

    elif state == 'shutdown':
        if v.status(guest) is not 'shutdown':
            res['changed'] = True
            res['msg'] = v.shutdown(guest)
    elif state == 'destroyed':
        if v.status(guest) is not 'shutdown':
            res['changed'] = True
            res['msg'] = v.destroy(guest)

This doesn't remove guests that are in shutdown state and probably should.

    elif state == 'paused':
        if v.status(guest) is 'running':
            res['changed'] = True
            res['msg'] = v.pause(guest)

What should happen if the machine is not running? Should it start it and then pause? Or should it raise an error? This seems like it should probably be handled.

    else:

I'd suspect these commands are in the command form that they are probably because they don't lend themselves well to stateful management, but I'm open to exploring how to make that more logical.

Contributor

mpdehaan commented Nov 27, 2013

Logic review:

 if state == 'running':
        if v.status(guest) is 'paused':
            res['changed'] = True
            res['msg'] = v.unpause(guest)
        elif v.status(guest) is not 'running':
            res['changed'] = True
            res['msg'] = v.start(guest)

unpause looks fine here

    elif state == 'shutdown':
        if v.status(guest) is not 'shutdown':
            res['changed'] = True
            res['msg'] = v.shutdown(guest)
    elif state == 'destroyed':
        if v.status(guest) is not 'shutdown':
            res['changed'] = True
            res['msg'] = v.destroy(guest)

This doesn't remove guests that are in shutdown state and probably should.

    elif state == 'paused':
        if v.status(guest) is 'running':
            res['changed'] = True
            res['msg'] = v.pause(guest)

What should happen if the machine is not running? Should it start it and then pause? Or should it raise an error? This seems like it should probably be handled.

    else:

I'd suspect these commands are in the command form that they are probably because they don't lend themselves well to stateful management, but I'm open to exploring how to make that more logical.

@candlerb

This comment has been minimized.

Show comment Hide comment
@candlerb

candlerb Nov 27, 2013

Contributor

This doesn't remove guests that are in shutdown state and probably should.

"destroy" doesn't remove a guest (either from the stored libvirt XML definition, nor its disk image)

In libvirt terminology, "destroy" just means "immediate halt", like pulling the plug out, whereas "shutdown" means "send the (ACPI) shutdown signal, and leave it up to the VM when to shut itself down". IMO "destroy" was a bad choice of terminology, but it's what we have to live with.

AFAIK there are not separate states "VM was shutdown" and "VM was destroyed" (*). When you see state=shutdown it just means "not running", and there are two ways to get it into that state: command=shutdown or command=destroy. The first may not always work, e.g. if the VM is hung, and it could take a long time. The second may be what you wanted anyway, e.g. if this is a temporary VM that you want to halt immediately and don't care about filesystem corruption.

In ansible, being able to request state=destroyed would be much more convenient than command=destroy, because command=destroy fails if the VM is not currently running (even though that is the state you wanted!)

Regarding state=paused:

What should happen if the machine is not running? Should it start it and then pause? Or should it raise an error?

I thought along the same lines as you, but I took the view that if the machine is currently shutdown, it should stay like that. There is no "state=unpaused", so when you want to unpause a machine you set "state=running". If the machine were shutdown that would of course start it.

There seems to be little point in having a "start-then-immediately-pause" operation, and in any case that would be difficult to implement safely.

(*) I note that there are two libvirt states which are mapped to "shutdown" in ansible:

VIRT_STATE_NAME_MAP = {
   0 : "running",
   1 : "running",
   2 : "running",
   3 : "paused",
   4 : "shutdown",
   5 : "shutdown",
   6 : "crashed"
}

After sending a 'destroy' command, or even after manually killing the KVM process, ansible says the domain is 'shutdown'. So I don't know what condition can result in 'crashed'

Looking at libvirt source (libvirt/include/libvirt/libvirt.h) I see:

/**
 * virDomainState:
 *
 * A domain may be in different states at a given point in time
 */
typedef enum {
    VIR_DOMAIN_NOSTATE = 0,     /* no state */
    VIR_DOMAIN_RUNNING = 1,     /* the domain is running */
    VIR_DOMAIN_BLOCKED = 2,     /* the domain is blocked on resource */
    VIR_DOMAIN_PAUSED  = 3,     /* the domain is paused by user */
    VIR_DOMAIN_SHUTDOWN= 4,     /* the domain is being shut down */
    VIR_DOMAIN_SHUTOFF = 5,     /* the domain is shut off */
    VIR_DOMAIN_CRASHED = 6,     /* the domain is crashed */
    VIR_DOMAIN_PMSUSPENDED = 7, /* the domain is suspended by guest
                                   power management */

So actually 4 and 5 are different: 4 means "in the process of shutting down" and 5 is "off".

It would be better IMO if ansible reported "shutting_down" and "shutdown" as two separate states. Then the idempotent "shutdown" operation would not send a second signal to a machine which has already been told to shut down.

Contributor

candlerb commented Nov 27, 2013

This doesn't remove guests that are in shutdown state and probably should.

"destroy" doesn't remove a guest (either from the stored libvirt XML definition, nor its disk image)

In libvirt terminology, "destroy" just means "immediate halt", like pulling the plug out, whereas "shutdown" means "send the (ACPI) shutdown signal, and leave it up to the VM when to shut itself down". IMO "destroy" was a bad choice of terminology, but it's what we have to live with.

AFAIK there are not separate states "VM was shutdown" and "VM was destroyed" (*). When you see state=shutdown it just means "not running", and there are two ways to get it into that state: command=shutdown or command=destroy. The first may not always work, e.g. if the VM is hung, and it could take a long time. The second may be what you wanted anyway, e.g. if this is a temporary VM that you want to halt immediately and don't care about filesystem corruption.

In ansible, being able to request state=destroyed would be much more convenient than command=destroy, because command=destroy fails if the VM is not currently running (even though that is the state you wanted!)

Regarding state=paused:

What should happen if the machine is not running? Should it start it and then pause? Or should it raise an error?

I thought along the same lines as you, but I took the view that if the machine is currently shutdown, it should stay like that. There is no "state=unpaused", so when you want to unpause a machine you set "state=running". If the machine were shutdown that would of course start it.

There seems to be little point in having a "start-then-immediately-pause" operation, and in any case that would be difficult to implement safely.

(*) I note that there are two libvirt states which are mapped to "shutdown" in ansible:

VIRT_STATE_NAME_MAP = {
   0 : "running",
   1 : "running",
   2 : "running",
   3 : "paused",
   4 : "shutdown",
   5 : "shutdown",
   6 : "crashed"
}

After sending a 'destroy' command, or even after manually killing the KVM process, ansible says the domain is 'shutdown'. So I don't know what condition can result in 'crashed'

Looking at libvirt source (libvirt/include/libvirt/libvirt.h) I see:

/**
 * virDomainState:
 *
 * A domain may be in different states at a given point in time
 */
typedef enum {
    VIR_DOMAIN_NOSTATE = 0,     /* no state */
    VIR_DOMAIN_RUNNING = 1,     /* the domain is running */
    VIR_DOMAIN_BLOCKED = 2,     /* the domain is blocked on resource */
    VIR_DOMAIN_PAUSED  = 3,     /* the domain is paused by user */
    VIR_DOMAIN_SHUTDOWN= 4,     /* the domain is being shut down */
    VIR_DOMAIN_SHUTOFF = 5,     /* the domain is shut off */
    VIR_DOMAIN_CRASHED = 6,     /* the domain is crashed */
    VIR_DOMAIN_PMSUSPENDED = 7, /* the domain is suspended by guest
                                   power management */

So actually 4 and 5 are different: 4 means "in the process of shutting down" and 5 is "off".

It would be better IMO if ansible reported "shutting_down" and "shutdown" as two separate states. Then the idempotent "shutdown" operation would not send a second signal to a machine which has already been told to shut down.

@candlerb

This comment has been minimized.

Show comment Hide comment
@candlerb

candlerb Nov 27, 2013

Contributor

Related: https://groups.google.com/forum/#!topic/ansible-project/WpRblldA2PQ

Yes, sorry about my fuzzy use of language, but I was trying to distinguish the "state=foo" option of the virt module from the "command=foo" option.

Contributor

candlerb commented Nov 27, 2013

Related: https://groups.google.com/forum/#!topic/ansible-project/WpRblldA2PQ

Yes, sorry about my fuzzy use of language, but I was trying to distinguish the "state=foo" option of the virt module from the "command=foo" option.

@candlerb

This comment has been minimized.

Show comment Hide comment
@candlerb

candlerb Nov 27, 2013

Contributor

Maybe there is value in having both state=paused and state=unpaused.

state=paused: "if this maching is running, pause it. But if it's paused or shutdown, leave it alone."

state=unpaused: "if this machine is paused, unpause it. But if it's running or shutdown, leave it alone."

Contributor

candlerb commented Nov 27, 2013

Maybe there is value in having both state=paused and state=unpaused.

state=paused: "if this maching is running, pause it. But if it's paused or shutdown, leave it alone."

state=unpaused: "if this machine is paused, unpause it. But if it's running or shutdown, leave it alone."

@mpdehaan

This comment has been minimized.

Show comment Hide comment
@mpdehaan

mpdehaan Dec 1, 2013

Contributor

That seems curious, because you'd not know what the end state would be.

Contributor

mpdehaan commented Dec 1, 2013

That seems curious, because you'd not know what the end state would be.

@candlerb

This comment has been minimized.

Show comment Hide comment
@candlerb

candlerb Dec 2, 2013

Contributor

On 01/12/2013 22:29, Michael DeHaan wrote:

That seems curious, because you'd not know what the end state would be.

It's just a possible usage case. Why might you want to pause VMs in the
first place? Perhaps because you want to quiesce them for non-live
migration, or because you want to deal with a temporary resource overload.

Once you have completed this work, you want to put them back as they
were: if they were running before then unpause them, but if they were
halted then you don't want to start them.

However this isn't a use case that I need, so I don't really mind if we
just have "paused" and "running".

Contributor

candlerb commented Dec 2, 2013

On 01/12/2013 22:29, Michael DeHaan wrote:

That seems curious, because you'd not know what the end state would be.

It's just a possible usage case. Why might you want to pause VMs in the
first place? Perhaps because you want to quiesce them for non-live
migration, or because you want to deal with a temporary resource overload.

Once you have completed this work, you want to put them back as they
were: if they were running before then unpause them, but if they were
halted then you don't want to start them.

However this isn't a use case that I need, so I don't really mind if we
just have "paused" and "running".

jimi-c added a commit that referenced this pull request Mar 11, 2014

Merge pull request #5079 from candlerb/candlerb/virt_states
Additional target states for virt module: "destroyed" and "paused"

@jimi-c jimi-c merged commit 10f7a20 into ansible:devel Mar 11, 2014

@jimi-c

This comment has been minimized.

Show comment Hide comment
@jimi-c

jimi-c Mar 11, 2014

Owner

Merged, thanks!

Owner

jimi-c commented Mar 11, 2014

Merged, thanks!

@ansibot ansibot added feature and removed feature_pull_request labels Mar 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment