11:07 a.m.
repo: ssh://git@git.baserock.org/baserock/baserock/morph
branch: baserock/tiagogomes/ssh-rsync-work
commit: 8af8e58ae1b2c44489501c5028105a90473c6f87
Tiago Gomes (1):
Improvements to ssh-rsync extension
morphlib/exts/ssh-rsync.write | 145 ++++++++++++++++++++++-------------------
1 file changed, 77 insertions(+), 68 deletions(-)
--
1.7.10.4
11:07 a.m.
---
morphlib/exts/ssh-rsync.write | 145 ++++++++++++++++++++++-------------------
1 file changed, 77 insertions(+), 68 deletions(-)
diff --git a/morphlib/exts/ssh-rsync.write b/morphlib/exts/ssh-rsync.write
index 9697e21..2436617 100755
--- a/morphlib/exts/ssh-rsync.write
+++ b/morphlib/exts/ssh-rsync.write
@@ -50,65 +50,74 @@ class SshRsyncWriteExtension(morphlib.writeexts.WriteExtension):
root_disk = self.find_root_disk(location)
version_label = os.environ.get('VERSION_LABEL')
+ self.status(msg='Creating remote mount point')
+ remote_mnt = cliapp.ssh_runcmd(location, ['mktemp',
'-d']).strip()
try:
- self.status(msg='Creating remote mount point')
- remote_mnt = cliapp.ssh_runcmd(location, ['mktemp',
'-d']).strip()
-
self.status(msg='Mounting root disk')
cliapp.ssh_runcmd(location, ['mount', root_disk, remote_mnt])
+ except Exception as e:
+ try:
+ cliapp.ssh_runcmd(location, ['rmdir', remote_mnt])
+ except:
+ pass
+ raise e
+ try:
version_root = os.path.join(remote_mnt, 'systems', version_label)
run_dir = os.path.join(version_root, 'run')
orig_dir = os.path.join(version_root, 'orig')
+
+ self.status(msg='Creating %s' % version_root)
+ cliapp.ssh_runcmd(location, ['mkdir', version_root])
+
+ self.create_remote_orig(location, version_root, remote_mnt,
+ temp_root)
+
+ self.status(msg='Creating "run" subvolume')
+ cliapp.ssh_runcmd(location, ['btrfs', 'subvolume',
+ 'snapshot', orig_dir, run_dir])
+
+ self.status(msg='Updating system configuration')
+ bscs_loc = os.path.join(run_dir, 'usr', 'bin',
+ 'baserock-system-config-sync')
+
+ output = cliapp.ssh_runcmd(location, ['sh', '-c',
+ '%s merge %s &> /dev/null || echo -n cmdfailed'
+ % (bscs_loc, version_label)])
+ if output:
+ self.status(msg='Updating system configuration failed')
+
+ self.install_remote_kernel(location, version_root, temp_root)
+ default_path = os.path.join(remote_mnt, 'systems',
'default')
+ if self.bootloader_is_wanted():
+ output = cliapp.ssh_runcmd(location, ['sh', '-c',
+ 'test -e "$1" && stat -c %F
"$1" '
+ '|| echo missing file',
+ '-', default_path])
+ if output != "symbolic link":
+ # we are upgrading and old system that does
+ # not have an updated extlinux config file
+ self.update_remote_extlinux(location, remote_mnt,
+ version_label)
+ cliapp.ssh_runcmd(location, ['ln', '-sfn', version_label,
+ default_path])
+ except Exception as e:
try:
- self.status(msg='Creating %s' % version_root)
- cliapp.ssh_runcmd(location, ['mkdir', version_root])
-
- self.create_remote_orig(location, version_root, remote_mnt,
- temp_root)
-
- self.status(msg='Creating "run" subvolume')
- cliapp.ssh_runcmd(location, ['btrfs', 'subvolume',
- 'snapshot', orig_dir, run_dir])
-
- self.status(msg='Updating system configuration')
- bscs_loc = os.path.join(run_dir, 'usr', 'bin',
- 'baserock-system-config-sync')
- try:
- cliapp.ssh_runcmd(location, ['sh', bscs_loc,
'merge',
- version_label])
- except:
- self.status(msg='Updating system configuration failed')
-
- self.install_remote_kernel(location, version_root, temp_root)
- default_path = os.path.join(remote_mnt, 'systems',
'default')
- if self.bootloader_is_wanted():
- output = cliapp.ssh_runcmd(location, ['sh', '-c',
- 'test -e "$1" && stat -c %F
"$1"'
- ' || '
- 'echo missing file',
- '-', default_path])
- if output != "symbolic link":
- # we are upgrading and old system that does
- # not have an updated extlinux config file
- self.update_remote_extlinux(location, remote_mnt,
- version_label)
- cliapp.ssh_runcmd(location, ['ln', '-sfn',
version_label,
- default_path])
- except Exception as e:
- try:
- cliapp.ssh_runcmd(location,
- ['btrfs', 'subvolume', 'delete',
run_dir])
- cliapp.ssh_runcmd(location,
- ['btrfs', 'subvolume', 'delete',
orig_dir])
- cliapp.ssh_runcmd(location, ['rm', '-rf',
version_root])
- except:
- pass
- raise e
-
- except:
- raise
- else:
+ cliapp.ssh_runcmd(location,
+ ['btrfs', 'subvolume', 'delete', run_dir])
+ except:
+ pass
+ try:
+ cliapp.ssh_runcmd(location,
+ ['btrfs', 'subvolume', 'delete', orig_dir])
+ except:
+ pass
+ try:
+ cliapp.ssh_runcmd(location, ['rm', '-rf', version_root])
+ except:
+ pass
+ raise e
+ finally:
self.status(msg='Removing temporary mounts')
cliapp.ssh_runcmd(location, ['umount', remote_mnt])
cliapp.ssh_runcmd(location, ['rmdir', remote_mnt])
@@ -118,8 +127,8 @@ class SshRsyncWriteExtension(morphlib.writeexts.WriteExtension):
self.status(msg='Creating extlinux.conf')
config = os.path.join(remote_mnt, 'extlinux.conf')
- temp_file = tempfile.mkstemp()[1]
- with open(temp_file, 'w') as f:
+ temp_file = tempfile.mkstemp()
+ with os.fdopen(temp_file[0], 'w') as f:
f.write('default linux\n')
f.write('timeout 1\n')
f.write('label linux\n')
@@ -128,14 +137,13 @@ class SshRsyncWriteExtension(morphlib.writeexts.WriteExtension):
'rootflags=subvol=systems/default/run '
'init=/sbin/init rw\n')
- cliapp.ssh_runcmd(location, ['mv', config, config+'~'])
-
try:
- cliapp.runcmd(['rsync', '-as', temp_file,
- '%s:%s' % (location, config)])
+ cliapp.runcmd(['rsync', '-as', temp_file[1],
+ '%s:%s~' % (location, config)])
+ cliapp.ssh_runcmd(location, ['mv', config+'~', config])
except Exception as e:
try:
- cliapp.ssh_runcmd(location, ['mv', config+'~', config])
+ cliapp.ssh_runcmd(location, ['rm', '-f',
config+'~'])
except:
pass
raise e
@@ -168,19 +176,19 @@ class SshRsyncWriteExtension(morphlib.writeexts.WriteExtension):
for line in contents.splitlines():
line_words = line.split()
if (line_words[1] == '/' and line_words[0] != 'rootfs'):
- return line_words[0]
+ return line_words[0]
def install_remote_kernel(self, location, version_root, temp_root):
'''Install the kernel in temp_root inside version_root on
location'''
self.status(msg='Installing kernel')
- image_names = ['vmlinuz', 'zImage', 'uImage']
+ image_names = ('vmlinuz', 'zImage', 'uImage')
kernel_dest = os.path.join(version_root, 'kernel')
for name in image_names:
try_path = os.path.join(temp_root, 'boot', name)
if os.path.exists(try_path):
cliapp.runcmd(['rsync', '-as', try_path,
- '%s:%s' % (location, kernel_dest)])
+ '%s:%s' % (location, kernel_dest)])
def check_valid_target(self, location):
try:
@@ -189,14 +197,15 @@ class SshRsyncWriteExtension(morphlib.writeexts.WriteExtension):
raise cliapp.AppException('%s does not respond to ssh:\n%s'
% (location, e))
- try:
- cliapp.ssh_runcmd(location, ['test', '-d',
'/baserock'])
- except:
- raise cliapp.AppException('%s is not a baserock system' % location)
+ output = cliapp.ssh_runcmd(location, ['sh', '-c',
+ 'test -d /baserock || echo -n dirnotfound'])
+ if output:
+ raise cliapp.AppException('%s is not a baserock system'
+ % location)
- try:
- cliapp.ssh_runcmd(location, ['which', 'rsync'])
- except:
+ output = cliapp.ssh_runcmd(location, ['sh', '-c',
+ 'type rsync &> /dev/null || echo -n cmdnotfound'])
+ if output:
raise cliapp.AppException('%s does not have rsync')
SshRsyncWriteExtension().run()
--
1.7.10.4
11:25 a.m.
On Fri, Jun 28, 2013 at 04:07:08PM +0100, Tiago Gomes wrote:
---
morphlib/exts/ssh-rsync.write | 145 ++++++++++++++++++++++-------------------
1 file changed, 77 insertions(+), 68 deletions(-)
diff --git a/morphlib/exts/ssh-rsync.write b/morphlib/exts/ssh-rsync.write
index 9697e21..2436617 100755
--- a/morphlib/exts/ssh-rsync.write
+++ b/morphlib/exts/ssh-rsync.write
@@ -50,65 +50,74 @@ class SshRsyncWriteExtension(morphlib.writeexts.WriteExtension):
[snip]
+ output = cliapp.ssh_runcmd(location, ['sh',
'-c',
+ '%s merge %s &> /dev/null || echo -n cmdfailed'
+ % (bscs_loc, version_label)])
This will fall foul to whitespace if the version label has spaces in it.
Try the following argv:
['sh', '-c', '"$1" merge "$2" &>
/dev/null || echo -n cmdfailed',
'-', bscs_loc, version_label]
Technically &> is non-portable, but I don't care, so long as it works
in busybox ash.
+ if output:
I would prefer if you check the output is "cmdfailed", since otherwise
you may as well catch the exception ssh_runcmd gives to determine if
it failed.
[snip]
@@ -118,8 +127,8 @@ class
SshRsyncWriteExtension(morphlib.writeexts.WriteExtension):
self.status(msg='Creating extlinux.conf')
config = os.path.join(remote_mnt, 'extlinux.conf')
- temp_file = tempfile.mkstemp()[1]
- with open(temp_file, 'w') as f:
+ temp_file = tempfile.mkstemp()
+ with os.fdopen(temp_file[0], 'w') as f:
I prefer temp_fd, temp_path = tempfile.mkstemp(), and use temp_fd. However
I'm more than happy to accept it as it is.
[snip]
def install_remote_kernel(self, location, version_root,
temp_root):
'''Install the kernel in temp_root inside version_root on
location'''
self.status(msg='Installing kernel')
- image_names = ['vmlinuz', 'zImage', 'uImage']
+ image_names = ('vmlinuz', 'zImage', 'uImage')
Changes like this have come up before. I don't know if it's any faster
to use a tuple instead of a list, but I do prefer it :)
[snip]
@@ -189,14 +197,15 @@ class
SshRsyncWriteExtension(morphlib.writeexts.WriteExtension):
raise cliapp.AppException('%s does not respond to ssh:\n%s'
% (location, e))
- try:
- cliapp.ssh_runcmd(location, ['test', '-d',
'/baserock'])
- except:
- raise cliapp.AppException('%s is not a baserock system' % location)
+ output = cliapp.ssh_runcmd(location, ['sh', '-c',
+ 'test -d /baserock || echo -n dirnotfound'])
+ if output:
+ raise cliapp.AppException('%s is not a baserock system'
+ % location)
- try:
- cliapp.ssh_runcmd(location, ['which', 'rsync'])
- except:
+ output = cliapp.ssh_runcmd(location, ['sh', '-c',
+ 'type rsync &> /dev/null || echo -n cmdnotfound'])
+ if output:
raise cliapp.AppException('%s does not have rsync')
I'm not sure why you're changing from catching the exception to checking
the output. If you're going to test whether a command failed predictably
then I think you should check the output is for an expected orror,
and raise a different exception if it's one you didn't.
11:40 a.m.
On 28/06/13 16:25, Richard Maw wrote:
On Fri, Jun 28, 2013 at 04:07:08PM +0100, Tiago Gomes wrote:
> ---
> morphlib/exts/ssh-rsync.write | 145 ++++++++++++++++++++++-------------------
> 1 file changed, 77 insertions(+), 68 deletions(-)
>
> diff --git a/morphlib/exts/ssh-rsync.write b/morphlib/exts/ssh-rsync.write
> index 9697e21..2436617 100755
> --- a/morphlib/exts/ssh-rsync.write
> +++ b/morphlib/exts/ssh-rsync.write
> @@ -50,65 +50,74 @@ class SshRsyncWriteExtension(morphlib.writeexts.WriteExtension):
[snip]
> + output = cliapp.ssh_runcmd(location, ['sh', '-c',
> + '%s merge %s &> /dev/null || echo -n
cmdfailed'
> + % (bscs_loc, version_label)])
This will fall foul to whitespace if the version label has spaces in it.
Try the following argv:
['sh', '-c', '"$1" merge "$2" &>
/dev/null || echo -n cmdfailed',
'-', bscs_loc, version_label]
Technically &> is non-portable, but I don't care, so long as it works
in busybox ash.
> + if output:
I would prefer if you check the output is "cmdfailed", since otherwise
you may as well catch the exception ssh_runcmd gives to determine if
it failed.
I can change to cmdfailed here, and to check against dirnotfound and
cmdnotfound on the other parts of the code. But note that if ssh_runcmd
gives an exception, this block is not even run.
[snip]
> @@ -118,8 +127,8 @@ class SshRsyncWriteExtension(morphlib.writeexts.WriteExtension):
>
> self.status(msg='Creating extlinux.conf')
> config = os.path.join(remote_mnt, 'extlinux.conf')
> - temp_file = tempfile.mkstemp()[1]
> - with open(temp_file, 'w') as f:
> + temp_file = tempfile.mkstemp()
> + with os.fdopen(temp_file[0], 'w') as f:
I prefer temp_fd, temp_path = tempfile.mkstemp(), and use temp_fd. However
I'm more than happy to accept it as it is.
[snip]
> def install_remote_kernel(self, location, version_root, temp_root):
> '''Install the kernel in temp_root inside version_root on
location'''
>
> self.status(msg='Installing kernel')
> - image_names = ['vmlinuz', 'zImage', 'uImage']
> + image_names = ('vmlinuz', 'zImage', 'uImage')
Changes like this have come up before. I don't know if it's any faster
to use a tuple instead of a list, but I do prefer it :)
[snip]
> @@ -189,14 +197,15 @@ class
SshRsyncWriteExtension(morphlib.writeexts.WriteExtension):
> raise cliapp.AppException('%s does not respond to ssh:\n%s'
> % (location, e))
>
> - try:
> - cliapp.ssh_runcmd(location, ['test', '-d',
'/baserock'])
> - except:
> - raise cliapp.AppException('%s is not a baserock system' %
location)
> + output = cliapp.ssh_runcmd(location, ['sh', '-c',
> + 'test -d /baserock || echo -n dirnotfound'])
> + if output:
> + raise cliapp.AppException('%s is not a baserock system'
> + % location)
>
> - try:
> - cliapp.ssh_runcmd(location, ['which', 'rsync'])
> - except:
> + output = cliapp.ssh_runcmd(location, ['sh', '-c',
> + 'type rsync &> /dev/null || echo -n
cmdnotfound'])
> + if output:
> raise cliapp.AppException('%s does not have rsync')
I'm not sure why you're changing from catching the exception to checking
the output. If you're going to test whether a command failed predictably
then I think you should check the output is for an expected orror,
and raise a different exception if it's one you didn't.
Your own words:
"Technically this could fail from not having which or rsync, or a host
of other problems. With a bare except, it could even fail from a
syntax error."
Tiago
_______________________________________________
baserock-dev mailing list
baserock-dev(a)baserock.org
http://vlists.pepperfish.net/cgi-bin/mailman/listinfo/baserock-dev-basero...
11:53 a.m.
On Fri, Jun 28, 2013 at 04:40:20PM +0100, Tiago Gomes wrote:
On 28/06/13 16:25, Richard Maw wrote:
>I'm not sure why you're changing from catching the exception to checking
>the output. If you're going to test whether a command failed predictably
>then I think you should check the output is for an expected orror,
>and raise a different exception if it's one you didn't.
Your own words:
"Technically this could fail from not having which or rsync, or a host
of other problems. With a bare except, it could even fail from a
syntax error."
In which case we need to both check the output and check its return code.
It will print the error message and return true in the case that the command
you wanted to run failed.
The command can also return non-zero from an internal error in the shell, or
if the ssh connection failed. At which point the exception also needs to be
caught for cleanup.
12:47 p.m.
On 28/06/13 16:53, Richard Maw wrote:
On Fri, Jun 28, 2013 at 04:40:20PM +0100, Tiago Gomes wrote:
> On 28/06/13 16:25, Richard Maw wrote:
>> I'm not sure why you're changing from catching the exception to checking
>> the output. If you're going to test whether a command failed predictably
>> then I think you should check the output is for an expected orror,
>> and raise a different exception if it's one you didn't.
> Your own words:
>
> "Technically this could fail from not having which or rsync, or a host
> of other problems. With a bare except, it could even fail from a
> syntax error."
In which case we need to both check the output and check its return code.
It will print the error message and return true in the case that the command
you wanted to run failed.
The command can also return non-zero from an internal error in the shell, or
if the ssh connection failed. At which point the exception also needs to be
caught for cleanup.
I am not completely sure of what you mean. I see 3 possible outcomes for
testing for instance if /baserock exists on the host:
1: the command ran with success and /baserock exists - do nothing
2: the command ran with success and and /baserock does not exist - raise
an exception informing the user that /baserock was not found.
3: the command returned with non-zero code, in which case something
unexpected had happen (e.g. ssh connection failed) - propagate the
exception, not handle it.
Tiago
_______________________________________________
baserock-dev mailing list
baserock-dev(a)baserock.org
http://vlists.pepperfish.net/cgi-bin/mailman/listinfo/baserock-dev-basero...
5:16 a.m.
Sorry, I'm communicating badly.
On Fri, Jun 28, 2013 at 05:47:26PM +0100, Tiago Gomes wrote:
I am not completely sure of what you mean. I see 3 possible outcomes
for testing for instance if /baserock exists on the host:
1: the command ran with success and /baserock exists - do nothing
2: the command ran with success and and /baserock does not exist -
raise an exception informing the user that /baserock was not found.
Your approach is more or less correct here, but I would prefer you to
explicitly check the output of commands, since I have previously had
issues with scripts that expect no output on success.
3: the command returned with non-zero code, in which case something
unexpected had happen (e.g. ssh connection failed) - propagate the
exception, not handle it.
You need to handle it to do the necessary cleanup.
5:29 a.m.
On 01/07/13 10:16, Richard Maw wrote:
Sorry, I'm communicating badly.
On Fri, Jun 28, 2013 at 05:47:26PM +0100, Tiago Gomes wrote:
> I am not completely sure of what you mean. I see 3 possible outcomes
> for testing for instance if /baserock exists on the host:
> 1: the command ran with success and /baserock exists - do nothing
> 2: the command ran with success and and /baserock does not exist -
> raise an exception informing the user that /baserock was not found.
Your approach is more or less correct here, but I would prefer you to
explicitly check the output of commands, since I have previously had
issues with scripts that expect no output on success.
I already changed to do that.
> 3: the command returned with non-zero code, in which case something
> unexpected had happen (e.g. ssh connection failed) - propagate the
> exception, not handle it.
You need to handle it to do the necessary cleanup.
At this point, I don't think that I need to do any necessary cleanup.
This part is the first bit of code that is runned by the extension. The
remote disk was not even mounted. at this stage.
Tiago
_______________________________________________
baserock-dev mailing list
baserock-dev(a)baserock.org
http://vlists.pepperfish.net/cgi-bin/mailman/listinfo/baserock-dev-basero...
5:44 a.m.
On Mon, Jul 01, 2013 at 10:29:23AM +0100, Tiago Gomes wrote:
At this point, I don't think that I need to do any necessary
cleanup. This part is the first bit of code that is runned by the
extension. The remote disk was not even mounted. at this stage.
If that's the case then I'm fine with that approach, though would
like comments so I don't think it's a bug in future.
I'll re-read the patch with an eye for cleanup, but if I find no
problems then I'll be happy for you to merge.
5:52 a.m.
On Mon, Jul 01, 2013 at 10:44:29AM +0100, Richard Maw wrote:
I'll re-read the patch with an eye for cleanup, but if I find no
problems then I'll be happy for you to merge.
Your knowledge of the code is obviously superior to mine :), cleanup
does appear to be properly handled.
My only remaining question is why a failure to merge the config is
treated as a non-fatal error.
6:07 a.m.
On 01/07/13 10:52, Richard Maw wrote:
On Mon, Jul 01, 2013 at 10:44:29AM +0100, Richard Maw wrote:
> I'll re-read the patch with an eye for cleanup, but if I find no
> problems then I'll be happy for you to merge.
Your knowledge of the code is obviously superior to mine :), cleanup
does appear to be properly handled.
My only remaining question is why a failure to merge the config is
treated as a non-fatal error.
I don't think that it should be a fatal error. The upgraded version will
work fine without the system configuration merged.
The merge script will fail for instance if "a" is a regular file in one
version and a symbolic link to a file in another version. If this
happens the user can run manually the merge script on the target, verify
on the output why it failed and update the configuration manually.
Tiago
_______________________________________________
baserock-dev mailing list
baserock-dev(a)baserock.org
http://vlists.pepperfish.net/cgi-bin/mailman/listinfo/baserock-dev-basero...
3623
days inactive
3626
days old
10 comments
2 participants
participants (2)
-
Richard Maw -
Tiago Gomes