Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3391008:
New application cros_image_to_target.py for ssh_tunnel upgrade (Closed)
|
Created:
10 years, 3 months ago by Paul Stewart Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa Base URL:
ssh://gitrw.chromium.org/crosutils.git Visibility:
Public. |
DescriptionNew application cros_image_to_target.py for ssh_tunnel upgrade
The RF testbed has hosts behind a one-way firewall, so the
conventional "image_to_live" script does not work. My initial
attempt at solving this, 'cros_copy_upgrade_server', while
sometimes functional, was always quite brittle since it had
widely ranging dependencies which often changed in ways that
the authors could not predict would break our setup.
I've written from scratch a new script in python which uses
ssh tunnels to get around the problem of the firewall issue.
It also has retlatively minimal dependencies (stateful_update,
cros_generate_update_paylod, cgpt, get_latest_image.sh and
the testing keys) so hopefully it won't get blown away too
often.
BUG=none
TEST=Ran under chroot -- Need help testing
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=b0d5cf2
Patch Set 1 #
Total comments: 59
Patch Set 2 : Correct board handling, timeouts #Patch Set 3 : gpylint run #Patch Set 4 : Large set of fixes from gpylint and sosa's suggestions #
Total comments: 27
Patch Set 5 : Second round of sosa input #Patch Set 6 : Latest batch of sosa fixes -- changed child exit behavior #Messages
Total messages: 9 (0 generated)
Replace me on review list by sosa+wad.
I have a big concern that you are duplicating a lot of the dev server work which is going to be reasonably difficult to maintain and probably will be just as hard to maintain. For example, most of my re-work of the dev server has been to clean it up and fix a lot of bugs it had with the changes for verified boot and the new update engine. If changes to the interface happen again, the dev server will have to change as well as this script. Having said that, I understand how frustrating it has been to be down downwind of changes you don't care about, but I think this is one of many possible solutions. For example, I've been meaning to look into creating subclasses of the dev servers that handled each of the control flows separately. In your case, you'd want to build a subclass that had most of the same functionality but didn't use certain functions. But what I'm trying to say with that, I think a better solution that duplicating the dev server work is to help fix the dev server. Not enough work has really been put into it given how many things we have relying on it. Part of that is adding much more tests to the dev server and my work right now is focused on doing that. Sadly if you go this route, you won't see much gain from that work. I'm still ok with this. However, you should at least do the following things before submitting: 1) Run gpylint, your style is close, but remember we use PEP-8 + 2 sp indent and MethodNames. gpylint may throw you some bogus errors (like the copyright, etc), but for the most part is correct. This will make your code much more maintainable and easier to read. 2) Docstrings for all your public functions. At least a single line of what is returned. I completely see what all your functions are doing since there's pretty much a 1:1 correspondence to functions in the dev server, however, an outside observer will not. 3) Some unittests. import unittest (and mox if you want to mock out things). I would at least make a few functional tests that exercise most of your code. You can start with a few high level tests that use the unittest module but aren't true unittests and refine that as the code ages. However, since this is Python, please include tests. Let's get out of the habit of getting all these re-imaging / build items without tests. I'll even volunteer to add the tests to our build workflow so you'll know immediately if another developer broke your workflow. http://codereview.chromium.org/3391008/diff/1/2 File bin/cros_image_to_target.py (right): http://codereview.chromium.org/3391008/diff/1/2#newcode3 bin/cros_image_to_target.py:3: # Copyright (c) 2009 The Chromium OS Authors. All rights reserved. 2010 http://codereview.chromium.org/3391008/diff/1/2#newcode16 bin/cros_image_to_target.py:16: __author__ = 'pstew@google.com (Paul Stewart)' No authors in Chromium OS code. http://codereview.chromium.org/3391008/diff/1/2#newcode28 bin/cros_image_to_target.py:28: class Command: docstring at least for each class. http://codereview.chromium.org/3391008/diff/1/2#newcode28 bin/cros_image_to_target.py:28: class Command: As a suggestion, take a look at RunCommand in our common py lib in lib/cros_build_lib.py. See if you can borrow any of that work. I do like your subclassing of SSHCommand though and you may to consider it making it more public of a function for other build tools. http://codereview.chromium.org/3391008/diff/1/2#newcode29 bin/cros_image_to_target.py:29: def __init__(self, env): All indentation should be 2 sp http://codereview.chromium.org/3391008/diff/1/2#newcode30 bin/cros_image_to_target.py:30: self.env = env Line between. http://codereview.chromium.org/3391008/diff/1/2#newcode31 bin/cros_image_to_target.py:31: def run_pipe(self, pipeline, infile=None, outfile=None, RunPipe not run_pipe per code style. http://codereview.chromium.org/3391008/diff/1/2#newcode37 bin/cros_image_to_target.py:37: if last_pipe is not None: Switch all of these i.e. s/if var is not None:/if var:/ http://codereview.chromium.org/3391008/diff/1/2#newcode41 bin/cros_image_to_target.py:41: if len(pipeline) or capture: if pipeline (does same thing) or capture: http://codereview.chromium.org/3391008/diff/1/2#newcode72 bin/cros_image_to_target.py:72: self.remote = remote It is preferred to have a Setup function than do work in _init_. It makes it much easier to write unittests for the class since you don't have to mock out the constructor. http://codereview.chromium.org/3391008/diff/1/2#newcode116 bin/cros_image_to_target.py:116: def info(self, msg): Suggest using info / error from common lib. If you don't you should consider making these global methods. http://codereview.chromium.org/3391008/diff/1/2#newcode132 bin/cros_image_to_target.py:132: self.cros_path('get_latest_image.sh')) No ./ ? http://codereview.chromium.org/3391008/diff/1/2#newcode134 bin/cros_image_to_target.py:134: def cached(self, src, dst): IsCached - methods should start with verbs http://codereview.chromium.org/3391008/diff/1/2#newcode197 bin/cros_image_to_target.py:197: if lsb_release is None: if not lsb_release: http://codereview.chromium.org/3391008/diff/1/2#newcode220 bin/cros_image_to_target.py:220: if status is None: if not status http://codereview.chromium.org/3391008/diff/1/2#newcode230 bin/cros_image_to_target.py:230: time.sleep(5) sleep constant? http://codereview.chromium.org/3391008/diff/1/2#newcode436 bin/cros_image_to_target.py:436: help='Display running commands') use default=False o/w you're checking against None or True rather than False or True http://codereview.chromium.org/3391008/diff/1/2#newcode440 bin/cros_image_to_target.py:440: if options.src is None: if not options.src http://codereview.chromium.org/3391008/diff/1/2#newcode441 bin/cros_image_to_target.py:441: parser.error("No --from argument given and no default image found") Use consistent " vs. '. I suggest using ''s for all but docstrings. http://codereview.chromium.org/3391008/diff/1/2#newcode447 bin/cros_image_to_target.py:447: image_directory = options.src Use 2 space indents http://codereview.chromium.org/3391008/diff/1/2#newcode448 bin/cros_image_to_target.py:448: image_file = os.path.join(options.src, 'chromiumos_image.bin') Are you sure you want image rather than chromiumos_test_image.bin? May want an option for image name. Also may want to make into a global constant http://codereview.chromium.org/3391008/diff/1/2#newcode461 bin/cros_image_to_target.py:461: if rel is None: if not rel http://codereview.chromium.org/3391008/diff/1/2#newcode463 bin/cros_image_to_target.py:463: return sys.exit(1) or better, add cros_env.Die. I suggest taking a look at the small but common functions in src/scripts/lib/cros_build_lib.py http://codereview.chromium.org/3391008/diff/1/2#newcode468 bin/cros_image_to_target.py:468: return Ditto http://codereview.chromium.org/3391008/diff/1/2#newcode474 bin/cros_image_to_target.py:474: update_file = os.path.join(image_directory, 'update.gz') Make into a global constant. http://codereview.chromium.org/3391008/diff/1/2#newcode475 bin/cros_image_to_target.py:475: stateful_file = os.path.join(image_directory, 'stateful.image.gz') Make into a global constant. http://codereview.chromium.org/3391008/diff/1/2#newcode479 bin/cros_image_to_target.py:479: return Die with an error message http://codereview.chromium.org/3391008/diff/1/2#newcode481 bin/cros_image_to_target.py:481: if not options.server_only: Why start client before server? Seems like needless sleeping. http://codereview.chromium.org/3391008/diff/1/2#newcode486 bin/cros_image_to_target.py:486: time.sleep(1) Make 1 into some constant. http://codereview.chromium.org/3391008/diff/1/2#newcode487 bin/cros_image_to_target.py:487: cros_env.start_client(options.port) Why set remote earlier but not the port? http://codereview.chromium.org/3391008/diff/1/2#newcode490 bin/cros_image_to_target.py:490: return exit(0)? http://codereview.chromium.org/3391008/diff/1/2#newcode496 bin/cros_image_to_target.py:496: return sys.exit(1)
I've done gpylint, and responded to Sosa's comments. There's only one of them that I don't understand. http://codereview.chromium.org/3391008/diff/1/2 File bin/cros_image_to_target.py (right): http://codereview.chromium.org/3391008/diff/1/2#newcode16 bin/cros_image_to_target.py:16: __author__ = 'pstew@google.com (Paul Stewart)' On 2010/09/17 20:51:22, sosa wrote: > No authors in Chromium OS code. Done. http://codereview.chromium.org/3391008/diff/1/2#newcode28 bin/cros_image_to_target.py:28: class Command: On 2010/09/17 20:51:22, sosa wrote: > docstring at least for each class. Done. http://codereview.chromium.org/3391008/diff/1/2#newcode29 bin/cros_image_to_target.py:29: def __init__(self, env): On 2010/09/17 20:51:22, sosa wrote: > All indentation should be 2 sp Done. http://codereview.chromium.org/3391008/diff/1/2#newcode30 bin/cros_image_to_target.py:30: self.env = env On 2010/09/17 20:51:22, sosa wrote: > Line between. Done. http://codereview.chromium.org/3391008/diff/1/2#newcode31 bin/cros_image_to_target.py:31: def run_pipe(self, pipeline, infile=None, outfile=None, On 2010/09/17 20:51:22, sosa wrote: > RunPipe not run_pipe per code style. Done. http://codereview.chromium.org/3391008/diff/1/2#newcode37 bin/cros_image_to_target.py:37: if last_pipe is not None: On 2010/09/17 20:51:22, sosa wrote: > Switch all of these i.e. s/if var is not None:/if var:/ Not here. Python's mapping from object to boolean is opaque unless you're intimately familiar with all objects. I don't know the implicit len() or __int__() mapping for a subprocess.Popen object, and I don't intend to force anyone who reads my code to also know it. http://codereview.chromium.org/3391008/diff/1/2#newcode41 bin/cros_image_to_target.py:41: if len(pipeline) or capture: On 2010/09/17 20:51:22, sosa wrote: > if pipeline (does same thing) or capture: Done. http://codereview.chromium.org/3391008/diff/1/2#newcode72 bin/cros_image_to_target.py:72: self.remote = remote On 2010/09/17 20:51:22, sosa wrote: > It is preferred to have a Setup function than do work in _init_. It makes it > much easier to write unittests for the class since you don't have to mock out > the constructor. Done. http://codereview.chromium.org/3391008/diff/1/2#newcode132 bin/cros_image_to_target.py:132: self.cros_path('get_latest_image.sh')) On 2010/09/17 20:51:22, sosa wrote: > No ./ ? Nope. This script can be run from any directory, inside or outside the chroot. In addition, due to the long (possibly aborted?) move of scripts to the bin/ subdirectory of src/scripts, I have no idea where these executables will end up in the future. Therefore I added a function behind which I can hide that stuff. http://codereview.chromium.org/3391008/diff/1/2#newcode134 bin/cros_image_to_target.py:134: def cached(self, src, dst): On 2010/09/17 20:51:22, sosa wrote: > IsCached - methods should start with verbs Done. http://codereview.chromium.org/3391008/diff/1/2#newcode197 bin/cros_image_to_target.py:197: if lsb_release is None: On 2010/09/17 20:51:22, sosa wrote: > if not lsb_release: Done. http://codereview.chromium.org/3391008/diff/1/2#newcode230 bin/cros_image_to_target.py:230: time.sleep(5) On 2010/09/17 20:51:22, sosa wrote: > sleep constant? Done. http://codereview.chromium.org/3391008/diff/1/2#newcode440 bin/cros_image_to_target.py:440: if options.src is None: On 2010/09/17 20:51:22, sosa wrote: > if not options.src Done. http://codereview.chromium.org/3391008/diff/1/2#newcode441 bin/cros_image_to_target.py:441: parser.error("No --from argument given and no default image found") On 2010/09/17 20:51:22, sosa wrote: > Use consistent " vs. '. I suggest using ''s for all but docstrings. Done. http://codereview.chromium.org/3391008/diff/1/2#newcode447 bin/cros_image_to_target.py:447: image_directory = options.src On 2010/09/17 20:51:22, sosa wrote: > Use 2 space indents Done. http://codereview.chromium.org/3391008/diff/1/2#newcode448 bin/cros_image_to_target.py:448: image_file = os.path.join(options.src, 'chromiumos_image.bin') On 2010/09/17 20:51:22, sosa wrote: > Are you sure you want image rather than chromiumos_test_image.bin? May want an > option for image name. > > Also may want to make into a global constant Done. http://codereview.chromium.org/3391008/diff/1/2#newcode461 bin/cros_image_to_target.py:461: if rel is None: On 2010/09/17 20:51:22, sosa wrote: > if not rel Done. http://codereview.chromium.org/3391008/diff/1/2#newcode463 bin/cros_image_to_target.py:463: return On 2010/09/17 20:51:22, sosa wrote: > sys.exit(1) or better, add cros_env.Die. I suggest taking a look at the small > but common functions in src/scripts/lib/cros_build_lib.py Done. http://codereview.chromium.org/3391008/diff/1/2#newcode468 bin/cros_image_to_target.py:468: return On 2010/09/17 20:51:22, sosa wrote: > Ditto Done. http://codereview.chromium.org/3391008/diff/1/2#newcode474 bin/cros_image_to_target.py:474: update_file = os.path.join(image_directory, 'update.gz') On 2010/09/17 20:51:22, sosa wrote: > Make into a global constant. Done. http://codereview.chromium.org/3391008/diff/1/2#newcode475 bin/cros_image_to_target.py:475: stateful_file = os.path.join(image_directory, 'stateful.image.gz') On 2010/09/17 20:51:22, sosa wrote: > Make into a global constant. Done. http://codereview.chromium.org/3391008/diff/1/2#newcode479 bin/cros_image_to_target.py:479: return On 2010/09/17 20:51:22, sosa wrote: > Die with an error message Done. Error message is output by the callee. http://codereview.chromium.org/3391008/diff/1/2#newcode481 bin/cros_image_to_target.py:481: if not options.server_only: On 2010/09/17 20:51:22, sosa wrote: > Why start client before server? Seems like needless sleeping. The the time the fork takes to occur is miniscule. The server never returns, so the only opportunity we have to fork and start the client is before. http://codereview.chromium.org/3391008/diff/1/2#newcode486 bin/cros_image_to_target.py:486: time.sleep(1) On 2010/09/17 20:51:22, sosa wrote: > Make 1 into some constant. Done. http://codereview.chromium.org/3391008/diff/1/2#newcode487 bin/cros_image_to_target.py:487: cros_env.start_client(options.port) On 2010/09/17 20:51:22, sosa wrote: > Why set remote earlier but not the port? Could you restate this question? http://codereview.chromium.org/3391008/diff/1/2#newcode490 bin/cros_image_to_target.py:490: return On 2010/09/17 20:51:22, sosa wrote: > exit(0)? Whoops. We need to perform cleanup at the bottom. Changing this to a "finally" clause to make that more explicit. http://codereview.chromium.org/3391008/diff/1/2#newcode496 bin/cros_image_to_target.py:496: return On 2010/09/17 20:51:22, sosa wrote: > sys.exit(1) This is not necessarily an errored exit.
Thanks for the cleanup, mostly nits left. http://codereview.chromium.org/3391008/diff/9001/10001 File bin/cros_image_to_target.py (right): http://codereview.chromium.org/3391008/diff/9001/10001#newcode47 bin/cros_image_to_target.py:47: if last_pipe is not None: if last_pipe: http://codereview.chromium.org/3391008/diff/9001/10001#newcode49 bin/cros_image_to_target.py:49: elif infile is not None: elif infile: http://codereview.chromium.org/3391008/diff/9001/10001#newcode53 bin/cros_image_to_target.py:53: elif outfile is not None: elif outfile http://codereview.chromium.org/3391008/diff/9001/10001#newcode90 bin/cros_image_to_target.py:90: Command.__init__(self, env) This is generally done first rather than last in case you overwrite vars. http://codereview.chromium.org/3391008/diff/9001/10001#newcode119 bin/cros_image_to_target.py:119: os.unlink(self.known_hosts) Where is the original link? Why not rm? http://codereview.chromium.org/3391008/diff/9001/10001#newcode139 bin/cros_image_to_target.py:139: self.verbose = False Don't pass this in? http://codereview.chromium.org/3391008/diff/9001/10001#newcode157 bin/cros_image_to_target.py:157: return self.CrosPath(os.path.join('..', '..', 'chroot', Use relative root from os.path.abspath(sys.argv[0]) http://codereview.chromium.org/3391008/diff/9001/10001#newcode259 bin/cros_image_to_target.py:259: status = self.ssh_cmd.Output('/usr/bin/update_engine_client', '-status') Use --status to be more linux-y. Both work. http://codereview.chromium.org/3391008/diff/9001/10001#newcode287 bin/cros_image_to_target.py:287: """Ask the client machine to update from our server.""" May want to return True/False based on error condition. http://codereview.chromium.org/3391008/diff/9001/10001#newcode299 bin/cros_image_to_target.py:299: self.ssh_cmd.Run('/usr/bin/update_engine_client', '-update', Use -- vs - http://codereview.chromium.org/3391008/diff/9001/10001#newcode429 bin/cros_image_to_target.py:429: host, pdict = cgi.parse_header(handler.headers.getheader('Host')) You may want to consider parsing this some as you get a few of these and only one of them is important. The important request has a update request in the XML. That way you will only send the right ping once rather than each time. http://codereview.chromium.org/3391008/diff/9001/10001#newcode516 bin/cros_image_to_target.py:516: if options.remote is not None: if options.remote:
http://codereview.chromium.org/3391008/diff/9001/10001 File bin/cros_image_to_target.py (right): http://codereview.chromium.org/3391008/diff/9001/10001#newcode47 bin/cros_image_to_target.py:47: if last_pipe is not None: On 2010/09/18 07:57:31, sosa wrote: > if last_pipe: I'm not sure you saw my previous reply to this one: Not here. Python's mapping from object to boolean is opaque on trivial inspection unless you're intimately familiar with all objects. I don't know the implicit len() or __int__() mapping for a subprocess.Popen object, and I don't intend to force anyone who reads my code to also know it. http://codereview.chromium.org/3391008/diff/9001/10001#newcode49 bin/cros_image_to_target.py:49: elif infile is not None: On 2010/09/18 07:57:31, sosa wrote: > elif infile: Done. http://codereview.chromium.org/3391008/diff/9001/10001#newcode53 bin/cros_image_to_target.py:53: elif outfile is not None: On 2010/09/18 07:57:31, sosa wrote: > elif outfile Done. http://codereview.chromium.org/3391008/diff/9001/10001#newcode119 bin/cros_image_to_target.py:119: os.unlink(self.known_hosts) On 2010/09/18 07:57:31, sosa wrote: > Where is the original link? Why not rm? os.unlink() (also called os.remove()) refers to the UNIX "unlink" system call which unlinks a file from its parent directory. It has no direct relation to symlinks, if that's your question. Is this a question of why I'm using a python os library call vs doing "system('rm ...')"? http://codereview.chromium.org/3391008/diff/9001/10001#newcode139 bin/cros_image_to_target.py:139: self.verbose = False On 2010/09/18 07:57:31, sosa wrote: > Don't pass this in? I'll stop by and chat about what this means. http://codereview.chromium.org/3391008/diff/9001/10001#newcode157 bin/cros_image_to_target.py:157: return self.CrosPath(os.path.join('..', '..', 'chroot', On 2010/09/18 07:57:31, sosa wrote: > Use relative root from os.path.abspath(sys.argv[0]) We should also chat about this. This is finding the root of the chroot filesystem, relative to os.path.abspath(sys.argv[0]). What you're asking for is done (more or less) in CrosPath(). http://codereview.chromium.org/3391008/diff/9001/10001#newcode259 bin/cros_image_to_target.py:259: status = self.ssh_cmd.Output('/usr/bin/update_engine_client', '-status') On 2010/09/18 07:57:31, sosa wrote: > Use --status to be more linux-y. Both work. Done. http://codereview.chromium.org/3391008/diff/9001/10001#newcode287 bin/cros_image_to_target.py:287: """Ask the client machine to update from our server.""" On 2010/09/18 07:57:31, sosa wrote: > May want to return True/False based on error condition. Done.
LGTM, can you add tests before committing? If not, please explain how you tested the changes in the issue before committing. http://codereview.chromium.org/3391008/diff/9001/10001 File bin/cros_image_to_target.py (right): http://codereview.chromium.org/3391008/diff/9001/10001#newcode153 bin/cros_image_to_target.py:153: def CrosPath(self, filename): CrosUtils path to be more explicit? http://codereview.chromium.org/3391008/diff/9001/10001#newcode222 bin/cros_image_to_target.py:222: def ParseShvars(self, string): ParseShVars
Hey sosa. I've addressed some of your suggestions including the init of the CrosEnv with verbose flag and the parsing of the incoming XML in ping requests. I've also changed the mechanics of the client exit to use SIGCHLD instead of having the child kill its parent (always messy). Tests won't be ready by the time this thing is committed, but will do so before I ask for readability. http://codereview.chromium.org/3391008/diff/9001/10001 File bin/cros_image_to_target.py (right): http://codereview.chromium.org/3391008/diff/9001/10001#newcode153 bin/cros_image_to_target.py:153: def CrosPath(self, filename): On 2010/09/20 16:41:31, sosa wrote: > CrosUtils path to be more explicit? > Done. http://codereview.chromium.org/3391008/diff/9001/10001#newcode222 bin/cros_image_to_target.py:222: def ParseShvars(self, string): On 2010/09/20 16:41:31, sosa wrote: > ParseShVars Done. http://codereview.chromium.org/3391008/diff/9001/10001#newcode299 bin/cros_image_to_target.py:299: self.ssh_cmd.Run('/usr/bin/update_engine_client', '-update', On 2010/09/18 07:57:31, sosa wrote: > Use -- vs - Done. http://codereview.chromium.org/3391008/diff/9001/10001#newcode429 bin/cros_image_to_target.py:429: host, pdict = cgi.parse_header(handler.headers.getheader('Host')) On 2010/09/18 07:57:31, sosa wrote: > You may want to consider parsing this some as you get a few of these and only > one of them is important. The important request has a update request in the > XML. That way you will only send the right ping once rather than each time. Done. http://codereview.chromium.org/3391008/diff/9001/10001#newcode516 bin/cros_image_to_target.py:516: if options.remote is not None: On 2010/09/18 07:57:31, sosa wrote: > if options.remote: Done.
LGTM. I'd be happy to take a look at them when the tests are ready. |
