Create libvirt xml files from templates with python #343

MaximilianMeister · 2015-05-13T08:23:40Z

No description provided.

aplanas · 2015-05-13T08:44:16Z

scripts/lib/libvirt/admin-config.py

+import argparse
+import libvirt_helpers
+from string import Template
+


If we are using flake8, I would suggest to follow some 'second-line' rules:

Put first (in alphabetically order) the Python libraries (argparse and string)

Separated with a line, put the other imports (also sorted) non-python libs (libvirt, libvirt_helpers)

https://www.python.org/dev/peps/pep-0008/#id17

aplanas · 2015-05-13T08:51:56Z

scripts/lib/libvirt/admin-config.py

+def main():
+    cloud = args.cloud
+    cpuflags = libvirt_helpers.cpuflags()
+    fout = open("/tmp/{0}-admin.xml".format(cloud), "w")


This file is not used until the end. I would suggest to use with statement:

with open("/tmp/{0}-admin.xml".format(cloud), "w") as f: f.write(xml)

http://effbot.org/zone/python-with-statement.htm

+1 for @aplanas's suggestion.

aplanas · 2015-05-13T08:57:13Z

scripts/lib/mkcloud-libvirt.sh

-    libvirt_onhost_create_adminnode_config
-    libvirt_onhost_create_admin_network_config
+    onhost_local_repository_mount
+    python ${mkcloud_lib_dir}/libvirt/admin-config.py $cloud $admin_node_memory $adminvcpus $emulator $admin_node_disk "$local_repository_mount"


Maybe you can chmod a+x admin-config.py and call as a normal command. In this case even I would recommend removing the .py extension too.

Yes, these scripts should definitely be executable. +1 for @aplanas's suggestion here too.

(Remember that git tracks file permissions too, not just contents.)

aspiers · 2015-05-13T09:06:12Z

scripts/lib/libvirt/compute-config.py

+    fout.write(xml)
+    fout.close()
+
+if __name__ == "__main__":


Same in this file - currently this __name__ check is useless because your CLI-handling code is in the wrong place.

aspiers · 2015-05-13T09:07:34Z

Great work, but please cleanly separate the CLI code from the library code like I suggested.

MaximilianMeister · 2015-05-13T09:44:24Z

thanks for all the input :) i will continue after lunch

MaximilianMeister · 2015-05-13T11:44:40Z

Great work, but please cleanly separate the CLI code from the library code like I suggested.

@aplanas @aspiers
I also had the intention to write this more modular, hence not one CLI for each "function", but I was really not sure how.
e.g. how I should pass all the computed variables that we need from mkcloud, how should I call it from within mkcloud and so on. I was even thinking about one central script with subcommands for each function but I don't know what makes most sense in the long term (and longer... if we go beyond libvirt with mkcloud)

I would definitely need some more help on that from the python side, due to lack of expertise.
So my suggestion would be doing this in a separate PR (maybe with some dedicated discussion of the python experts about the architecture), as it affects also the python scripts that have already been merged. This here is just another small step in the refactoring of mkcloud. What do you suggest?

aplanas · 2015-05-13T12:15:24Z

@MaximilianMeister I see.

So far the logic behind this code is not big. Initially I would put all the functions in a single Python package, and create several CLI (as you did) that parse the args and call the appropriate (and slim) library.

This approach can be used if mkcloud is finally written in Bash or Ruby. But if is done in Python we will need to refactor the library into several new packages with coupled functionality, because it will grow up a lot : )

Do we have a final decision about mkcloud?

jdsn · 2015-05-13T12:56:01Z

Do we have a final decision about mkcloud?

@aplanas No, we don't. And this is IMHO not related (direclty). The overall goal is to split mkcloud into separate tools (not just libraries). It should become separate tools that can be used individually, and mkcloud is using all of them.
Once mkcloud is small enough we can do a rewrite of it. I see python as the most likely option here (this is where it is related). Once mkcloud itself is python it can use the librarlies of the other tools directly. Until then it will call the scripts (ideally with subcommands, and not one script per function).

aplanas · 2015-05-13T13:06:37Z

@jdsn @MaximilianMeister I can help to refactor this to create subcommands. I will do it after the merge of this PR.

Can we provide some test file for this PR, so we can start the refactoring in a good way?

MaximilianMeister · 2015-05-13T13:21:41Z

I will try to setup some basic unittests as well for now. And maybe extract some of the logic in each CLI into the libvirt_helpers "module", which will be the base for the unittests.

aspiers · 2015-05-13T14:36:06Z

Great work, but please cleanly separate the CLI code from the library code like I suggested.

@aplanas @aspiers
I also had the intention to write this more modular, hence not one CLI for each "function", but I was really not sure how.

No problem, but since then I explained how you can do it :-)

e.g. how I should pass all the computed variables that we need from mkcloud, how should I call it from within mkcloud and so on.

The bash -> python interface you have done is already good. You only need to change the scripts so they're also usable as modules.

I was even thinking about one central script with subcommands for each function but I don't know what makes most sense in the long term (and longer... if we go beyond libvirt with mkcloud)

Big -1 for a single script. Code should always be in small, clean chunks, where each chunk does one thing really well.

I think you're too focused on the CLI aspect. Sure we can support a CLI as long as we need one, but the focus should be on writing reusable, testable, maintainable code, and the right way to do that is to ensure clean separation of concerns into separate modules, and to keep the CLI code separate from the business logic.

I would definitely need some more help on that from the python side, due to lack of expertise.
So my suggestion would be doing this in a separate PR (maybe with some dedicated discussion of the python experts about the architecture), as it affects also the python scripts that have already been merged. This here is just another small step in the refactoring of mkcloud. What do you suggest?

I suggest:

make each script reusable as a module by separating the CLI code from the business logic
later, move each script into a new module namespace which groups them all together, and move the CLI code into a single script which provides subcommands which delegate to each submodule in that namespace.

aspiers · 2015-05-13T14:39:19Z

scripts/lib/libvirt/admin-config

+
+    cloud = args.cloud
+    cpuflags = libvirt_helpers.cpuflags()
+    fin = libvirt_helpers.readfile("templates/admin-node.xml")


This is still not quite right. The template name is not specific to the CLI code so it doesn't belong here. The goal is ultimately to be able to write something like this from a Python-based mkcloud caller:

import mkcloud.libvirt ... mkcloud.libvirt.write_admin_node_xml(output_file, values)

bmwiedemann · 2015-05-21T12:27:53Z

scripts/lib/libvirt/libvirt_setup.py

+        targetbus = "virtio"
+    else:
+        nicmodel = "virtio"
+        targetdev = "sda"


should cover all vd* sd*

jdsn · 2015-05-22T09:30:30Z

scripts/lib/mkcloud-libvirt.sh

    libvirt_modprobe_kvm
    libvirt_start_daemon
-    python ${mkcloud_lib_dir}/libvirt/net-start.py $cloud-admin || exit $?
-    python ${mkcloud_lib_dir}/libvirt/vm-start.py $cloud-admin || exit $?
+    ${mkcloud_lib_dir}/libvirt/net-start $cloud-admin || exit $?


use full path to xml file here as well

jdsn · 2015-05-22T14:30:12Z

👍 but waiting for results of final test run

Module Fuctions: * helper methods * VM + Network XML file creation methods * VM + Network define/start methods * cleanup method Templates: * XML Templates for VM's, Admin Network, CPU-Flags Unittests and fixtures: * helper methods * VM + Network XML file creation methods Fixes SUSE-Cloud#345

* remove .py extension and call it without python * use libvirt_setup module for the business logic

…config

dguitarbite · 2015-05-26T11:08:24Z

Makefile


 suseinstall:
-	sudo zypper install perl-JSON-XS perl-libxml-perl
+	sudo zypper install perl-JSON-XS perl-libxml-perl libvirt-python

 genericinstall:
 	sudo pip install bashate flake8


You may also install libvirt-python in the generic install via. pip

$ pip install libvirt-python

Actually I find this ugly. There are installations via zypper and others via pip as root. I would never install anything with pip as root in my system. I would suggest two options here:

Because python-lake8 is in Factory, so I would submit python-bashate from Cloud:OpenStack:Master for Factory too.

and/or use virtualenv to install all the python dependencies that are not in any repository.

dguitarbite · 2015-05-26T11:34:42Z

I would suggest making the code more modular as Adam did. I would like to note that keeping the test cases in the same folder is a bad idea. May be after splitting mkcloud into multiple repositories, the test cases can lie in their special tests/ folder (but suggest to do it sooner than later).

Some hard coding in the XML files, and some other minor issues which may be carried out in following patches. Looks good to me. +1

Create libvirt xml files from templates with python

The <features> was missing from the VM's libvirt config on AMD hosts, and this was causing a lack of ACPI support, which in turn meant that virsh shutdown etc. didn't work, and hence that mkcloud didn't work properly. Some history: A long time ago, 5696c16 originally introduced some logic so that if the 'npt' (AMD Nested Page Tables) flag was present in /proc/cpuinfo on the host, the <cpu> section would not be included. Later, PR SUSE-Cloud#343 switched libvirt VM creation code from bash to Python with templates, but at this point <features> was always included. Much more recently, 7fe19ff from PR SUSE-Cloud#1049 moved the <features> section into the CPU templates, resulting in it missing when the AMD npt flag was present. 668a415 then added support for other architectures, but still left AMD broken.

MaximilianMeister changed the title ~~[dont merge yet] Create libvirt xml files from templates with python~~ [WIP][dont merge yet] Create libvirt xml files from templates with python May 13, 2015

MaximilianMeister force-pushed the python-libvirt-xml branch from 96cad47 to daf1cf1 Compare May 13, 2015 08:37

aplanas reviewed May 13, 2015
View reviewed changes

MaximilianMeister force-pushed the python-libvirt-xml branch from daf1cf1 to cd1e5d1 Compare May 13, 2015 08:51

aplanas reviewed May 13, 2015
View reviewed changes

MaximilianMeister force-pushed the python-libvirt-xml branch from cd1e5d1 to 72738b5 Compare May 13, 2015 08:56

aplanas reviewed May 13, 2015
View reviewed changes

MaximilianMeister force-pushed the python-libvirt-xml branch from 72738b5 to 854d209 Compare May 13, 2015 09:05

aspiers reviewed May 13, 2015
View reviewed changes

MaximilianMeister force-pushed the python-libvirt-xml branch 2 times, most recently from eab4112 to d313b98 Compare May 13, 2015 09:35

MaximilianMeister force-pushed the python-libvirt-xml branch from 0d91e4d to 0032245 Compare May 13, 2015 12:01

MaximilianMeister force-pushed the python-libvirt-xml branch 2 times, most recently from b4927d4 to c0f91f5 Compare May 13, 2015 14:32

aspiers reviewed May 13, 2015
View reviewed changes

bmwiedemann reviewed May 21, 2015
View reviewed changes

scripts/lib/libvirt/libvirt_setup.py

targetbus = "virtio"

else:

nicmodel = "virtio"

targetdev = "sda"

Copy link

Member

bmwiedemann May 21, 2015

Choose a reason for hiding this comment

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

should cover all vd* sd*

MaximilianMeister force-pushed the python-libvirt-xml branch 4 times, most recently from 591ea52 to c9d91ec Compare May 22, 2015 08:48

jdsn reviewed May 22, 2015
View reviewed changes

MaximilianMeister force-pushed the python-libvirt-xml branch 3 times, most recently from 3a18f72 to 3f5e763 Compare May 22, 2015 14:08

MaximilianMeister and others added 10 commits May 26, 2015 09:32

libvirt_cleanup: CLI changes

96b9fad

* remove .py extension and call it without python * use libvirt_setup module for the business logic

libvirt_vm_start: CLI changes

8f69de9

* remove .py extension and call it without python * use libvirt_setup module for the business logic

libvirt_net_start: CLI changes

03d9cca

* remove .py extension and call it without python * use libvirt_setup module for the business logic

libvirt_net_config: use python CLI to create the network XML config

29b4756

libvirt_admin_config: use python CLI to create the adminnode XML config

d4d9b93

libvirt_compute_config: use python CLI to create the computenode XML …

955284e

…config

travis: enable python unittests

a4a341c

libvirt: fix xml indentation

24d82f8

libvirt: move ceph and drbd volume handling to libvirt

425b50e

MaximilianMeister force-pushed the python-libvirt-xml branch from 3f5e763 to 425b50e Compare May 26, 2015 07:42

dguitarbite reviewed May 26, 2015
View reviewed changes

jdsn added a commit that referenced this pull request May 26, 2015

Merge pull request #343 from MaximilianMeister/python-libvirt-xml

e2f0f0d

Create libvirt xml files from templates with python

jdsn merged commit e2f0f0d into SUSE-Cloud:master May 26, 2015

MaximilianMeister deleted the python-libvirt-xml branch May 27, 2015 08:48

aspiers mentioned this pull request Aug 10, 2016

fix cpuflags (and hence ACPI / virsh shutdown) on AMD #1132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create libvirt xml files from templates with python #343

Create libvirt xml files from templates with python #343

MaximilianMeister commented May 13, 2015

aplanas May 13, 2015

aplanas May 13, 2015

aspiers May 13, 2015

aplanas May 13, 2015

aspiers May 13, 2015

aspiers May 13, 2015

aspiers May 13, 2015

aspiers commented May 13, 2015

MaximilianMeister commented May 13, 2015

MaximilianMeister commented May 13, 2015

aplanas commented May 13, 2015

jdsn commented May 13, 2015

aplanas commented May 13, 2015

MaximilianMeister commented May 13, 2015

aspiers commented May 13, 2015

aspiers May 13, 2015

bmwiedemann May 21, 2015

jdsn May 22, 2015

jdsn commented May 22, 2015

dguitarbite May 26, 2015

aplanas May 26, 2015

dguitarbite commented May 26, 2015

Create libvirt xml files from templates with python #343

Create libvirt xml files from templates with python #343

Conversation

MaximilianMeister commented May 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aspiers commented May 13, 2015

MaximilianMeister commented May 13, 2015

MaximilianMeister commented May 13, 2015

aplanas commented May 13, 2015

jdsn commented May 13, 2015

aplanas commented May 13, 2015

MaximilianMeister commented May 13, 2015

aspiers commented May 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdsn commented May 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dguitarbite commented May 26, 2015