[PATCH 1/2] Add a "morph deploy" plugin with a "morph deploy rsync" command

Lars Wirzenius lars.wirzenius at codethink.co.uk
Thu Jan 3 14:41:26 GMT 2013


A general comment: I think a deployment command in morph is a great
idea. I have a fair bit of comments, below, but please don't get
the impression that I don't think this is a good idea.

On Fri, Dec 21, 2012 at 11:28:05PM +0000, Jannis Pohlmann wrote:
>   morph rsync DESTINATION [limit N] REPO REF SYSTEM

This reverses the usual order of source and destination for rsync.
Would a "morph rsync REPO REF SYSTEM DESTINATION [more rsync args]"
syntax be acceptable?

Also, the code does not actually implement this syntax. It implements
"morph deploy rsync ..." instead. I am ambivalent about whether the
"deploy" subcommand should be there or not.

Alternatives:

* morph deploy rsync REPO REF SYSTEM DESTINATION ...
* morph deploy REPO REF SYSTEM rsync DESTINATION ...
* morph rsync REPO REF SYSTEM DESTINATION ...

We could replace the "rsync" subcommand or sub-subcommand by using
URLs:

* morph deploy REPO REF SYSTEM rsync://USER@HOST:PORT/PATH ...

We could also make this work only within a system branch, in which
case the commands become a bit shorter, since REPO and REF can be
dropped.

> This method works for all types of systems and copies both the root
> file system and the kernel image to DESTINATION (typically something
> like somehost:/some/path).
> 
> At the moment only hard-coded, BSP-unspecific deployment methods
> are supported. In the future, the plugin can be extended for this
> purpose, however.

Is this deployment plugin meant only for initial installation of a new
system, or also for upgrading an existing Baserock system?

Should the code be doing configuration of the deployed system that
is dependent of the instance, such as setting hostname?

Not important at this time, but I'll note it for later: It would be nice
if deployment methods could be provided by other plugins, so that
when they grow large, they do not all need to be in the same source
file.

> +class UsageError(cliapp.AppException):

Deriving from morphlib.Error would be preferable.

> +    def __init__(self, usage):
> +        cliapp.AppException.__init__(
> +                self, 'Usage: morph deploy %s REPO REF SYSTEM' % usage)

s/usage/subsubcommand/ or something? Usage seems like a weird name for
the variable.

> +    def deploy(self, args):
> +        '''Deploys a system using one of the supported deployment methods.
> +
> +        Morph distinguishes between generic and BSP-specific deployment
> +        methods. The generic methods currently include "rsync" nad "netcat".

s/ nad/ and /

netcat does not seem to actually be implemented. There should probably,
some day, be a way to list the deployment methods automatically. Not
important for this generation of the deployment plugin, just something
to do some day in the future.

> +        BSP-specific methods need to be defined in the BSP stratum of the
> +        system you are building.
> +
> +        NOTE: At the moment, only generic methods are supported.
> +
> +        '''
> +
> +        if len(args) < 4:
> +            raise cliapp.AppException(
> +                    'Expecting at least a deployment METHOD and '
> +                    'a REPO REF SYSTEM triplet to deploy')

It'd probably be good to not silently ignore extra arguments.

> +    def system_artifacts(self, repo, ref, system):
> +        # Resolve all artifacts involved in building this system.

The comment should be a docstring.

> +        rsync_args = []
> +        for i in xrange(0, len(args)):
> +            if args[i] == 'limit':
> +                if i < len(args)-1:
> +                    rsync_args += ['--bwlimit=%s' % args[i+1]]
> +                else:
> +                    raise UsageError(usage)
> +        rsync_args += filenames
> +        rsync_args.append(args[0]) # the destination

As a point of general usability, I'd rather this kind of thing was
a configuration setting in the program. That way a) the command line
is easier to parse and b) the setting can be set in a configuration
file, if the user wants it.

> +        self.run_cmd(['rsync', '-vP'] + rsync_args)

rsync -P produces progress reporting. This can be a bad idea if
"morph deploy" is run from a cron job, or jenkins job, and even when
it's run interactively, it would be nice to be able to turn it off,
since it can make the output be voluminous, and that can make it
harder to notice errors.

Would --partial be enough (-P is an alias for --partial and --progress
together), unless the user requests verbosity?

> +    def run_cmd(self, args):
> +        process = subprocess.Popen(args,
> +                                   stdout=subprocess.PIPE,
> +                                   stderr=subprocess.STDOUT)

I'd like all the external command invocations to go via the
morphlib.app.Application.runcmd method, so that we get consistent
logging of them.

> +        stdout = process.stdout
> +        while True:
> +            line = stdout.readline()
> +            if not line:
> +                break
> +            self.app.output.write(line)

Should this just use stdout=self.app.output and skip the loop?

-- 
http://www.codethink.co.uk/ http://wiki.baserock.org/ http://www.baserock.com/




More information about the baserock-dev mailing list