Skip to content

New vmctl module to interface with the OpenBSD VMM hypervisor#45164

Merged
rallytime merged 3 commits intosaltstack:developfrom
jasperla:vmctl
Jan 9, 2018
Merged

New vmctl module to interface with the OpenBSD VMM hypervisor#45164
rallytime merged 3 commits intosaltstack:developfrom
jasperla:vmctl

Conversation

@jasperla
Copy link
Contributor

What does this PR do?

Provide a new module that allows Salt to interact with the OpenBSD VMM hypervisor.

Previous Behavior

NA

New Behavior

This new module implements the following functionality:

  • create a new disk image
  • load a new configuration file
  • reload the default configuration
  • stop VMs
  • reset VMM or one of the subsystems
  • define new VMs and start existing VMs
  • request status from all or one VM(s)

Tests written?

No

Commits signed with GPG?

Yes

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.

@jasperla Thanks again for these additions! Keep 'em coming!

I have a couple of comments here. This will also need some unit tests added as well before we can get it in.

Copy link
Contributor

Choose a reason for hiding this comment

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

A versionadded for Fluorine will be needed for this new file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check, I'll it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We're moving away from bare salt.utils imports. All of the functions there have been moved over into their own files. Can you update this import to import the individual files needed import salt.utils.x? (And of course update the references in the function calls below.)

The salt.utils.__init__.py file should have the up-to-date paths in the deprecation warnings for each function for reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was actually only one (salt.utils.path). I'll adjust it.

@rallytime rallytime added the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Dec 27, 2017
@jasperla jasperla force-pushed the vmctl branch 3 times, most recently from 62fb339 to 1f73701 Compare December 27, 2017 20:56
@jasperla
Copy link
Contributor Author

I've now added tests too.

Copy link
Contributor

@eradman eradman left a comment

Choose a reason for hiding this comment

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

If no argument is passed to vmctl.status then this function mistakenly filters VMs by the name None

[INFO    ] Executing command ['vmctl', 'status', None] in directory '/home/eradman'
[TRACE   ] stdout:    ID   PID VCPUS  MAXMEM  CURMEM     TTY        OWNER NAME
[DEBUG   ] LazyLoaded nested.output
[TRACE   ] data = {'local': {}}

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be if id:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I had accidentally changed that. It's fixed now.

Copy link
Contributor

@eradman eradman left a comment

Choose a reason for hiding this comment

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

There is an edge case not handled here when vmctl reports that the VM is in the state "stopping"

@jasperla jasperla force-pushed the vmctl branch 2 times, most recently from 00892c3 to b33eb37 Compare December 28, 2017 10:39
@jasperla
Copy link
Contributor Author

@eradman I've fixed the edge case too now. Turns out it's a bug in vmctl where it doesn't output the status line for a VM that is stopping when the status is requested for the VM by name (works by id). i.e. vmctl stop foo && vmctl status foo emits the header but nothing else.

Copy link
Contributor

@eradman eradman left a comment

Choose a reason for hiding this comment

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

@jasperla: previous fixes work. There is another flaw in the way vmctl reports status that causes some confusion. If I start a VM with a name that is already used the output is unclear:

$ doas salt-call --local vmctl.start name=web1 bootpath='/bsd.rd' nics=2 memory=512M disk='/tmp/disk.img'
local:
    ----------
    changes:
        True
    console:
        None

The output from vmctl is not much better

$ doas vmctl start web1 -i 2 -b /bsd.rd -m 512M -d /tmp/disk.img
vmctl: start vm command failed: Operation not permitted
$ echo $?
0

Not sure what this Salt module should do--perhaps we need to parse the status line for this VM?

(saltenv) $ doas vmctl status web1
   ID   PID VCPUS  MAXMEM  CURMEM     TTY        OWNER NAME
   12 54520     1    512M   93.3M   ttypj         root web1
 VCPU:  0 STATE: STOPPED

Copy link
Contributor

Choose a reason for hiding this comment

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

s/boothpath/bootpath/

@rallytime rallytime removed the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Dec 28, 2017
@jasperla
Copy link
Contributor Author

jasperla commented Dec 28, 2017

@eradman thanks for pointing out that bug in vmctl, I'll look into that.

I've now changed start() to return:

local:
    ----------
    changes:
        False
    comment:
        VM already exists and cannot be redefined
    console:
        None

So changes is now False and a comment is populated in case the VM that one tries to define already exists.

EDIT: tests now pass again too.

@eradman
Copy link
Contributor

eradman commented Dec 28, 2017

Functionally this change seems to be good!

@rallytime
Copy link
Contributor

re-run py

@rallytime
Copy link
Contributor

re-run py

@jasperla
Copy link
Contributor Author

jasperla commented Jan 9, 2018

Looking better now :)

@rallytime rallytime merged commit 57b6860 into saltstack:develop Jan 9, 2018
@jasperla jasperla deleted the vmctl branch January 9, 2018 20:27
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.

3 participants