7:44 a.m.
Repo: git://git.baserock.org/baserock/baserock/morph.git
Branch: baserock/pedroalvarez/S8631/openstack-configure-write
Sha1: ce8f5fb4aace8d8e6ca5c1fd4316df680b36d81d
Pedro Alvarez (1):
exts: Add openstack configure/write exts
morphlib/exts/openstackssh.write | 169 ++++++++++++++++++++++++++++++++++++++
morphlib/exts/vdaboot.configure | 34 ++++++++
2 files changed, 203 insertions(+)
create mode 100755 morphlib/exts/openstackssh.write
create mode 100755 morphlib/exts/vdaboot.configure
--
1.7.10.4
7:44 a.m.
New subject: [PATCH] exts: Add openstack configure/write exts
openstackssh.write: Write extension which deploy a raw image of baserock
directly to an OpenStack machine using ssh. It sends the image first,
and then deploy the image into the OpenStack system. The raw image
deployed has modified its bootloader to use virtio disks.
vdaboot.configure: Configuration extension to change the mount point of
"/" to use virtio disks (/dev/vda).
---
morphlib/exts/openstackssh.write | 169 ++++++++++++++++++++++++++++++++++++++
morphlib/exts/vdaboot.configure | 34 ++++++++
2 files changed, 203 insertions(+)
create mode 100755 morphlib/exts/openstackssh.write
create mode 100755 morphlib/exts/vdaboot.configure
diff --git a/morphlib/exts/openstackssh.write b/morphlib/exts/openstackssh.write
new file mode 100755
index 0000000..9e2a5ff
--- /dev/null
+++ b/morphlib/exts/openstackssh.write
@@ -0,0 +1,169 @@
+#!/usr/bin/python
+# Copyright (C) 2012-2013 Codethink Limited
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 2 of the License.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+
+'''A Morph deployment write extension for deploying to
OpenStack.'''
+
+
+import cliapp
+import os
+import re
+import sys
+import tempfile
+import urlparse
+
+import morphlib.writeexts
+
+
+class OpenStackWriteExtension(morphlib.writeexts.WriteExtension):
+
+ '''Create a raw disk image during Morph's deployment and upload it
+ to an OpenStack host creating the image on the system using Glance.
+
+ The location command line argument is the ssh path to upload the
+ image file into OpenStack using the following syntax:
+
+ ssh://[USER@]HOST/PATH/TO/FILE
+
+ where
+
+ * USER is optional, is the user to connect via ssh
+ * HOST is the host running OpenStack
+ * PATH/TO/FILE is the path to store the image file into the host.
+
+
+ This extension needs in the enviroment the following variables:
+
+ * OPENSTACK_USER is the username to use in the deployment.
+ * OPENSTACK_TENANT is the project name to use in the deployment.
+ * OPENSTACK_IMAGENAME is the name of the image to create.
+ *
+ *
+
+
+ The extension will connect to HOST via ssh to push the image and then
+ it will connect again via ssh to run OpenStack management tools.
+
+ '''
+
+ def process_args(self, args):
+ if len(args) != 2:
+ raise cliapp.AppException('Wrong number of command line args')
+
+ temp_root, location = args
+ ssh_host, ssh_path = self.parse_location(location)
+
+ self.check_openstack_requirements()
+
+ fd, raw_disk = tempfile.mkstemp()
+ os.close(fd)
+ self.create_local_system(temp_root, raw_disk)
+ self.status(msg='Temporal disk image has been created at %s' % raw_disk)
+
+ self.upgrade_extlinux(raw_disk)
+
+
+ self.upload_openstack(raw_disk, ssh_host, ssh_path)
+
+ os.remove(raw_disk)
+ self.status(msg='Temporal image has been deleted')
+
+ self.deploy_image_openstack(ssh_host, ssh_path)
+
+ def upgrade_extlinux(self, raw_disk):
+ mp = self.mount(raw_disk)
+ '''Updating extlinux to use virtio disks'''
+
+ self.status(msg='Updating extlinux.conf')
+
+ config = os.path.join(mp, 'extlinux.conf')
+ with open(config, 'w') as f:
+ f.write('default linux\n')
+ f.write('timeout 1\n')
+ f.write('label linux\n')
+ f.write('kernel /systems/default/kernel\n')
+ f.write('append root=/dev/vda '
+ 'rootflags=subvol=systems/default/run '
+ 'init=/sbin/int rw\n')
+
+# Working without this:
+# cliapp.runcmd(['extlinux', '--install', mp])
+# cliapp.runcmd(['sync'])
+# time.sleep(2)
+
+ self.unmount(mp)
+
+
+ def check_openstack_requirements(self):
+ os_user = os.environ.get('OPENSTACK_USER')
+ os_tenant = os.environ.get('OPENSTACK_TENANT')
+ os_imagename = os.environ.get('OPENSTACK_IMAGENAME')
+ os_password = os.environ.get('OPENSTACK_PASSWORD')
+ os_authurl = os.environ.get('OPENSTACK_AUTH_URL')
+
+ if os_user is None:
+ raise cliapp.AppException('OPENSTACK_USER was not given')
+ if os_tenant is None:
+ raise cliapp.AppException('OPENSTACK_TENANT was not given')
+ if os_imagename is None:
+ raise cliapp.AppException('OPENSTACK_IMAGENAME was not given')
+ if os_password is None:
+ raise cliapp.AppException('OPENSTACK_PASSWORD was not given')
+ if os_authurl is None:
+ raise cliapp.AppException('OPENSTACK_AUTH_URL was not given')
+
+ def upload_openstack(self, raw_disk, ssh_host, ssh_path):
+ self.status(msg='Uploading raw image to OpenStack via ssh...')
+ ssh_fullpath = ssh_host + ':' + ssh_path
+ cliapp.runcmd(['rsync', '-szS', raw_disk, ssh_fullpath ])
+ self.status(msg='Raw image uploaded.')
+
+ def parse_location(self, location):
+ x = urlparse.urlparse(location)
+ if x.scheme != 'ssh':
+ raise cliapp.AppException('URL schema must be ssh in %s' % location)
+ return x.netloc, x.path
+
+ def deploy_image_openstack(self, ssh_host, ssh_path):
+ self.status(msg='Creating image into OpenStack...')
+# os_user='demo'
+# os_tenant='demo'
+# os_imagename='sshimagenamewrite'
+# os_password='horizonpass'
+# os_authurl='http://localhost:5000/v2.0'
+
+ os_user = os.environ['OPENSTACK_USER']
+ os_tenant = os.environ['OPENSTACK_TENANT']
+ os_imagename = os.environ['OPENSTACK_IMAGENAME']
+ os_password = os.environ['OPENSTACK_PASSWORD']
+ os_authurl = os.environ['OPENSTACK_AUTH_URL']
+
+ cmdline = ['glance',
+ '--os-username', os_user,
+ '--os-tenant-name', os_tenant,
+ '--os-password', os_password,
+ '--os-auth-url', os_authurl,
+ 'image-create',
+ '--name=%s' % os_imagename,
+ '--disk-format=raw',
+ '--container-format', 'bare',
+ '--file', ssh_path]
+ cliapp.ssh_runcmd(ssh_host, cmdline)
+
+ self.status(msg='Image created.'
+ )
+OpenStackWriteExtension().run()
+
diff --git a/morphlib/exts/vdaboot.configure b/morphlib/exts/vdaboot.configure
new file mode 100755
index 0000000..a21269d
--- /dev/null
+++ b/morphlib/exts/vdaboot.configure
@@ -0,0 +1,34 @@
+#!/bin/sh
+# Copyright (C) 2013 Codethink Limited
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 2 of the License.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+
+# Change the "/" mount ponit to /dev/vda to use virtio disks.
+
+set -e
+
+if [ "$OPENSTACK_USER" ]
+then
+ # Modifying fstab
+ if [ -f "$1/etc/fstab" ]
+ then
+ mv "$1/etc/fstab" "$1/etc/fstab.old"
+ awk 'BEGIN {print "/dev/vda / btrfs defaults,rw,noatime 0 1"}; $2
!= "/" {print $0 };' "$1/etc/fstab.old" >
"$1/etc/fstab"
+ rm "$1/etc/fstab.old"
+ cp "$1/etc/fstab" "$1/etc/fstab.modified"
+ else
+ echo "/dev/vda / btrfs defaults,rw,noatime 0 1">
"$1/etc/fstab"
+ fi
+fi
--
1.7.10.4
8:42 a.m.
New subject: [PATCH] exts: Add openstack configure/write exts
On Fri, Aug 30, 2013 at 01:44:59PM +0100, Pedro Alvarez wrote:
My overall opinion of this commit is -0. I see no problems with the
code, but I have a lot to say about the formatting.
openstackssh.write: Write extension which deploy a raw image of
baserock
directly to an OpenStack machine using ssh. It sends the image first,
and then deploy the image into the OpenStack system. The raw image
deployed has modified its bootloader to use virtio disks.
vdaboot.configure: Configuration extension to change the mount point of
"/" to use virtio disks (/dev/vda).
Each commit should generally be for one thing, they don't work without each
other.
In this case, vdaboot.configure would be a separate commit to
openstackssh.write, although I can appreciate that fiddling with git to split
it into two commits can be tricky.
---
morphlib/exts/openstackssh.write | 169 ++++++++++++++++++++++++++++++++++++++
morphlib/exts/vdaboot.configure | 34 ++++++++
2 files changed, 203 insertions(+)
create mode 100755 morphlib/exts/openstackssh.write
create mode 100755 morphlib/exts/vdaboot.configure
diff --git a/morphlib/exts/openstackssh.write b/morphlib/exts/openstackssh.write
new file mode 100755
index 0000000..9e2a5ff
--- /dev/null
+++ b/morphlib/exts/openstackssh.write
@@ -0,0 +1,169 @@
+#!/usr/bin/python
+# Copyright (C) 2012-2013 Codethink Limited
This file has only existed in 2013, so it should only be Copyright 2013.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 2 of the License.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+
+'''A Morph deployment write extension for deploying to
OpenStack.'''
+
+
+import cliapp
+import os
+import re
+import sys
+import tempfile
+import urlparse
+
+import morphlib.writeexts
+
+
+class OpenStackWriteExtension(morphlib.writeexts.WriteExtension):
+
+ '''Create a raw disk image during Morph's deployment and upload it
+ to an OpenStack host creating the image on the system using Glance.
+
+ The location command line argument is the ssh path to upload the
+ image file into OpenStack using the following syntax:
+
+ ssh://[USER@]HOST/PATH/TO/FILE
I spy tabs. We don't like tabs, since different text editors expand tabs
to different widths. Replace each tab with 4 spaces.
+
+ where
+
+ * USER is optional, is the user to connect via ssh
+ * HOST is the host running OpenStack
+ * PATH/TO/FILE is the path to store the image file into the host.
+
+
+ This extension needs in the enviroment the following variables:
+
+ * OPENSTACK_USER is the username to use in the deployment.
+ * OPENSTACK_TENANT is the project name to use in the deployment.
+ * OPENSTACK_IMAGENAME is the name of the image to create.
+ *
+ *
Useless bullet points. It's one of those careless mistakes that's easily
overlooked, and is why code review helps :)
+
+
+ The extension will connect to HOST via ssh to push the image and then
+ it will connect again via ssh to run OpenStack management tools.
+
+ '''
+
+ def process_args(self, args):
+ if len(args) != 2:
+ raise cliapp.AppException('Wrong number of command line args')
+
+ temp_root, location = args
+ ssh_host, ssh_path = self.parse_location(location)
Having code be at the correct indentation level is very important for python.
Some default settings to vim can mess with that (automatically indenting with
tabs is annoying). Our general guide for indentation is four spaces, and if
that isn't enough, something has gone wrong.
There are indentation issues in other places, but I'm not going to point them
all out.
+
+ self.check_openstack_requirements()
+
+ fd, raw_disk = tempfile.mkstemp()
+ os.close(fd)
+ self.create_local_system(temp_root, raw_disk)
+ self.status(msg='Temporal disk image has been created at %s' % raw_disk)
This line is more than 79 characters wide. Nothing will break, but it makes
it harder to read in some terminals, and part of our style guide is to not
go over 79 characters. If this was in morphlib, instead of an extension,
then ./check --full would complain about it.
Also, you probably mean 'Temporary' instead of 'Temporal', since
'Temporal'
means it's to do with time, not the opposite of permanent.
Whether we need to mention this is another issue, but that's more like
arguing about the colour of the bikeshed (look up bikeshedding for an
explanation of the metaphor)
+
+ self.upgrade_extlinux(raw_disk)
+
+
+ self.upload_openstack(raw_disk, ssh_host, ssh_path)
+
+ os.remove(raw_disk)
+ self.status(msg='Temporal image has been deleted')
Don't forget about the timey-wimey here as well :)
+
+ self.deploy_image_openstack(ssh_host, ssh_path)
+
+ def upgrade_extlinux(self, raw_disk):
I don't think 'upgrade' is the word you want here, but 'update' as
you use in the docstring. We aren't making extlinux better, but we
are changing it to something more appropriate.
+ mp = self.mount(raw_disk)
+ '''Updating extlinux to use virtio disks'''
The docstring (the stuff in '''s) should go before anything else after
the function definition.
+
+ self.status(msg='Updating extlinux.conf')
+
+ config = os.path.join(mp, 'extlinux.conf')
+ with open(config, 'w') as f:
+ f.write('default linux\n')
+ f.write('timeout 1\n')
+ f.write('label linux\n')
+ f.write('kernel /systems/default/kernel\n')
+ f.write('append root=/dev/vda '
+ 'rootflags=subvol=systems/default/run '
+ 'init=/sbin/int rw\n')
+
+# Working without this:
+# cliapp.runcmd(['extlinux', '--install', mp])
+# cliapp.runcmd(['sync'])
+# time.sleep(2)
You should delete code that's not needed, rather than commenting it out.
In situations where you had the code and want it back, you should have
already committed it in git history, so that you can get it back.
Since you say it works without it, it should not be kept.
+
+ self.unmount(mp)
+
+
+ def check_openstack_requirements(self):
+ os_user = os.environ.get('OPENSTACK_USER')
+ os_tenant = os.environ.get('OPENSTACK_TENANT')
+ os_imagename = os.environ.get('OPENSTACK_IMAGENAME')
+ os_password = os.environ.get('OPENSTACK_PASSWORD')
+ os_authurl = os.environ.get('OPENSTACK_AUTH_URL')
+
+ if os_user is None:
+ raise cliapp.AppException('OPENSTACK_USER was not given')
+ if os_tenant is None:
+ raise cliapp.AppException('OPENSTACK_TENANT was not given')
+ if os_imagename is None:
+ raise cliapp.AppException('OPENSTACK_IMAGENAME was not given')
+ if os_password is None:
+ raise cliapp.AppException('OPENSTACK_PASSWORD was not given')
+ if os_authurl is None:
+ raise cliapp.AppException('OPENSTACK_AUTH_URL was not given')
+
+ def upload_openstack(self, raw_disk, ssh_host, ssh_path):
+ self.status(msg='Uploading raw image to OpenStack via ssh...')
+ ssh_fullpath = ssh_host + ':' + ssh_path
The way we commonly make strings out of variables is with
'%s:%s' % (ssh_host, ssh_path)
Mainly because using '+' for many different things can get confusing.
+ cliapp.runcmd(['rsync', '-szS', raw_disk,
ssh_fullpath ])
+ self.status(msg='Raw image uploaded.')
+
+ def parse_location(self, location):
+ x = urlparse.urlparse(location)
+ if x.scheme != 'ssh':
+ raise cliapp.AppException('URL schema must be ssh in %s' % location)
This line is too long, too, although this depends quite a lot on how wide
your tabs are.
+ return x.netloc, x.path
+
+ def deploy_image_openstack(self, ssh_host, ssh_path):
+ self.status(msg='Creating image into OpenStack...')
+# os_user='demo'
+# os_tenant='demo'
+# os_imagename='sshimagenamewrite'
+# os_password='horizonpass'
+# os_authurl='http://localhost:5000/v2.0'
More commented out code (plus the very-naughty not having long enough
indentation)
+
+ os_user = os.environ['OPENSTACK_USER']
+ os_tenant = os.environ['OPENSTACK_TENANT']
+ os_imagename = os.environ['OPENSTACK_IMAGENAME']
+ os_password = os.environ['OPENSTACK_PASSWORD']
+ os_authurl = os.environ['OPENSTACK_AUTH_URL']
+
+ cmdline = ['glance',
+ '--os-username', os_user,
+ '--os-tenant-name', os_tenant,
+ '--os-password', os_password,
+ '--os-auth-url', os_authurl,
+ 'image-create',
+ '--name=%s' % os_imagename,
+ '--disk-format=raw',
+ '--container-format', 'bare',
+ '--file', ssh_path]
When you have elements of an array on new lines, you want to align them
so that they all start at the same width. I don't know if you're actually
doing this because of tabs.
+ cliapp.ssh_runcmd(ssh_host, cmdline)
+
+ self.status(msg='Image created.'
+ )
I don't think you want this newline between opening and closing the
brace.
+OpenStackWriteExtension().run()
+
diff --git a/morphlib/exts/vdaboot.configure b/morphlib/exts/vdaboot.configure
new file mode 100755
index 0000000..a21269d
--- /dev/null
+++ b/morphlib/exts/vdaboot.configure
@@ -0,0 +1,34 @@
+#!/bin/sh
+# Copyright (C) 2013 Codethink Limited
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 2 of the License.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+
+# Change the "/" mount ponit to /dev/vda to use virtio disks.
Typo. you want 'point', not 'ponit'.
+
+set -e
+
+if [ "$OPENSTACK_USER" ]
+then
+ # Modifying fstab
+ if [ -f "$1/etc/fstab" ]
+ then
+ mv "$1/etc/fstab" "$1/etc/fstab.old"
+ awk 'BEGIN {print "/dev/vda / btrfs defaults,rw,noatime 0 1"}; $2
!= "/" {print $0 };' "$1/etc/fstab.old" >
"$1/etc/fstab"
I'll have to defer to someone else to say if this is correct. awk is a strange
language I don't understand
Also, again, longer than 79 characters. You may be able to separate
it into multiple lines by ending the line with a backslash, but Richard
would know more about it than me.
+ rm "$1/etc/fstab.old"
+ cp "$1/etc/fstab" "$1/etc/fstab.modified"
+ else
+ echo "/dev/vda / btrfs defaults,rw,noatime 0 1">
"$1/etc/fstab"
+ fi
+fi
9:28 a.m.
New subject: [PATCH] exts: Add openstack configure/write exts
On Fri, Aug 30, 2013 at 02:42:39PM +0100, Jonathan Maw wrote:
On Fri, Aug 30, 2013 at 01:44:59PM +0100, Pedro Alvarez wrote:
>
My overall opinion of this commit is -0. I see no problems with the
code, but I have a lot to say about the formatting.
> openstackssh.write: Write extension which deploy a raw image of baserock
> directly to an OpenStack machine using ssh. It sends the image first,
> and then deploy the image into the OpenStack system. The raw image
> deployed has modified its bootloader to use virtio disks.
>
> vdaboot.configure: Configuration extension to change the mount point of
> "/" to use virtio disks (/dev/vda).
Each commit should generally be for one thing, they don't work without each
other.
In this case, vdaboot.configure would be a separate commit to
openstackssh.write, although I can appreciate that fiddling with git to split
it into two commits can be tricky.
They _don't_ work without each other anyway. It's fine for them to be
in one commit IMO.
> + awk 'BEGIN {print "/dev/vda / btrfs
defaults,rw,noatime 0 1"}; $2 != "/" {print $0 };'
"$1/etc/fstab.old" > "$1/etc/fstab"
I'll have to defer to someone else to say if this is correct. awk is a strange
language I don't understand
It's correct. It doesn't print the previous mount at / because of the per-line
clause.
It also always prints the new entry.
Also, again, longer than 79 characters. You may be able to separate
it into multiple lines by ending the line with a backslash, but Richard
would know more about it than me.
The awk language accepts a line break at where it currently has semicolons.
9:24 a.m.
New subject: [PATCH] exts: Add openstack configure/write exts
On Fri, Aug 30, 2013 at 01:44:59PM +0100, Pedro Alvarez wrote:
diff --git a/morphlib/exts/openstackssh.write
b/morphlib/exts/openstackssh.write
new file mode 100755
index 0000000..9e2a5ff
--- /dev/null
+++ b/morphlib/exts/openstackssh.write
@@ -0,0 +1,169 @@
[snip]
+import cliapp
+import os
+import re
+import sys
You aren't using these libraries any more.
+import tempfile
+import urlparse
+
+import morphlib.writeexts
+
+
+class OpenStackWriteExtension(morphlib.writeexts.WriteExtension):
+
+ '''Create a raw disk image during Morph's deployment and upload it
+ to an OpenStack host creating the image on the system using Glance.
The first paragraph of a docstring should fit on one line.
It follows the same style as git for having a summary line, then a more
detailed description.
+ The location command line argument is the ssh path to upload the
+ image file into OpenStack using the following syntax:
+
+ ssh://[USER@]HOST/PATH/TO/FILE
+
+ where
+
+ * USER is optional, is the user to connect via ssh
+ * HOST is the host running OpenStack
+ * PATH/TO/FILE is the path to store the image file into the host.
I suspect this is no longer accurate.
+ This extension needs in the enviroment the following variables:
+
+ * OPENSTACK_USER is the username to use in the deployment.
+ * OPENSTACK_TENANT is the project name to use in the deployment.
+ * OPENSTACK_IMAGENAME is the name of the image to create.
+ *
+ *
I believe this is missing OPENSTACK_PASSWORD and OPENSTACK_AUTH_URL
+ The extension will connect to HOST via ssh to push the image and
then
+ it will connect again via ssh to run OpenStack management tools.
+
+ '''
+
+ def process_args(self, args):
+ if len(args) != 2:
+ raise cliapp.AppException('Wrong number of command line args')
+
+ temp_root, location = args
+ ssh_host, ssh_path = self.parse_location(location)
We don't use tab characters for indentation in python projects, if people
use a different tab width when tabs and spaces are mixed then it results
in code that can appear incorrectly indented.
We could have text editors merge tabs and spaces, or use only tabs
for indentation, but badly configured text editors can mess this up,
so it's easiest to only use spaces.
Plus we have some zealots who believe tabs are evil running the project :)
I should have remembered to tell you to run the test suite before
submitting the patch, this would have caught the tab characters.
+ self.check_openstack_requirements()
This is also badly indented, but I think it should also return all 5
options. I'll get to that later.
+ fd, raw_disk = tempfile.mkstemp()
+ os.close(fd)
This is not your fault, it's more for other people reading this,
but I think we should change our APIs to take file objects and use
/proc/$pid/fd/$fd if they need a file path, since I dislike opening us
up to symlink attacks.
+ self.create_local_system(temp_root, raw_disk)
+ self.status(msg='Temporal disk image has been created at %s' % raw_disk)
The word is temporary. Temporal means "something related to the passage of
time".
+ self.upgrade_extlinux(raw_disk)
This needs a better name, I will get to that later.
+ self.upload_openstack(raw_disk, ssh_host, ssh_path)
I believe this should be called upload_to_openstack.
+ os.remove(raw_disk)
This is normally a cleanup error, since you will get an exception if a
command fails, such as not being able to connect to the openstack server.
In this case however, configure and write extensions are given a private
TMPDIR, so they do not have to clean up their own temp files.
+ self.status(msg='Temporal image has been deleted')
s/Temporal/Temporary/
+ self.deploy_image_openstack(ssh_host, ssh_path)
I believe it would be clearer to call this `configure_openstack_image`,
since deploy means more than just setting up openstack to start the image.
However I may be confusing our definition of deploy with openstack's own.
Also, I believe this function should be given the openstack configuration
variables, instead of getting them from the environment itself.
+ def upgrade_extlinux(self, raw_disk):
I think we should change the create_local_system function to virtio,
but we can look at that later.
I think we should change the name to set_exlinux_root_to_virtio. It's
not necessarily an upgrade, since I think emulated disk can discard,
while virtio can't, but virtio's faster.
+ mp = self.mount(raw_disk)
You _have_ to be careful with mountpoints. If any of your commands
fail then it will not unmount the disk image, and will require manual
intervention (or a restart in some cases) to free up the space consumed
by the disk image.
The usual flow is something like this:
mp = self.mount(raw_disk)
try:
do()
some()
things()
finally:
self.umount(mp)
+ '''Updating extlinux to use virtio disks'''
The docstring must go first in the function definition.
+
+ self.status(msg='Updating extlinux.conf')
+
+ config = os.path.join(mp, 'extlinux.conf')
+ with open(config, 'w') as f:
+ f.write('default linux\n')
+ f.write('timeout 1\n')
+ f.write('label linux\n')
+ f.write('kernel /systems/default/kernel\n')
+ f.write('append root=/dev/vda '
+ 'rootflags=subvol=systems/default/run '
+ 'init=/sbin/int rw\n')
Instead of writing the contents again, I would recommend reading it in,
changing it, then writing it out again, so that the logic for creating
extlinux.conf is not duplicated, needing to be updated in two places.
Something like the following should be used instead:
path = os.path.join(mp, 'extlinux.conf')
with open(path) as f:
extlinux_conf = f.read()
with open(path, "w") as f:
f.write(extlinux_conf.replace('root=/dev/sda', 'root=/dev/vda'))
+# Working without this:
+# cliapp.runcmd(['extlinux', '--install', mp])
+# cliapp.runcmd(['sync'])
+# time.sleep(2)
This is because the bootloader has already been installed, you're just
changing its configuration file.
If the code is not needed _remove_ it. Commented out code should not
be committed.
+
+ self.unmount(mp)
+
+
+ def check_openstack_requirements(self):
+ os_user = os.environ.get('OPENSTACK_USER')
+ os_tenant = os.environ.get('OPENSTACK_TENANT')
+ os_imagename = os.environ.get('OPENSTACK_IMAGENAME')
+ os_password = os.environ.get('OPENSTACK_PASSWORD')
+ os_authurl = os.environ.get('OPENSTACK_AUTH_URL')
+
+ if os_user is None:
+ raise cliapp.AppException('OPENSTACK_USER was not given')
+ if os_tenant is None:
+ raise cliapp.AppException('OPENSTACK_TENANT was not given')
+ if os_imagename is None:
+ raise cliapp.AppException('OPENSTACK_IMAGENAME was not given')
+ if os_password is None:
+ raise cliapp.AppException('OPENSTACK_PASSWORD was not given')
+ if os_authurl is None:
+ raise cliapp.AppException('OPENSTACK_AUTH_URL was not given')
There is a lot of redundant logic here. If I wanted the same behaviour
I would do something like this:
keys = ('OPENSTACK_USER', 'OPENSTACK_TENANT',
'OPENSTACK_IMAGENAME', 'OPENSTACK_PASSWORD',
'OPENSTACK_AUTH_URL')
for key in keys:
if key not in os.environ:
raise cliapp.AppException(key + ' was not given')
I also suggested that it should return the values, so that it is not
getting environment variables twice.
You can return the specified values by adding the following.
return (os.environment[key] for key in keys)
+ def upload_openstack(self, raw_disk, ssh_host, ssh_path):
+ self.status(msg='Uploading raw image to OpenStack via ssh...')
+ ssh_fullpath = ssh_host + ':' + ssh_path
+ cliapp.runcmd(['rsync', '-szS', raw_disk, ssh_fullpath ])
+ self.status(msg='Raw image uploaded.')
+
+ def parse_location(self, location):
+ x = urlparse.urlparse(location)
+ if x.scheme != 'ssh':
+ raise cliapp.AppException('URL schema must be ssh in %s' % location)
+ return x.netloc, x.path
This look ok, apart from the mix of tabs and spaces.
+ def deploy_image_openstack(self, ssh_host, ssh_path):
See previous comment on the name and taking parameters.
This should also have a docstring, unless the logging or output adequately
describes what is happening.
+ self.status(msg='Creating image into OpenStack...')
+# os_user='demo'
+# os_tenant='demo'
+# os_imagename='sshimagenamewrite'
+# os_password='horizonpass'
+# os_authurl='http://localhost:5000/v2.0'
See previous comments on commented out code.
+ os_user = os.environ['OPENSTACK_USER']
+ os_tenant = os.environ['OPENSTACK_TENANT']
+ os_imagename = os.environ['OPENSTACK_IMAGENAME']
+ os_password = os.environ['OPENSTACK_PASSWORD']
+ os_authurl = os.environ['OPENSTACK_AUTH_URL']
+
+ cmdline = ['glance',
+ '--os-username', os_user,
+ '--os-tenant-name', os_tenant,
+ '--os-password', os_password,
+ '--os-auth-url', os_authurl,
+ 'image-create',
+ '--name=%s' % os_imagename,
+ '--disk-format=raw',
+ '--container-format', 'bare',
+ '--file', ssh_path]
+ cliapp.ssh_runcmd(ssh_host, cmdline)
If I'm reading this correctly, your approach for adding a Baserock image
to openstack is to rsync the image onto the target, then register it
with openstack's tools locally.
I can see why you have done it this way, since it is how we deploy to
libvirt and virtualbox, but I do not believe this is how OpenStack is
usually deployed to.
I think glance is supposed to do the image upload, using OpenStack's
authentication.
This is important, as we're doing this work for a big company that will
not be allowing ssh access to the OpenStack infrastructure.
+ self.status(msg='Image created.'
+ )
Why is the closing parenthesis on a different line?
diff --git a/morphlib/exts/vdaboot.configure
b/morphlib/exts/vdaboot.configure
new file mode 100755
index 0000000..a21269d
--- /dev/null
+++ b/morphlib/exts/vdaboot.configure
@@ -0,0 +1,34 @@
+#!/bin/sh
+# Copyright (C) 2013 Codethink Limited
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 2 of the License.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+
+# Change the "/" mount ponit to /dev/vda to use virtio disks.
+
+set -e
+
+if [ "$OPENSTACK_USER" ]
+then
+ # Modifying fstab
+ if [ -f "$1/etc/fstab" ]
+ then
+ mv "$1/etc/fstab" "$1/etc/fstab.old"
+ awk 'BEGIN {print "/dev/vda / btrfs defaults,rw,noatime 0 1"}; $2
!= "/" {print $0 };' "$1/etc/fstab.old" >
"$1/etc/fstab"
This is mostly ok, but this awk command should be broken up into multiple lines so it is
not so long.
+ rm "$1/etc/fstab.old"
+ cp "$1/etc/fstab" "$1/etc/fstab.modified"
Why are you keeping a copy of the modified version here?
+ else
+ echo "/dev/vda / btrfs defaults,rw,noatime 0 1">
"$1/etc/fstab"
+ fi
+fi
5:52 a.m.
New subject: [PATCH] exts: Add openstack configure/write exts
On Fri, Aug 30, 2013 at 03:24:19PM +0100, Richard Maw wrote:
> + fd, raw_disk = tempfile.mkstemp()
> + os.close(fd)
This is not your fault, it's more for other people reading this,
but I think we should change our APIs to take file objects and use
/proc/$pid/fd/$fd if they need a file path, since I dislike opening us
up to symlink attacks.
If we want to avoid such symlink attacks, then the correct thing to do
is to use the file descriptor, and if a Python file handle is wanted,
use os.fdopen to create one.
I'd avoid the symlink attacks by creating a directory with
tempfile.mkdtemp, and then creating files in the directory.
--
http://www.codethink.co.uk/ http://wiki.baserock.org/ http://www.baserock.com/
6:54 a.m.
New subject: [PATCH] exts: Add openstack configure/write exts
On Mon, Sep 02, 2013 at 11:52:32AM +0100, Lars Wirzenius wrote:
If we want to avoid such symlink attacks, then the correct thing to
do
is to use the file descriptor, and if a Python file handle is wanted,
use os.fdopen to create one.
Yes, but the issue is when there's APIs that require file paths, like
how we currently shell out to losetup.
I'd avoid the symlink attacks by creating a directory with
tempfile.mkdtemp, and then creating files in the directory.
That solution slipped my mind at the time, and is certainly a better
solution in the short term, and will last into the long term.
3492
days inactive
3495
days old
7 comments
4 participants
participants (4)
-
Jonathan Maw -
Lars Wirzenius -
Pedro Alvarez -
Richard Maw