[libvirt] [sandbox 00/11] Adding a virt-builder source

Hi all, Here are a few patches to add a virt-builder source to virt-sandbox-image. There are also patches fixing a few issues here and there. Cédric Bosdonnat (11): Fix memory leak virt-sandbox-image: fix error string formatting. Make virt-sandbox-image executable virt-sandbox-image: remove undefined default_disk_format virt-sandbox-image: add a source post_run hook virt-sandbox-image: move DockerSource _format_disk to Source virt-sandbox-image: smarter source name computing virt-sandbox-image: add a virt-builder source virt-sandbox-image: automatically call download and create if needed virt-sandbox-image: fix exception catching virt-sandbox-image: add error handling for sources import bin/virt-sandbox-image | 0 libvirt-sandbox.spec.in | 1 + libvirt-sandbox/image/cli.py | 154 +++++++++++---------- libvirt-sandbox/image/sources/DockerSource.py | 32 +++-- libvirt-sandbox/image/sources/Makefile.am | 1 + libvirt-sandbox/image/sources/Source.py | 50 +++++++ libvirt-sandbox/image/sources/VirtBuilderSource.py | 143 +++++++++++++++++++ libvirt-sandbox/image/template.py | 24 +++- libvirt-sandbox/libvirt-sandbox-builder.c | 1 + 9 files changed, 311 insertions(+), 95 deletions(-) mode change 100644 => 100755 bin/virt-sandbox-image create mode 100644 libvirt-sandbox/image/sources/VirtBuilderSource.py -- 2.1.4

--- libvirt-sandbox/libvirt-sandbox-builder.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libvirt-sandbox/libvirt-sandbox-builder.c b/libvirt-sandbox/libvirt-sandbox-builder.c index ea7d064..cbfa780 100644 --- a/libvirt-sandbox/libvirt-sandbox-builder.c +++ b/libvirt-sandbox/libvirt-sandbox-builder.c @@ -759,6 +759,7 @@ gboolean gvir_sandbox_builder_clean_post_stop(GVirSandboxBuilder *builder, g_object_unref(enumerator); g_object_unref(libsFile); g_free(libsdir); + g_free(dskfile); return ret; } -- 2.1.4

On Wed, Sep 23, 2015 at 09:53:31AM +0200, Cédric Bosdonnat wrote:
--- libvirt-sandbox/libvirt-sandbox-builder.c | 1 + 1 file changed, 1 insertion(+)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

--- libvirt-sandbox/image/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libvirt-sandbox/image/cli.py b/libvirt-sandbox/image/cli.py index 776b71f..75daf72 100755 --- a/libvirt-sandbox/image/cli.py +++ b/libvirt-sandbox/image/cli.py @@ -80,7 +80,7 @@ def delete(args): source.delete_template(template=tmpl, templatedir=args.template_dir) except Exception,e: - print "Delete Error %s", str(e) + print "Delete Error %s" % str(e) def create(args): try: -- 2.1.4

On Wed, Sep 23, 2015 at 09:53:32AM +0200, Cédric Bosdonnat wrote:
--- libvirt-sandbox/image/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

--- bin/virt-sandbox-image | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 bin/virt-sandbox-image diff --git a/bin/virt-sandbox-image b/bin/virt-sandbox-image old mode 100644 new mode 100755 -- 2.1.4

On Wed, Sep 23, 2015 at 09:53:33AM +0200, Cédric Bosdonnat wrote:
--- bin/virt-sandbox-image | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 bin/virt-sandbox-image
diff --git a/bin/virt-sandbox-image b/bin/virt-sandbox-image old mode 100644 new mode 100755
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

--- libvirt-sandbox/image/sources/DockerSource.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libvirt-sandbox/image/sources/DockerSource.py b/libvirt-sandbox/image/sources/DockerSource.py index ab0b765..6c02cc3 100644 --- a/libvirt-sandbox/image/sources/DockerSource.py +++ b/libvirt-sandbox/image/sources/DockerSource.py @@ -243,7 +243,7 @@ class DockerSource(Source): def create_template(self, template, templatedir, connect=None, format=None): if format is None: - format = self.default_disk_format + format = "qcow2" self._check_disk_format(format) imagelist = self._get_image_list(template, templatedir) imagelist.reverse() -- 2.1.4

On Wed, Sep 23, 2015 at 09:53:34AM +0200, Cédric Bosdonnat wrote:
--- libvirt-sandbox/image/sources/DockerSource.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libvirt-sandbox/image/sources/DockerSource.py b/libvirt-sandbox/image/sources/DockerSource.py index ab0b765..6c02cc3 100644 --- a/libvirt-sandbox/image/sources/DockerSource.py +++ b/libvirt-sandbox/image/sources/DockerSource.py @@ -243,7 +243,7 @@ class DockerSource(Source):
def create_template(self, template, templatedir, connect=None, format=None): if format is None: - format = self.default_disk_format + format = "qcow2" self._check_disk_format(format) imagelist = self._get_image_list(template, templatedir) imagelist.reverse()
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

This hook in the source allows additional cleanup to take place once the run command is terminated. This is not used by the docker source, but will be used by the virt-builder one --- libvirt-sandbox/image/cli.py | 1 + libvirt-sandbox/image/sources/Source.py | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/libvirt-sandbox/image/cli.py b/libvirt-sandbox/image/cli.py index 75daf72..e991e2d 100755 --- a/libvirt-sandbox/image/cli.py +++ b/libvirt-sandbox/image/cli.py @@ -142,6 +142,7 @@ def run(args): cmd = cmd + params + ['--'] + commandToRun subprocess.call(cmd) os.unlink(diskfile) + source.post_run(tmpl, args.template_dir, name) except Exception,e: print "Run Error %s" % str(e) diff --git a/libvirt-sandbox/image/sources/Source.py b/libvirt-sandbox/image/sources/Source.py index 33feb7c..20f4af0 100644 --- a/libvirt-sandbox/image/sources/Source.py +++ b/libvirt-sandbox/image/sources/Source.py @@ -102,3 +102,15 @@ class Source(): Get the dict of environment variables to set """ pass + + def post_run(self, template, templatedir, imagename): + """ + :param template: libvirt_sandbox.template.Template object + :param templatedir: local directory path in which to find template + :param imagename: name of the image that just stopped running + + Hook called after the image has been stopped. By default is doesn't + do anything, subclasses can override this to do some additional + cleanup. + """ + pass -- 2.1.4

On Wed, Sep 23, 2015 at 09:53:35AM +0200, Cédric Bosdonnat wrote:
This hook in the source allows additional cleanup to take place once the run command is terminated. This is not used by the docker source, but will be used by the virt-builder one --- libvirt-sandbox/image/cli.py | 1 + libvirt-sandbox/image/sources/Source.py | 12 ++++++++++++ 2 files changed, 13 insertions(+)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Formatting a disk is a generic operation that will be needed by other sources, at least a virt-builder one. --- libvirt-sandbox/image/sources/DockerSource.py | 14 +------------- libvirt-sandbox/image/sources/Source.py | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/libvirt-sandbox/image/sources/DockerSource.py b/libvirt-sandbox/image/sources/DockerSource.py index 6c02cc3..41df7a7 100644 --- a/libvirt-sandbox/image/sources/DockerSource.py +++ b/libvirt-sandbox/image/sources/DockerSource.py @@ -261,7 +261,7 @@ class DockerSource(Source): subprocess.call(cmd) if parentImage is None: - self._format_disk(templateImage,format,connect) + self.format_disk(templateImage,format,connect) self._extract_tarballs(templatedir + "/" + imagetagid + "/template.",format,connect) parentImage = templateImage @@ -299,18 +299,6 @@ class DockerSource(Source): imagetagid = parent return imagelist - def _format_disk(self,disk,format,connect): - cmd = ['virt-sandbox'] - if connect is not None: - cmd.append("-c") - cmd.append(connect) - cmd.append("-p") - params = ['--disk=file:disk_image=%s,format=%s' %(disk,format), - '/sbin/mkfs.ext3', - '/dev/disk/by-tag/disk_image'] - cmd = cmd + params - subprocess.call(cmd) - def _extract_tarballs(self,directory,format,connect): tarfile = directory + "tar.gz" diskfile = directory + "qcow2" diff --git a/libvirt-sandbox/image/sources/Source.py b/libvirt-sandbox/image/sources/Source.py index 20f4af0..444baa3 100644 --- a/libvirt-sandbox/image/sources/Source.py +++ b/libvirt-sandbox/image/sources/Source.py @@ -21,6 +21,7 @@ # Author: Eren Yagdiran <erenyagdiran@gmail.com> from abc import ABCMeta, abstractmethod +import subprocess class Source(): '''The Source class defines the base interface for @@ -114,3 +115,18 @@ class Source(): cleanup. """ pass + + + # Utility functions to share between the sources. + + def format_disk(self,disk,format,connect): + cmd = ['virt-sandbox'] + if connect is not None: + cmd.append("-c") + cmd.append(connect) + cmd.append("-p") + params = ['--disk=file:disk_image=%s,format=%s' %(disk,format), + '/sbin/mkfs.ext3', + '/dev/disk/by-tag/disk_image'] + cmd = cmd + params + subprocess.call(cmd) -- 2.1.4

On Wed, Sep 23, 2015 at 09:53:36AM +0200, Cédric Bosdonnat wrote:
Formatting a disk is a generic operation that will be needed by other sources, at least a virt-builder one. --- libvirt-sandbox/image/sources/DockerSource.py | 14 +------------- libvirt-sandbox/image/sources/Source.py | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 13 deletions(-)
ACK
diff --git a/libvirt-sandbox/image/sources/Source.py b/libvirt-sandbox/image/sources/Source.py index 20f4af0..444baa3 100644 --- a/libvirt-sandbox/image/sources/Source.py +++ b/libvirt-sandbox/image/sources/Source.py @@ -21,6 +21,7 @@ # Author: Eren Yagdiran <erenyagdiran@gmail.com>
from abc import ABCMeta, abstractmethod +import subprocess
class Source(): '''The Source class defines the base interface for @@ -114,3 +115,18 @@ class Source(): cleanup. """ pass + + + # Utility functions to share between the sources. + + def format_disk(self,disk,format,connect): + cmd = ['virt-sandbox'] + if connect is not None: + cmd.append("-c") + cmd.append(connect) + cmd.append("-p") + params = ['--disk=file:disk_image=%s,format=%s' %(disk,format), + '/sbin/mkfs.ext3',
This reminds me we should switch to mkfs.ext4 to be consistent with our previous switch to ext4 mount by default.
+ '/dev/disk/by-tag/disk_image'] + cmd = cmd + params + subprocess.call(cmd)
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

To compute the source class name from the URI scheme, we need to be a bit smarter to have a nice scheme for sources like the virt-builder one. In order to have a scheme like virt-builder:// and a class name like VirtBuilderSource, strip the non-word characters from the scheme and camel case the words. --- libvirt-sandbox/image/template.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/libvirt-sandbox/image/template.py b/libvirt-sandbox/image/template.py index 17da6c0..4713b0a 100644 --- a/libvirt-sandbox/image/template.py +++ b/libvirt-sandbox/image/template.py @@ -21,6 +21,7 @@ import urlparse import importlib +import re class Template(object): @@ -56,10 +57,13 @@ class Template(object): self.params = {} def get_source_impl(self): + p = re.compile("\W") + sourcename = "".join([i.capitalize() for i in p.split(self.source)]) + mod = importlib.import_module( "libvirt_sandbox.image.sources." + - self.source.capitalize() + "Source") - classname = self.source.capitalize() + "Source" + sourcename + "Source") + classname = sourcename + "Source" classimpl = getattr(mod, classname) return classimpl() -- 2.1.4

On Wed, Sep 23, 2015 at 09:53:37AM +0200, Cédric Bosdonnat wrote:
To compute the source class name from the URI scheme, we need to be a bit smarter to have a nice scheme for sources like the virt-builder one. In order to have a scheme like virt-builder:// and a class name like VirtBuilderSource, strip the non-word characters from the scheme and camel case the words. --- libvirt-sandbox/image/template.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Allow virt-sandbox-image to pull templates from virt-builder and run sandboxes on top of them. --- libvirt-sandbox.spec.in | 1 + libvirt-sandbox/image/cli.py | 1 + libvirt-sandbox/image/sources/Makefile.am | 1 + libvirt-sandbox/image/sources/VirtBuilderSource.py | 136 +++++++++++++++++++++ libvirt-sandbox/image/template.py | 2 + 5 files changed, 141 insertions(+) create mode 100644 libvirt-sandbox/image/sources/VirtBuilderSource.py diff --git a/libvirt-sandbox.spec.in b/libvirt-sandbox.spec.in index 54fde55..f84eabc 100644 --- a/libvirt-sandbox.spec.in +++ b/libvirt-sandbox.spec.in @@ -36,6 +36,7 @@ Requires: systemd >= 198 Requires: pygobject3-base Requires: libselinux-python Requires: %{name}-libs = %{version}-%{release} +Requires: %{_bindir}/virt-builder %package libs Group: Development/Libraries diff --git a/libvirt-sandbox/image/cli.py b/libvirt-sandbox/image/cli.py index e991e2d..fb1104a 100755 --- a/libvirt-sandbox/image/cli.py +++ b/libvirt-sandbox/image/cli.py @@ -188,6 +188,7 @@ Example supported URI formats: docker:///ubuntu?tag=15.04 docker://username:password@index.docker.io/private/image docker://registry.access.redhat.com/rhel6 + virt-builder:///fedora-20 """) return parser diff --git a/libvirt-sandbox/image/sources/Makefile.am b/libvirt-sandbox/image/sources/Makefile.am index 069557d..52e9a7e 100644 --- a/libvirt-sandbox/image/sources/Makefile.am +++ b/libvirt-sandbox/image/sources/Makefile.am @@ -4,6 +4,7 @@ pythonimage_DATA = \ __init__.py \ Source.py \ DockerSource.py \ + VirtBuilderSource.py \ $(NULL) EXTRA_DIST = $(pythonimage_DATA) diff --git a/libvirt-sandbox/image/sources/VirtBuilderSource.py b/libvirt-sandbox/image/sources/VirtBuilderSource.py new file mode 100644 index 0000000..4a7e383 --- /dev/null +++ b/libvirt-sandbox/image/sources/VirtBuilderSource.py @@ -0,0 +1,136 @@ +#!/usr/bin/python +# +# Copyright (C) 2015 SUSE LLC +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +# +# Author: Cedric Bosdonnat <cbosdonnat@suse.com> +# + +from Source import Source +import os +import os.path +import subprocess + +class VirtBuilderSource(Source): + + def _get_template_name(self, template): + # We shouldn't have '/' in the names, but let's make sure + # nobody can try to alter the folders structure later. + return template.path[1:].replace('/', '_') + + def download_template(self, template, templatedir): + # We don't do anything here: virt-builder will do it for us in + # the create action + pass + + def _check_disk_format(self,format): + supportedFormats = ['qcow2', 'raw'] + if not format in supportedFormats: + raise ValueError(["Unsupported image format %s" % format]) + + def create_template(self, template, templatedir, + connect=None, format=None): + # FIXME Force qcow2 format: do we really want people to mess with that? + if not os.path.exists(templatedir): + os.makedirs(templatedir) + + # Get the image using virt-builder + templatename = self._get_template_name(template) + imagepath_original = "%s/%s-original.qcow2" % (templatedir, templatename) + imagepath = "%s/%s.%s" % (templatedir, templatename, format) + cmd = ["virt-builder", templatename, + "-o", imagepath_original, "--format", "qcow2"] + subprocess.call(cmd) + + # We need to convert this image into a single partition one. + tarfile = "%s/%s.tar" % (templatedir, templatename) + cmd = ["virt-tar-out", "-a", imagepath_original, "/", tarfile] + subprocess.call(cmd) + + os.unlink(imagepath_original) + + cmd = ["qemu-img", "create", "-q", "-f", format, imagepath, "10G"] + subprocess.call(cmd) + + self.format_disk(imagepath, format, connect) + self._extract_tarball(imagepath, format, tarfile, connect) + os.unlink(tarfile) + + + def delete_template(self, template, templatedir): + # Check for running sandboxes using this template before killing it + images = self._get_template_uses(template, templatedir) + if len(images) != 0: + imagesStr = ", ".join(images) + raise ValueError(["Images still using the template: %s" % imagesStr]) + + os.unlink("%s/%s.qcow2" % (templatedir, self._get_template_name(template))) + os.unlink("%s/%s.uses" % (templatedir, self._get_template_name(template))) + + def _get_template_uses(self, template, templatedir): + uses = open("%s/%s.uses" % (templatedir, self._get_template_name(template)), "r") + lines = uses.read() + uses.close() + return [l for l in lines.split("\n") if l != ""] + + def post_run(self, template, templatedir, imagename): + images = self._get_template_uses(template, templatedir) + images.remove(imagename) + + uses = open("%s/%s.uses" % (templatedir, self._get_template_name(template)), "w") + uses.write("\n".join(images)) + uses.close() + + def get_command(self, template, templatedir, userargs): + return userargs + + def get_disk(self,template, templatedir, imagedir, sandboxname): + diskfile = "%s/%s.qcow2" % (templatedir, self._get_template_name(template)) + tempfile = imagedir + "/" + sandboxname + ".qcow2" + if not os.path.exists(imagedir): + os.makedirs(imagedir) + cmd = ["qemu-img", "create", "-q", + "-f", "qcow2", + "-o", "backing_fmt=qcow2,backing_file=%s" % diskfile, + tempfile] + subprocess.call(cmd) + + # Add the image to the uses files + uses = open("%s/%s.uses" % (templatedir, + self._get_template_name(template)), "a") + uses.write("%s\n" % sandboxname) + uses.close() + return tempfile + + def get_env(self,template, templatedir): + return [] + + def _extract_tarball(self, diskfile, format, tarfile, connect): + cmd = ['virt-sandbox'] + if connect is not None: + cmd.append("-c") + cmd.append(connect) + cmd.append("-p") + params = ['-m', + 'host-image:/mnt=%s,format=%s' % (diskfile, format), + '--', + '/bin/tar', + 'xf', + '%s' % tarfile, + '-C', + '/mnt'] + cmd = cmd + params + subprocess.call(cmd) diff --git a/libvirt-sandbox/image/template.py b/libvirt-sandbox/image/template.py index 4713b0a..efdd845 100644 --- a/libvirt-sandbox/image/template.py +++ b/libvirt-sandbox/image/template.py @@ -43,6 +43,8 @@ class Template(object): docker:///ubuntu docker+https://index.docker.io/ubuntu?tag=latest + + virt-builder:///fedora-20 """ self.source = source -- 2.1.4

On Wed, Sep 23, 2015 at 09:53:38AM +0200, Cédric Bosdonnat wrote:
Allow virt-sandbox-image to pull templates from virt-builder and run sandboxes on top of them. --- libvirt-sandbox.spec.in | 1 + libvirt-sandbox/image/cli.py | 1 + libvirt-sandbox/image/sources/Makefile.am | 1 + libvirt-sandbox/image/sources/VirtBuilderSource.py | 136 +++++++++++++++++++++ libvirt-sandbox/image/template.py | 2 + 5 files changed, 141 insertions(+) create mode 100644 libvirt-sandbox/image/sources/VirtBuilderSource.py
+class VirtBuilderSource(Source): + + def _get_template_name(self, template): + # We shouldn't have '/' in the names, but let's make sure + # nobody can try to alter the folders structure later. + return template.path[1:].replace('/', '_') + + def download_template(self, template, templatedir): + # We don't do anything here: virt-builder will do it for us in + # the create action + pass
I guess I could see us invoking virt-builder in the download step to pull down the image and format it to qcow2.
+ + def _check_disk_format(self,format): + supportedFormats = ['qcow2', 'raw'] + if not format in supportedFormats: + raise ValueError(["Unsupported image format %s" % format]) + + def create_template(self, template, templatedir, + connect=None, format=None): + # FIXME Force qcow2 format: do we really want people to mess with that? + if not os.path.exists(templatedir): + os.makedirs(templatedir) + + # Get the image using virt-builder + templatename = self._get_template_name(template) + imagepath_original = "%s/%s-original.qcow2" % (templatedir, templatename) + imagepath = "%s/%s.%s" % (templatedir, templatename, format) + cmd = ["virt-builder", templatename, + "-o", imagepath_original, "--format", "qcow2"] + subprocess.call(cmd)
eg, this bit could live in download, and then the following bit stay in create. Honestly the distinction is kind of blury though. I wonder if this is a sign that we should kill off the distinction between download + create. The key idea behind having the separate commands is that users may want to "prime the cache" for images they expect to need to run in the future, so saving time later. This doesn't require us to have download+create separate though - we could just as easily have a single "prepare" command that combines them both.
+ + # We need to convert this image into a single partition one. + tarfile = "%s/%s.tar" % (templatedir, templatename) + cmd = ["virt-tar-out", "-a", imagepath_original, "/", tarfile] + subprocess.call(cmd) + + os.unlink(imagepath_original) + + cmd = ["qemu-img", "create", "-q", "-f", format, imagepath, "10G"] + subprocess.call(cmd) + + self.format_disk(imagepath, format, connect) + self._extract_tarball(imagepath, format, tarfile, connect) + os.unlink(tarfile) + + + def delete_template(self, template, templatedir): + # Check for running sandboxes using this template before killing it + images = self._get_template_uses(template, templatedir) + if len(images) != 0: + imagesStr = ", ".join(images) + raise ValueError(["Images still using the template: %s" % imagesStr]) + + os.unlink("%s/%s.qcow2" % (templatedir, self._get_template_name(template))) + os.unlink("%s/%s.uses" % (templatedir, self._get_template_name(template))) + + def _get_template_uses(self, template, templatedir): + uses = open("%s/%s.uses" % (templatedir, self._get_template_name(template)), "r") + lines = uses.read() + uses.close() + return [l for l in lines.split("\n") if l != ""] + + def post_run(self, template, templatedir, imagename): + images = self._get_template_uses(template, templatedir) + images.remove(imagename) + + uses = open("%s/%s.uses" % (templatedir, self._get_template_name(template)), "w") + uses.write("\n".join(images)) + uses.close() + + def get_command(self, template, templatedir, userargs): + return userargs + + def get_disk(self,template, templatedir, imagedir, sandboxname): + diskfile = "%s/%s.qcow2" % (templatedir, self._get_template_name(template)) + tempfile = imagedir + "/" + sandboxname + ".qcow2" + if not os.path.exists(imagedir): + os.makedirs(imagedir) + cmd = ["qemu-img", "create", "-q", + "-f", "qcow2", + "-o", "backing_fmt=qcow2,backing_file=%s" % diskfile, + tempfile] + subprocess.call(cmd) + + # Add the image to the uses files + uses = open("%s/%s.uses" % (templatedir, + self._get_template_name(template)), "a") + uses.write("%s\n" % sandboxname) + uses.close()
Ah interest, so with the DockerSource we don't actually currently do anything to track usage in this way. We only track usage wrt to the template layers - not running VM instances. Technically I don't think we actually need to track usage from running VMs as you illustrate. If a VM is running with an template, then QEMU/ qemu-nbd will hold an open file descriptor for the backing file it is usage. So even if we delete the image from the cache, it won't harm running VMs thanks to POSIX close/unlink semantics.
+ return tempfile + + def get_env(self,template, templatedir): + return [] + + def _extract_tarball(self, diskfile, format, tarfile, connect): + cmd = ['virt-sandbox'] + if connect is not None: + cmd.append("-c") + cmd.append(connect) + cmd.append("-p") + params = ['-m', + 'host-image:/mnt=%s,format=%s' % (diskfile, format), + '--', + '/bin/tar', + 'xf', + '%s' % tarfile, + '-C', + '/mnt'] + cmd = cmd + params + subprocess.call(cmd)
Seems we can share this method with the DockerSource too, Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

To provide a smooth user experience, run automatically calls create if needed and create automatically calls download if needed. --- libvirt-sandbox/image/cli.py | 18 +++++++++++++++--- libvirt-sandbox/image/sources/DockerSource.py | 16 ++++++++++++++++ libvirt-sandbox/image/sources/Source.py | 22 ++++++++++++++++++++++ libvirt-sandbox/image/sources/VirtBuilderSource.py | 7 +++++++ 4 files changed, 60 insertions(+), 3 deletions(-) diff --git a/libvirt-sandbox/image/cli.py b/libvirt-sandbox/image/cli.py index fb1104a..f4472d9 100755 --- a/libvirt-sandbox/image/cli.py +++ b/libvirt-sandbox/image/cli.py @@ -42,6 +42,7 @@ if os.geteuid() == 0: else: default_template_dir = os.environ['HOME'] + "/.local/share/libvirt/templates" default_image_dir = os.environ['HOME'] + "/.local/share/libvirt/images" +default_format = "qcow2" debug = False verbose = False @@ -86,10 +87,18 @@ def create(args): try: tmpl = template.Template.from_uri(args.template) source = tmpl.get_source_impl() + + if not source.was_downloaded(tmpl, args.template_dir): + download(args) + + fmt = default_format + if "format" in vars(args): + fmt = args.format + source.create_template(template=tmpl, templatedir=args.template_dir, connect=args.connect, - format=args.format) + format=fmt) except Exception,e: print "Create Error %s" % str(e) @@ -97,10 +106,13 @@ def run(args): try: if args.connect is not None: check_connect(args.connect) - tmpl = template.Template.from_uri(args.template) source = tmpl.get_source_impl() + # Create the template image if needed + if not source.has_template(tmpl, args.template_dir): + create(args) + name = args.name if name is None: randomid = ''.join(random.choice(string.lowercase) for i in range(10)) @@ -213,7 +225,7 @@ def gen_create_args(subparser): requires_connect(parser) requires_template_dir(parser) parser.add_argument("-f","--format", - default="qcow2", + default=default_format, help=_("format format for image")) parser.set_defaults(func=create) diff --git a/libvirt-sandbox/image/sources/DockerSource.py b/libvirt-sandbox/image/sources/DockerSource.py index 41df7a7..be9063d 100644 --- a/libvirt-sandbox/image/sources/DockerSource.py +++ b/libvirt-sandbox/image/sources/DockerSource.py @@ -59,6 +59,22 @@ class DockerSource(Source): if (major == 2 and sys.hexversion < py2_7_9_hexversion) or (major == 3 and sys.hexversion < py3_4_3_hexversion): sys.stderr.write(SSL_WARNING) + def was_downloaded(self, template, templatedir): + try: + self._get_image_list(template, templatedir) + return True + except Exception: + return False + + + def has_template(self, template, templatedir): + try: + configfile, diskfile = self._get_template_data(template, templatedir) + return os.path.exists(diskfile) + except Exception: + return False + + def download_template(self, template, templatedir): self._check_cert_validate() diff --git a/libvirt-sandbox/image/sources/Source.py b/libvirt-sandbox/image/sources/Source.py index 444baa3..e647448 100644 --- a/libvirt-sandbox/image/sources/Source.py +++ b/libvirt-sandbox/image/sources/Source.py @@ -35,6 +35,28 @@ class Source(): pass @abstractmethod + def was_downloaded(self, template, templatedir): + """ + :param template: libvirt_sandbox.template.Template object + :param templatedir: local directory path in which to store the template + + Check if a template has already been downloaded. + """ + pass + + + @abstractmethod + def has_template(self, template, templatedir): + """ + :param template: libvirt_sandbox.template.Template object + :param templatedir: local directory path in which to store the template + + Check if a template has already been created. + """ + pass + + + @abstractmethod def download_template(self, template, templatedir): """ :param template: libvirt_sandbox.template.Template object diff --git a/libvirt-sandbox/image/sources/VirtBuilderSource.py b/libvirt-sandbox/image/sources/VirtBuilderSource.py index 4a7e383..2dde715 100644 --- a/libvirt-sandbox/image/sources/VirtBuilderSource.py +++ b/libvirt-sandbox/image/sources/VirtBuilderSource.py @@ -31,6 +31,13 @@ class VirtBuilderSource(Source): # nobody can try to alter the folders structure later. return template.path[1:].replace('/', '_') + def was_downloaded(self, template, templatedir): + return True + + def has_template(self, template, templatedir): + imagepath = "%s/%s.qcow2" % (templatedir, template.path) + return os.path.exists(imagepath) + def download_template(self, template, templatedir): # We don't do anything here: virt-builder will do it for us in # the create action -- 2.1.4

On Wed, Sep 23, 2015 at 09:53:39AM +0200, Cédric Bosdonnat wrote:
To provide a smooth user experience, run automatically calls create if needed and create automatically calls download if needed. --- libvirt-sandbox/image/cli.py | 18 +++++++++++++++--- libvirt-sandbox/image/sources/DockerSource.py | 16 ++++++++++++++++ libvirt-sandbox/image/sources/Source.py | 22 ++++++++++++++++++++++ libvirt-sandbox/image/sources/VirtBuilderSource.py | 7 +++++++ 4 files changed, 60 insertions(+), 3 deletions(-)
This makes sense, though see my previous suggestion about us merging downkoad+create into a single prepare step, which would simplify this even more.
diff --git a/libvirt-sandbox/image/cli.py b/libvirt-sandbox/image/cli.py index fb1104a..f4472d9 100755 --- a/libvirt-sandbox/image/cli.py +++ b/libvirt-sandbox/image/cli.py @@ -42,6 +42,7 @@ if os.geteuid() == 0: else: default_template_dir = os.environ['HOME'] + "/.local/share/libvirt/templates" default_image_dir = os.environ['HOME'] + "/.local/share/libvirt/images" +default_format = "qcow2"
debug = False verbose = False @@ -86,10 +87,18 @@ def create(args): try: tmpl = template.Template.from_uri(args.template) source = tmpl.get_source_impl() + + if not source.was_downloaded(tmpl, args.template_dir): + download(args) + + fmt = default_format + if "format" in vars(args): + fmt = args.format
Lets just kill format from the arguments for now and hardcode qcow2. We can re-think it later when we actually want to consider using different storage backends.
+ source.create_template(template=tmpl, templatedir=args.template_dir, connect=args.connect, - format=args.format) + format=fmt) except Exception,e: print "Create Error %s" % str(e)
@@ -97,10 +106,13 @@ def run(args): try: if args.connect is not None: check_connect(args.connect) - tmpl = template.Template.from_uri(args.template) source = tmpl.get_source_impl()
+ # Create the template image if needed + if not source.has_template(tmpl, args.template_dir): + create(args) + name = args.name if name is None: randomid = ''.join(random.choice(string.lowercase) for i in range(10)) @@ -213,7 +225,7 @@ def gen_create_args(subparser): requires_connect(parser) requires_template_dir(parser) parser.add_argument("-f","--format", - default="qcow2", + default=default_format, help=_("format format for image")) parser.set_defaults(func=create)
diff --git a/libvirt-sandbox/image/sources/DockerSource.py b/libvirt-sandbox/image/sources/DockerSource.py index 41df7a7..be9063d 100644 --- a/libvirt-sandbox/image/sources/DockerSource.py +++ b/libvirt-sandbox/image/sources/DockerSource.py @@ -59,6 +59,22 @@ class DockerSource(Source): if (major == 2 and sys.hexversion < py2_7_9_hexversion) or (major == 3 and sys.hexversion < py3_4_3_hexversion): sys.stderr.write(SSL_WARNING)
+ def was_downloaded(self, template, templatedir): + try: + self._get_image_list(template, templatedir) + return True + except Exception: + return False + + + def has_template(self, template, templatedir): + try: + configfile, diskfile = self._get_template_data(template, templatedir) + return os.path.exists(diskfile) + except Exception: + return False + + def download_template(self, template, templatedir): self._check_cert_validate()
diff --git a/libvirt-sandbox/image/sources/Source.py b/libvirt-sandbox/image/sources/Source.py index 444baa3..e647448 100644 --- a/libvirt-sandbox/image/sources/Source.py +++ b/libvirt-sandbox/image/sources/Source.py @@ -35,6 +35,28 @@ class Source(): pass
@abstractmethod + def was_downloaded(self, template, templatedir): + """ + :param template: libvirt_sandbox.template.Template object + :param templatedir: local directory path in which to store the template + + Check if a template has already been downloaded. + """ + pass + + + @abstractmethod + def has_template(self, template, templatedir): + """ + :param template: libvirt_sandbox.template.Template object + :param templatedir: local directory path in which to store the template + + Check if a template has already been created. + """ + pass + + + @abstractmethod def download_template(self, template, templatedir): """ :param template: libvirt_sandbox.template.Template object diff --git a/libvirt-sandbox/image/sources/VirtBuilderSource.py b/libvirt-sandbox/image/sources/VirtBuilderSource.py index 4a7e383..2dde715 100644 --- a/libvirt-sandbox/image/sources/VirtBuilderSource.py +++ b/libvirt-sandbox/image/sources/VirtBuilderSource.py @@ -31,6 +31,13 @@ class VirtBuilderSource(Source): # nobody can try to alter the folders structure later. return template.path[1:].replace('/', '_')
+ def was_downloaded(self, template, templatedir): + return True + + def has_template(self, template, templatedir): + imagepath = "%s/%s.qcow2" % (templatedir, template.path) + return os.path.exists(imagepath) + def download_template(self, template, templatedir): # We don't do anything here: virt-builder will do it for us in # the create action -- 2.1.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Catch the exception in the main to avoid problems when auto-calling another function that fails. For example if run calls create and that one fails, run would have ignored the error. --- libvirt-sandbox/image/cli.py | 156 ++++++++++++++++++++----------------------- 1 file changed, 73 insertions(+), 83 deletions(-) diff --git a/libvirt-sandbox/image/cli.py b/libvirt-sandbox/image/cli.py index f4472d9..eb6bbb4 100755 --- a/libvirt-sandbox/image/cli.py +++ b/libvirt-sandbox/image/cli.py @@ -66,98 +66,85 @@ def info(msg): sys.stdout.write(msg) def download(args): - try: - tmpl = template.Template.from_uri(args.template) - source = tmpl.get_source_impl() - source.download_template(template=tmpl, - templatedir=args.template_dir) - except Exception,e: - print "Download Error %s" % str(e) + tmpl = template.Template.from_uri(args.template) + source = tmpl.get_source_impl() + source.download_template(template=tmpl, + templatedir=args.template_dir) def delete(args): - try: - tmpl = template.Template.from_uri(args.template) - source = tmpl.get_source_impl() - source.delete_template(template=tmpl, - templatedir=args.template_dir) - except Exception,e: - print "Delete Error %s" % str(e) + tmpl = template.Template.from_uri(args.template) + source = tmpl.get_source_impl() + source.delete_template(template=tmpl, + templatedir=args.template_dir) def create(args): - try: - tmpl = template.Template.from_uri(args.template) - source = tmpl.get_source_impl() + tmpl = template.Template.from_uri(args.template) + source = tmpl.get_source_impl() - if not source.was_downloaded(tmpl, args.template_dir): - download(args) + if not source.was_downloaded(tmpl, args.template_dir): + download(args) - fmt = default_format - if "format" in vars(args): - fmt = args.format + fmt = default_format + if "format" in vars(args): + fmt = args.format - source.create_template(template=tmpl, - templatedir=args.template_dir, - connect=args.connect, - format=fmt) - except Exception,e: - print "Create Error %s" % str(e) + source.create_template(template=tmpl, + templatedir=args.template_dir, + connect=args.connect, + format=fmt) def run(args): - try: - if args.connect is not None: - check_connect(args.connect) - tmpl = template.Template.from_uri(args.template) - source = tmpl.get_source_impl() - - # Create the template image if needed - if not source.has_template(tmpl, args.template_dir): - create(args) - - name = args.name - if name is None: - randomid = ''.join(random.choice(string.lowercase) for i in range(10)) - name = tmpl.path[1:] + ":" + randomid - - diskfile = source.get_disk(template=tmpl, - templatedir=args.template_dir, - imagedir=args.image_dir, - sandboxname=name) - - format = "qcow2" - commandToRun = source.get_command(tmpl, args.template_dir, args.args) - if len(commandToRun) == 0: - commandToRun = ["/bin/sh"] - cmd = ['virt-sandbox', '--name', name] - if args.connect is not None: - cmd.append("-c") - cmd.append(args.connect) - params = ['-m','host-image:/=%s,format=%s' %(diskfile,format)] - - networkArgs = args.network - if networkArgs is not None: - params.append('-N') - params.append(networkArgs) - - allEnvs = source.get_env(tmpl, args.template_dir) - envArgs = args.env - if envArgs is not None: - allEnvs = allEnvs + envArgs - for env in allEnvs: - envsplit = env.split("=") - envlen = len(envsplit) - if envlen == 2: - params.append("--env") - params.append(env) - else: - pass - - cmd = cmd + params + ['--'] + commandToRun - subprocess.call(cmd) - os.unlink(diskfile) - source.post_run(tmpl, args.template_dir, name) - - except Exception,e: - print "Run Error %s" % str(e) + if args.connect is not None: + check_connect(args.connect) + tmpl = template.Template.from_uri(args.template) + source = tmpl.get_source_impl() + + # Create the template image if needed + if not source.has_template(tmpl, args.template_dir): + create(args) + + name = args.name + if name is None: + randomid = ''.join(random.choice(string.lowercase) for i in range(10)) + name = tmpl.path[1:] + ":" + randomid + + diskfile = source.get_disk(template=tmpl, + templatedir=args.template_dir, + imagedir=args.image_dir, + sandboxname=name) + + format = "qcow2" + commandToRun = source.get_command(tmpl, args.template_dir, args.args) + if len(commandToRun) == 0: + commandToRun = ["/bin/sh"] + cmd = ['virt-sandbox', '--name', name] + if args.connect is not None: + cmd.append("-c") + cmd.append(args.connect) + params = ['-m','host-image:/=%s,format=%s' %(diskfile,format)] + + networkArgs = args.network + if networkArgs is not None: + params.append('-N') + params.append(networkArgs) + + allEnvs = source.get_env(tmpl, args.template_dir) + envArgs = args.env + if envArgs is not None: + allEnvs = allEnvs + envArgs + for env in allEnvs: + envsplit = env.split("=") + envlen = len(envsplit) + if envlen == 2: + params.append("--env") + params.append(env) + else: + pass + + cmd = cmd + params + ['--'] + commandToRun + subprocess.call(cmd) + os.unlink(diskfile) + source.post_run(tmpl, args.template_dir, name) def requires_template(parser): parser.add_argument("template", @@ -276,3 +263,6 @@ def main(): sys.stderr.write("%s: %s\n" % (sys.argv[0], e)) sys.stderr.flush() sys.exit(1) + except Exception, e: + print e.message + sys.exit(1) -- 2.1.4

On Wed, Sep 23, 2015 at 09:53:40AM +0200, Cédric Bosdonnat wrote:
Catch the exception in the main to avoid problems when auto-calling another function that fails. For example if run calls create and that one fails, run would have ignored the error. --- libvirt-sandbox/image/cli.py | 156 ++++++++++++++++++++----------------------- 1 file changed, 73 insertions(+), 83 deletions(-)
ACK, this is much simpler and avoids repetition. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Rather than letting the python exception hit the user, catch them and provide a more meaningful message if no or a bad scheme is provided in the URI. --- libvirt-sandbox/image/template.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/libvirt-sandbox/image/template.py b/libvirt-sandbox/image/template.py index efdd845..58904a2 100644 --- a/libvirt-sandbox/image/template.py +++ b/libvirt-sandbox/image/template.py @@ -59,15 +59,21 @@ class Template(object): self.params = {} def get_source_impl(self): - p = re.compile("\W") - sourcename = "".join([i.capitalize() for i in p.split(self.source)]) - - mod = importlib.import_module( - "libvirt_sandbox.image.sources." + - sourcename + "Source") - classname = sourcename + "Source" - classimpl = getattr(mod, classname) - return classimpl() + if self.source == "": + raise Exception("Missing scheme in image URI") + + try: + p = re.compile("\W") + sourcename = "".join([i.capitalize() for i in p.split(self.source)]) + + mod = importlib.import_module( + "libvirt_sandbox.image.sources." + + sourcename + "Source") + classname = sourcename + "Source" + classimpl = getattr(mod, classname) + return classimpl() + except Exception: + raise Exception("Invalid source: '%s'" % self.source) def __repr__(self): if self.protocol is not None: -- 2.1.4

On Wed, Sep 23, 2015 at 09:53:41AM +0200, Cédric Bosdonnat wrote:
Rather than letting the python exception hit the user, catch them and provide a more meaningful message if no or a bad scheme is provided in the URI. --- libvirt-sandbox/image/template.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Cédric Bosdonnat
-
Daniel P. Berrange