Gilad Arnold has uploaded a new change for review.
Change subject: autotest: end-to-end autoupdate test
......................................................................
autotest: end-to-end autoupdate test
This implements an autotest test for the Chrome OS autoupdate
functionality. It receives a single test configuration and performs an
end-to-end test of the update process specified by it, which includes:
* Initializing the DUT to a known start state (source version)
* Spawning a local Omaha/devserver instance.
* Triggering an update process.
* Tracking the progress of the update.
* Rebooting the DUT after the update.
* Ensuring that the update has succeeded (target version running).
BUG=chromium-os:33760,chromium-os:33761,chromium-os:33762
TEST=Running with run_remote_tests.sh works
Change-Id: I215e8214d3239f525f062a27a4d97806451c9736
---
M client/common_lib/cros/autoupdater.py
M client/common_lib/cros/dev_server.py
M client/common_lib/site_utils.py
A server/site_tests/autoupdate_EndToEndTest/autoupdate_EndToEndTest.py
A server/site_tests/autoupdate_EndToEndTest/control
5 files changed, 839 insertions(+), 3 deletions(-)
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 2: (21 inline comments)
I haven't gotten to the event verifying stuff yet, but I think these comments are enough to be going on with.
....................................................
File client/common_lib/cros/dev_server.py
Line 180: devserver = devservers.pop(hash_index)
Why did you change the semantics here? We don't want to pop the server off the list unless it's not up.
Line 295: url_dict['mton_payload'] = ('%s/static/%s/au/%s_%s/update.gz' %
We spent a lot of effort getting "magic" format strings like this out of the code and unified into global_config.ini; see image_url_pattern and package_url_pattern
Line 299: return url_dict
Why are you creating these strings and returning them here? You can do this outside this method, and it would be less magical. There's no reason for this code to understand that "image" has "release" buried in it.
Line 348: (self.url(), image_dir, 'chromiumos_test_image.bin'))
This is similarly problematic as the above.
....................................................
File client/common_lib/site_utils.py
Line 142: result = base_utils.run(gs_cmd).stdout.split('\n')
why aren't you using system_output, here?
Line 145: return [path.rstrip() for path in result if path]
I think a bare newline in a string evaluates to True, so you're not skipping newlines here. Also, there is a more elegant way to split and tear out the newlines.
result = base_utils.system_output(gs_cmd).splitlines()
now, bare newlines in the output will be empty strings in result, and your list expression will do the right thing.
....................................................
File server/site_tests/autoupdate_EndToEndTest/autoupdate_EndToEndTest.py
Line 18: def _secs_to_hms(secs):
Python has many utilities for formatting times and such http://docs.python.org/2/library/time.html#module-time
You really shouldn't need to roll your own.
Line 245: # Timeout periods, given in seconds.
We usually like the constant names to have the units in them.
_WAIT_AFTER_SHUTDOWN_SECONDS, for example. Improves readability
Line 270: """Compute a unique IP port to bind a private Omaha/devserver to.
Why not just generate a random number in the right range, try it, and if it fails try another? Does trying take a long time?
Line 306: host.servo.set('usb_mux_sel1', 'servo_sees_usbkey')
These constants aren't stored somewhere in the Servo module/class? Shouldn't they be?
Line 331: host.servo.power_long_press()
why do you do a long-press? It seems like a short press (graceful shutdown) would be fine. Can you explain in a comment?
Line 333: if intermediate_callback:
This callback mechanism feels overengineered. Just wrap the 'shutdown' and 'power on' logic into two separate methods and then do
self._shutdown_and_wait(host)
self.<whatever the callback would be>
self._power_on_and_wait(host, is_dev_mode)
It's much less confusing.
Line 342: # TODO(garnold) implement waiting for MP-signed images; ideas include
Always good to capture a significant TODO in crosbug, and point to it. This goes for most TODOs in the CL, not just this one
Line 379: # probably want to use a common library call for that.
Why not do that refactor in an initial CL, then use the refactored code in this CL?
Line 382: host.servo.install_recovery_image(lorry_image_url)
wait...why does install_revovery_image() behave differently here and above? How does it know to be different?
Line 420: updater.trigger_update()
2 lines between functions
Line 437: 'image.zip')[0].strip('/')
This is confusing to read :-/
All this code makes a lot of assumptions about URL formats. That makes the code brittle; one of the first things we did when paying back the technical debt from the old lab setup was remove these kinds of assumptions from the code base. I think you need to avoid adding them here.
Line 508: lorry_devserver = dev_server.ImageServer.resolve(0)
Perhaps take this up with Sosa before trying to submit this CL?
Line 519: # Launch Omaha/devserver.
I think all this logic around the Omaha/devserver needs to be wrapped up in its own, unittestable class.
....................................................
File server/site_tests/autoupdate_EndToEndTest/control
Line 61: servo_args = hosts.SiteHost.get_servo_arguments(args_dict)
What're you using servo for?
Line 68: test_conf[key] = args_dict.get(key) or locals().get(key)
You won't be passing in any command line arguments.
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 2: (21 inline comments)
Thanks for reviewing
....................................................
File client/common_lib/cros/dev_server.py
Line 180: devserver = devservers.pop(hash_index)
The list is unshared and does not escape the scope of this function, hence the code change is semantics preserving and more elegant.
Line 295: url_dict['mton_payload'] = ('%s/static/%s/au/%s_%s/update.gz' %
I'll change it.
That said, not clear why putting it in global_config is better: (a) URL mapping is done by devserver, and so the knowledge should be encapsulated in the devserver abstraction layer (namely, this module). If we want to keep the module implementation clean, we can have the devserver return a set of URLs or a URL recipe in its response. (b) the URL patterns in global_config do not encode what values are represented by which placeholders, so whoever uses them has to have apriori knowledge on what's represented. We can improve this by using named placeholders in the global_config templates, for example: %(netloc)s/update/%(build)s
Line 299: return url_dict
The purpose of this module is to abstract away the devserver internal logic. Why would I want to deal with this magic outside the scope of this module?
Line 348: (self.url(), image_dir, 'chromiumos_test_image.bin'))
See above. Knowledge of devserver internal mappings should be encapsulated.
....................................................
File client/common_lib/site_utils.py
Line 142: result = base_utils.run(gs_cmd).stdout.split('\n')
So advised by sosa. Looks equivalent to me---?
Line 145: return [path.rstrip() for path in result if path]
You missed the split('\n') above. Will replace it with splitlines() though.
....................................................
File server/site_tests/autoupdate_EndToEndTest/autoupdate_EndToEndTest.py
Line 18: def _secs_to_hms(secs):
The closest I found was str() of datetime.timedelta, but that doesn't give me what I want and will need to be further massaged.
Line 245: # Timeout periods, given in seconds.
Done
Line 270: """Compute a unique IP port to bind a private Omaha/devserver to.
We want DUTs running an MP-signed image to infer the devserver's port number independently, based on local state only.
Line 306: host.servo.set('usb_mux_sel1', 'servo_sees_usbkey')
Remnant of historical code. Removed.
Line 331: host.servo.power_long_press()
Using brute force to be on the safe side. I vaguely remember short presses performed inconsistently in my early tests (possibly after recovery).
Line 333: if intermediate_callback:
Removed.
Line 342: # TODO(garnold) implement waiting for MP-signed images; ideas include
Done
Line 379: # probably want to use a common library call for that.
Removing comment. Code diverged.
Line 382: host.servo.install_recovery_image(lorry_image_url)
The first one installs a recovery image. This one boots a test image (via recovery) and runs chromeos-install. We call either of them based on the test configuration. Clarifying in comments.
Line 420: updater.trigger_update()
Done
Line 437: 'image.zip')[0].strip('/')
The only assumption made is that a test image is contained in image.zip. I would have loved to change that but unfortunately it'll break a gazillion other pieces of infrastructure that depend on this invariant.
Line 508: lorry_devserver = dev_server.ImageServer.resolve(0)
Under investigation ;-)
Line 519: # Launch Omaha/devserver.
Moved to separate class. Did not add unittests; this is merely starting a subprocess.
....................................................
File server/site_tests/autoupdate_EndToEndTest/control
Line 61: servo_args = hosts.SiteHost.get_servo_arguments(args_dict)
Currently for installing a source image and rebooting. In the future it'll be used for manipulating GPIO signals, resetting the TPM version and switching to/from dev firmware.
Line 68: test_conf[key] = args_dict.get(key) or locals().get(key)
What do you mean? They are being passed when I use it with run_remote_tests.
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 2: (1 inline comment)
....................................................
File server/site_tests/autoupdate_EndToEndTest/autoupdate_EndToEndTest.py
Line 508: lorry_devserver = dev_server.ImageServer.resolve(0)
You should use the load balancing mechanism and not force this onto the same one perhaps target_payload_lorry_url
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 2: (8 inline comments)
....................................................
File client/common_lib/cros/dev_server.py
Line 295: url_dict['mton_payload'] = ('%s/static/%s/au/%s_%s/update.gz' %
If we could have used named placeholders, we would have. but it fails to parse:-/
The reason we gathered all the formatting strings in one place was so that, when someone inevitably wants to change the convention of URL construction we can just handle it all in the config. This dev_server module didn't really exist when we made that choice, so (for example) the URL format for packages staged by the dev server is already over there. Please keep things consistent, and if we want to revisit the decision we can do so for ALL these kinds of format strings later.
Line 299: return url_dict
Then pass image and release in separately. This code shouldn't make assumptions about the format of 'image'
....................................................
File client/common_lib/site_utils.py
Line 142: result = base_utils.run(gs_cmd).stdout.split('\n')
Why are you using a lower-level primitive when a higher-level primitive exists that will give you the output you want, and is used throughout the rest of the code more frequently -- and is therefore more familiar to someone else who might ahve to work with your code in the future?
....................................................
File server/site_tests/autoupdate_EndToEndTest/autoupdate_EndToEndTest.py
Line 18: def _secs_to_hms(secs):
It's really not worth adding this complexity to go from "02:02:30" to "2h2m30s", is it? Also, if you're taking an argument in seconds, what is this about milliseconds?
Line 270: """Compute a unique IP port to bind a private Omaha/devserver to.
Explain that in the comments.
Line 331: host.servo.power_long_press()
I might ask richard if he thinks this is really the best way.
Line 437: 'image.zip')[0].strip('/')
I just mean the slicing and dicing of this URL to get the values that you want. If there's slicing and dicing to be done, do it all up front and then pass the right values to your methods. At least then the assumptions are all pulled together in one place.
....................................................
File server/site_tests/autoupdate_EndToEndTest/control
Line 68: test_conf[key] = args_dict.get(key) or locals().get(key)
Right, which isn't the way this is going to be used in production. You need to build this to support the intended mechanism of use, as opposed to just the method you're using to test it at your desk.
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 2: (8 inline comments)
Another patchset coming as soon as I manage to get a successful run again...
....................................................
File client/common_lib/cros/dev_server.py
Line 299: return url_dict
Agreed. Done.
....................................................
File client/common_lib/site_utils.py
Line 142: result = base_utils.run(gs_cmd).stdout.split('\n')
I was not aware of it. In hindsight, it has a counterintuitive name, does redundant processing, and elevates the abstraction by handing me the .stdout of the output of the presumed lower-level call. I'll change it, of course.
....................................................
File server/site_tests/autoupdate_EndToEndTest/autoupdate_EndToEndTest.py
Line 18: def _secs_to_hms(secs):
I'll just print the raw seconds and spare everyone from the extra complexity.
Line 270: """Compute a unique IP port to bind a private Omaha/devserver to.
Done
Line 331: host.servo.power_long_press()
He did the same in platform_InstallTestImage.
Line 437: 'image.zip')[0].strip('/')
There's only one value that's being extracted here, and it's assigned to source_image_uri_path and used as such thereafter. It just so happens that I need 5 different operations to obtained this value. There's only one assumption that's being made and it's stated in the comment. Do you want me to break it into multiple assignments? Name intermediate values? Something else?
Line 508: lorry_devserver = dev_server.ImageServer.resolve(0)
This specific value won't work because we need to get it from the devserver ;-) I'll use target_payload_uri, which should be unique for each of the test configs executed by this test case for each release.
....................................................
File server/site_tests/autoupdate_EndToEndTest/control
Line 68: test_conf[key] = args_dict.get(key) or locals().get(key)
I want to be able to run it via AFE as well as run_remote_tests. This code will work with either (or so I think). Where is the problem?
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 2: (3 inline comments)
....................................................
File client/common_lib/cros/dev_server.py
Line 180: devserver = devservers.pop(hash_index)
This is fine thought tecnically it does escape the stop as this is not a copy so you are modifying the array you are getting
Line 299: return url_dict
ALso please do not return dicts here. Return the tuple of args. No other methods returns dicts so its really strange to do it here for your use case.
Line 348: (self.url(), image_dir, 'chromiumos_test_image.bin'))
global_config.
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 2: (4 inline comments)
Thanks for reviewing, please see replies
....................................................
File client/common_lib/cros/dev_server.py
Line 180: devserver = devservers.pop(hash_index)
Not sure I understand what you mean. My understanding is that cls.devservers() returns a fresh list instance on each invocation. If that wasn't the case then obviously we would have been changing the semantics, but it's also indicative of a broken abstraction.
Line 295: url_dict['mton_payload'] = ('%s/static/%s/au/%s_%s/update.gz' %
I don't disagree and I've already changed this. My point is that changes to the URL conventions are unlikely to be accounted for by changes to patterns in the config alone, and that corresponding code changes will be inevitable. We may want to consider improving the encapsulation.
Line 299: return url_dict
Not getting the point. I want callers to be able to extract URLs for particular artifacts, such as the delta payloads (in my case). I would like to index these URLs by some descriptive key, instead of bundling them in a generic tuple.
If the dictionary is a problem, I can define a class for that and return an instance, although I don't really see the advantage. What do you think?
Line 348: (self.url(), image_dir, 'chromiumos_test_image.bin'))
Done
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 2: (6 inline comments)
....................................................
File client/common_lib/cros/autoupdater.py
Line 175: @return True upon success.
This boolean is meaningless. You raise an exception on failure, no need to return True on Success.
Line 182: logging.info('triggering update via: %s' % autoupdate_cmd)
s/%/,
Line 184: self._run(autoupdate_cmd, timeout=30)
Timeout seems a little long
....................................................
File client/common_lib/cros/dev_server.py
Line 180: devserver = devservers.pop(hash_index)
I was just disagreeing with what you said in your comment as it relies on cmasone knowing the implementation of cls.servers() which isn't in itself intuitive.
Line 299: return url_dict
I'd prefer a return object. That more closely follows the norms in autotest. This way you avoid things like:
I have this var I care about called arg which can be None or some string.
Let's say in my caller code I do this:
if dict.get('mistyped_arg')
dance()
This will always dance not because arg is None but because you mistyped arg. Return objects do not have this problem.
Or even worse, you did it right the first time but someone renames arg to arg_new and doesn't update your code. You prob won't notice for a while.
Dicts are usually fine with non-library code for passing around your own args. But if you're encapsulating some behavior in a library, objects are better for this reason.
....................................................
File client/common_lib/site_utils.py
Line 142: result = base_utils.run(gs_cmd).stdout.split('\n')
What cmasone said. The way I deal with autotest is the following -- look up the func I think I want in base_util or other utils module. Scan the rest of the module for higher-level methods. Use those if they make sense.
Sorry I pointed you down the wrong direction, but alas I do not have the API of autotest memorized completely.
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 2: (4 inline comments)
I'll upload the patchset as soon as I get the test to run successfully again.
....................................................
File client/common_lib/cros/autoupdater.py
Line 175: @return True upon success.
Done
Line 182: logging.info('triggering update via: %s' % autoupdate_cmd)
Done
Line 184: self._run(autoupdate_cmd, timeout=30)
Reduced to 10, similar to what's used in reset_stateful_partition().
....................................................
File client/common_lib/cros/dev_server.py
Line 299: return url_dict
I agree. Trying to generalize the lesson: an object is better suited for encapsulating data aggregates because it doesn't have the equivalent of a get() operation ;-)
I'll change the code. My current view is that in this case it should be okay to build an ad hoc object by simply assigning values to fields directly, and get away with not defining a class properly... please let me know if this is unacceptable or bad practice.
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 4: (4 inline comments)
....................................................
File client/common_lib/cros/autoupdater.py
Line 182: self._run(autoupdate_cmd, timeout=10)
why are you specifying 10 here? Is there a specific reason the default doesn't work? If so you need to add a comment here to reflect that.
....................................................
File client/common_lib/cros/dev_server.py
Line 309: """A container for URLs of staged artifacts."""
define the variables that should be set in __init__(self): Pylint should complain
Line 386: def trigger_test_image_download(self, image_dir, board=None, release=None,
Why are these optional? Why would you ever not want these?
....................................................
File client/common_lib/site_utils.py
Line 144: return []
Return None.
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 4: (4 inline comments)
See follow up response (site_utils.py). Thanks.
....................................................
File client/common_lib/cros/autoupdater.py
Line 182: self._run(autoupdate_cmd, timeout=10)
I'll add a comment that this is an async operation and so we just need a very short timeout.
....................................................
File client/common_lib/cros/dev_server.py
Line 309: """A container for URLs of staged artifacts."""
Couldn't get the pylint wrapper to run... Fixing.
Line 386: def trigger_test_image_download(self, image_dir, board=None, release=None,
When the caller doesn't care about the staged URL. Done this way to retain consistency with trigger_download() above, which in turn was done to preserve backward compatibility with existing codebase.
....................................................
File client/common_lib/site_utils.py
Line 144: return []
Why? If the point is to return a distinct value on a technical error, then I might as well just let the exception bubble up. What do you think?
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 4: (1 inline comment)
....................................................
File client/common_lib/site_utils.py
Line 144: return []
1) None is the default Python return. Usually unless I clearly see a reason to use something else, I don't deviate from it.
2) I'd probably just let the error bubble up. Id' rather know if the URL was wrong (a config issue) vs. it being empty -- which is different but maybe requires the same action.
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 6: (16 inline comments)
....................................................
File client/common_lib/cros/dev_server.py
Line 358: urls = self.ArtifactUrls()
Of course this is bad practice. Now I have to read the implementation details of your code in order to extract values from an ArtifactUrls.
I understand that people like to do this in python, but it makes for very hard to read code. If it's internal to your module, maybe it's OK. As a part of the API, this is a bad idea.
Line 390: branch=None):
Who cares about backward compatibility? If you change the API, update the callsites. Perhaps do this in an initial CL that this one depends on.
....................................................
File server/site_tests/autoupdate_EndToEndTest/autoupdate_EndToEndTest.py
Line 30: 'event_type', 'event_result', 'version', 'previous_version']
Since you're creating this encapsulation, why aren't you creating meaningful values for the event types and results? passing in '13' and '2' and stuff is really mysterious.
Line 33: self._expected_attrs = expected_attrs
Please specify the arguments, here. You can give them all the default value None, allowing callers to override only the ones they want. You can store them in a dict that you iterate through in verify, instead of having this global list of constants.
Line 51: for attr_name in self.ATTR_NAME_LIST])
why not just extract the key and value here and pass them in, instead of passing the name and then using it to extract the value inside _verify_attr()?
Line 77: self._expected_event_chain = expected_event_chain_args
You're just passing a dict, right? Why not take an argument that is a dict, and document it?
Line 96: return '%s' % self._expected_event_chain
I'd just say 'str(self._expected_event_chain)'
Line 172: len(self._event_log) > self._num_consumed_events):
Aren't these clauses redundant? if self._event_log is [], the len of it will always be <= self._num_consumed_events.
Line 229: # own IP address, or otherwise provisioned to it.
What's all this about? Is there a bug tracking the doing of this work so I can read more? if not, why not?
Line 254: raise error.TestError('omaha/devserver not running')
would that stderr output you captured in "line" above be helpful to understand why this happened?
Line 258: @staticmethod
Line 269: @staticmethod
Line 356: @param intermediate_callback: executed after shutdown and before
update comment; explaining why one might use 'is_disable_usb_hub' as well.
Line 561: logging.error('failed to start omaha/devserver: %s' % str(e))
, instead of %
Line 628: # TODO(garnold) for some reason, previous version is not reported
is there a bug tracking that fix?
....................................................
File server/site_tests/autoupdate_EndToEndTest/control
Line 70: test_conf[key] = args_dict.get(key) or locals().get(key)
The problem is that you're adding complexity to support a use case that is used only by you at your desk, and you're only using it because you haven't yet built the proper infrastructure to test your code.
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 6: (1 inline comment)
....................................................
File client/common_lib/cros/dev_server.py
Line 358: urls = self.ArtifactUrls()
You have to read the code anyway -- even if your using Python help etc to read the docstring. This is versus a tuple return or a dict which I really didn't like.
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 6: (1 inline comment)
....................................................
File client/common_lib/cros/dev_server.py
Line 358: urls = self.ArtifactUrls()
The choice is not a dict/tuple vs an object that you treat as a dict by creating arbitrary fields in it at runtime.
I'm asking for a class that has actual members in it that are documented in that class' docstrings.
Change subject: autotest: end-to-end autoupdate test
......................................................................
Patch Set 6: (16 inline comments)
....................................................
File client/common_lib/cros/dev_server.py
Line 358: urls = self.ArtifactUrls()
The __init__ method already initializes the data members. I'll add a docstring.
Line 390: branch=None):
I don't see the point of changing every call site to compute and feed in board/release/branch values for nothing.
....................................................
File server/site_tests/autoupdate_EndToEndTest/autoupdate_EndToEndTest.py
Line 30: 'event_type', 'event_result', 'version', 'previous_version']
Done
Line 33: self._expected_attrs = expected_attrs
Done
Line 51: for attr_name in self.ATTR_NAME_LIST])
Done
Line 77: self._expected_event_chain = expected_event_chain_args
It's a list. More convenient.
Line 96: return '%s' % self._expected_event_chain
Done
Line 172: len(self._event_log) > self._num_consumed_events):
_event_log used to be initialized to None at some point. Fixed.
Line 229: # own IP address, or otherwise provisioned to it.
This only supports spawning the private devserver on localhost (the server machine that's running the test). I'm assuming that we may want to be able to run it on arbitrary machines in the lab, in the future, hence the TODO. No tracking bug, I still need to figure out what's the intended infrastructure support for this.
Line 254: raise error.TestError('omaha/devserver not running')
Might be. I'll logging.error it.
Line 258: Done
Line 269: Done
Line 356: @param intermediate_callback: executed after shutdown and before
Done
Line 561: logging.error('failed to start omaha/devserver: %s' % str(e))
Done
Line 628: # TODO(garnold) for some reason, previous version is not reported
Did not verify that this is a bug. Removing the TODO notation, this remains as a reminder for the curious reader.
....................................................
File server/site_tests/autoupdate_EndToEndTest/control
Line 70: test_conf[key] = args_dict.get(key) or locals().get(key)
You expect anyone who wants to run this test to have a local AFE running?
