Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
890773003:
Adding the initial code for Omnibot multi-machine support (Closed)
|
Created:
5 years, 10 months ago by Mike Meade Modified:
5 years, 10 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding the initial code for Omnibot multi-machine support.
This cl contains the base code plus a "simple" hello world example test.
BUG=453679
Committed: https://crrev.com/abebda49c4269e96a48fe435272b3ddb6cccb942
Cr-Commit-Position: refs/heads/master@{#315080}
Patch Set 1 #
Total comments: 28
Patch Set 2 : Addressing initial comments #
Total comments: 79
Patch Set 3 : Fixing comments and reducing excess code. #Patch Set 4 : Doing a little refactoring and changes. #
Total comments: 4
Patch Set 5 : Fixing some silly subprocess code. #
Total comments: 12
Patch Set 6 : #Patch Set 7 : Fixed previous comments #Patch Set 8 : Changed the OTP format to be more descriptive #
Total comments: 8
Patch Set 9 : Fixing final remarks #Patch Set 10 : #Patch Set 11 : #
Messages
Total messages: 19 (5 generated)
mmeade@chromium.org changed reviewers: + maruel@google.com
https://codereview.chromium.org/890773003/diff/1/testing/legion/client_contro... File testing/legion/client_controller.py (right): https://codereview.chromium.org/890773003/diff/1/testing/legion/client_contro... testing/legion/client_controller.py:5: """The main client_controller code. FYI, we generally keep an empty line in between the module docstring and the other items. https://codereview.chromium.org/890773003/diff/1/testing/legion/client_contro... testing/legion/client_controller.py:30: def GetArgs(): Is there value in making it a function vs embedding in main() directly? https://codereview.chromium.org/890773003/diff/1/testing/legion/client_contro... testing/legion/client_controller.py:50: server = client_rpc_server.RPCServer() same https://codereview.chromium.org/890773003/diff/1/testing/legion/client_lib.py File testing/legion/client_lib.py (right): https://codereview.chromium.org/890773003/diff/1/testing/legion/client_lib.py... testing/legion/client_lib.py:4: import argparse In general we keep an empty line in between https://codereview.chromium.org/890773003/diff/1/testing/legion/client_lib.py... testing/legion/client_lib.py:19: SWARMING_DIR = os.path.join(THIS_DIR, '../../tools/swarming_client') if you plan this to work on windows, use os.path.join(). We generally use os.path.join() out of consistency. https://codereview.chromium.org/890773003/diff/1/testing/legion/client_lib.py... testing/legion/client_lib.py:42: global args not a fan https://codereview.chromium.org/890773003/diff/1/testing/legion/client_lib.py... testing/legion/client_lib.py:82: self._otp = str(uuid.uuid1()) eventually it'd be nice for this to be deterministic, e.g. the hash of all known states that makes this step idempotent. May not be feasible in practice. https://codereview.chromium.org/890773003/diff/1/testing/legion/client_lib.py... testing/legion/client_lib.py:126: def SetPriority(self, priority): Why not @priority.setter def priority(self, value): self._priority = value Ref: https://docs.python.org/2/library/functions.html#property and at that point, I'm not sure the methods are useful at all, I'm not a fan of accessors in python, this is not Java. :) https://codereview.chromium.org/890773003/diff/1/testing/legion/examples/hell... File testing/legion/examples/hello_world/client_test.py (right): https://codereview.chromium.org/890773003/diff/1/testing/legion/examples/hell... testing/legion/examples/hello_world/client_test.py:15: return 0 (I just like to do this for consistency) https://codereview.chromium.org/890773003/diff/1/testing/legion/examples/hell... testing/legion/examples/hello_world/client_test.py:18: main() sys.exit(main()) https://codereview.chromium.org/890773003/diff/1/testing/legion/examples/hell... File testing/legion/examples/hello_world/host_test.isolate (right): https://codereview.chromium.org/890773003/diff/1/testing/legion/examples/hell... testing/legion/examples/hello_world/host_test.isolate:10: ['os=="Linux"', { Why lock this to linux? https://codereview.chromium.org/890773003/diff/1/testing/legion/examples/hell... testing/legion/examples/hello_world/host_test.isolate:14: '--swarming-server=https://omnibot-swarming-server.appspot.com/', https:// and / are not necessary. https://codereview.chromium.org/890773003/diff/1/testing/legion/legion.isolate File testing/legion/legion.isolate (right): https://codereview.chromium.org/890773003/diff/1/testing/legion/legion.isolat... testing/legion/legion.isolate:1: # Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/890773003/diff/1/testing/legion/legion.isolat... testing/legion/legion.isolate:10: # Avoid using ./ here as it breaks the examples Remove, it's the defacto standard in isolate files.
https://codereview.chromium.org/890773003/diff/1/testing/legion/client_contro... File testing/legion/client_controller.py (right): https://codereview.chromium.org/890773003/diff/1/testing/legion/client_contro... testing/legion/client_controller.py:5: """The main client_controller code. On 2015/01/30 18:00:21, Marc-Antoine Ruel (Google) wrote: > FYI, we generally keep an empty line in between the module docstring and the > other items. Done. https://codereview.chromium.org/890773003/diff/1/testing/legion/client_contro... testing/legion/client_controller.py:30: def GetArgs(): On 2015/01/30 18:00:21, Marc-Antoine Ruel (Google) wrote: > Is there value in making it a function vs embedding in main() directly? Not explicitly. I was just trying to group by operation. I moved all of this code into main. https://codereview.chromium.org/890773003/diff/1/testing/legion/client_contro... testing/legion/client_controller.py:50: server = client_rpc_server.RPCServer() On 2015/01/30 18:00:21, Marc-Antoine Ruel (Google) wrote: > same Done. https://codereview.chromium.org/890773003/diff/1/testing/legion/client_lib.py File testing/legion/client_lib.py (right): https://codereview.chromium.org/890773003/diff/1/testing/legion/client_lib.py... testing/legion/client_lib.py:4: import argparse On 2015/01/30 18:00:21, Marc-Antoine Ruel (Google) wrote: > In general we keep an empty line in between Done. https://codereview.chromium.org/890773003/diff/1/testing/legion/client_lib.py... testing/legion/client_lib.py:19: SWARMING_DIR = os.path.join(THIS_DIR, '../../tools/swarming_client') On 2015/01/30 18:00:21, Marc-Antoine Ruel (Google) wrote: > if you plan this to work on windows, use os.path.join(). > We generally use os.path.join() out of consistency. Done. https://codereview.chromium.org/890773003/diff/1/testing/legion/client_lib.py... testing/legion/client_lib.py:42: global args On 2015/01/30 18:00:21, Marc-Antoine Ruel (Google) wrote: > not a fan Moved into the class. https://codereview.chromium.org/890773003/diff/1/testing/legion/client_lib.py... testing/legion/client_lib.py:82: self._otp = str(uuid.uuid1()) On 2015/01/30 18:00:21, Marc-Antoine Ruel (Google) wrote: > eventually it'd be nice for this to be deterministic, e.g. the hash of all known > states that makes this step idempotent. May not be feasible in practice. Maybe HOST_FQDN\CLIENT_NAME\SHA-1_LEGION_FILES? The only case that worries me is if the client is really late to start and attempts to connect with another host that was started on the same machine. I figured this way it acts as both an identifier and as a security token to keep tests from "crossing wires" https://codereview.chromium.org/890773003/diff/1/testing/legion/client_lib.py... testing/legion/client_lib.py:126: def SetPriority(self, priority): On 2015/01/30 18:00:21, Marc-Antoine Ruel (Google) wrote: > Why not > @priority.setter > def priority(self, value): > self._priority = value > > Ref: https://docs.python.org/2/library/functions.html#property > > and at that point, I'm not sure the methods are useful at all, I'm not a fan of > accessors in python, this is not Java. :) The only reason I added the method like this was for consistency with AddConfigVar and AddDimension. From the host controller's point of view the syntax is pretty consistent this way. I considered making the variables public and just letting the calling code use it directly. I can remove all of these and make public properties. https://codereview.chromium.org/890773003/diff/1/testing/legion/examples/hell... File testing/legion/examples/hello_world/client_test.py (right): https://codereview.chromium.org/890773003/diff/1/testing/legion/examples/hell... testing/legion/examples/hello_world/client_test.py:15: On 2015/01/30 18:00:21, Marc-Antoine Ruel (Google) wrote: > return 0 > > (I just like to do this for consistency) Done. https://codereview.chromium.org/890773003/diff/1/testing/legion/examples/hell... testing/legion/examples/hello_world/client_test.py:18: main() On 2015/01/30 18:00:21, Marc-Antoine Ruel (Google) wrote: > sys.exit(main()) Done. https://codereview.chromium.org/890773003/diff/1/testing/legion/examples/hell... File testing/legion/examples/hello_world/host_test.isolate (right): https://codereview.chromium.org/890773003/diff/1/testing/legion/examples/hell... testing/legion/examples/hello_world/host_test.isolate:10: ['os=="Linux"', { On 2015/01/30 18:00:21, Marc-Antoine Ruel (Google) wrote: > Why lock this to linux? Done. https://codereview.chromium.org/890773003/diff/1/testing/legion/examples/hell... testing/legion/examples/hello_world/host_test.isolate:14: '--swarming-server=https://omnibot-swarming-server.appspot.com/', On 2015/01/30 18:00:21, Marc-Antoine Ruel (Google) wrote: > https:// and / are not necessary. Done. https://codereview.chromium.org/890773003/diff/1/testing/legion/legion.isolate File testing/legion/legion.isolate (right): https://codereview.chromium.org/890773003/diff/1/testing/legion/legion.isolat... testing/legion/legion.isolate:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/30 18:00:21, Marc-Antoine Ruel (Google) wrote: > 2015 Done. https://codereview.chromium.org/890773003/diff/1/testing/legion/legion.isolat... testing/legion/legion.isolate:10: # Avoid using ./ here as it breaks the examples On 2015/01/30 18:00:21, Marc-Antoine Ruel (Google) wrote: > Remove, it's the defacto standard in isolate files. Done.
https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_co... File testing/legion/client_controller.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_co... testing/legion/client_controller.py:1: #!/usr/bin/env python Saved 19 lines, or 28% of lines, not too bad. :) IMO, the new version is more comprehensible. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... File testing/legion/client_lib.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:38: """The main client class. Few issues: - The class name is misleading, it's not one client. It's more akin a ClientsManager. - This first docstring line is information-less. Starts it with a verb about what it's doing. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:66: self._dimensions = [] this must be a dict, since duplicate keys are not acceptable. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:85: def _IncreaseCount(cls): Why a function if it is used exactly at one place? https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:98: def _ParseArgs(self): Why is this a method? https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:112: key: The config vars key. Frankly, I don't see the value of this documentation. It's pretty self-explanatory. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:117: self._config_vars.append((key, value)) The fact previous keys are not overridden IS something worth documenting. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:123: key: The dimension key. same https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:130: def Create(self, wait=False, timeout=None): Why options? Will there be code that will use all the combinations? https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:182: '--isolate=' + self.isolate_file, Trust me, that's a recipe for bugs. Use instead: '--isolate', self.isolate_file, and the like. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:187: cmd += ['--isolate-server', self._args.isolate_server] I generally prefer cmd.extend(...) https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:192: if subprocess.call(cmd, stdout=subprocess.PIPE) != 0: This may hang BTW since you are not reading stdout. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:222: def _RegisterOnConnectCallback(self): Same thing, it's used exactly at one place, I don't see value in making it a function, add a comment at the call site instead if needed but I think that: self._discovery_server.RegisterClientCallback(self._otp, self._OnConnect) is completely self-explanatory. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:225: This callback is used to receive the client's IP address once it starts What the callback does is not relevant documentation to this function. It should be commented on the callback itself. What the callback receives should be documented in RegisterClientCallback(). No need to repeat it elsewhere unless the arguments are illnamed due to historical reasons or something like that. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... File testing/legion/client_rpc_methods.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_methods.py:4: """Client RPC Methods. This is not informative. "client_rpc_methods.py" pretty much convey this information. :) https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_methods.py:38: p.wait() This will hang. You need to use .communicate() https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... File testing/legion/client_rpc_server.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:35: """A custom handler class that provides simple authentication. """Restricts access to only specified IP address. This call assumes server is RPCServer. """ This is short, simple, and explains much better. We know the context is a request because the class is named "Request". https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:42: """Report a 403 Forbidden error.""" Not necessary. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:45: self.send_header("Content-type", "text/plain") Did you copy-paste this from somewhere? It looks like. It's using different quotes. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:51: """Custom do_POST that verifies the client is authorized to perform RPCs.""" """Verifies the client is ...""" https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:63: """Custom RPC server that supports authorized client addresses.""" """Restricts all endpoints to only specified IP address.""" https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:65: def __init__(self, address=None, port=None): def __init__(self, *args, **kwargs): https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:66: """ctor. Remove all docstring. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:72: address = address or SERVER_ADDRESS I don't like that. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:75: self, (address, port), allow_none=True, logRequests=False, super(RPCServer, self).__init__( *args, allow_none=True, logRequests=False, requestHandler=RequestHandler, **kwargs) (unless it's an old style class, then keep the previous calling convention) https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:91: Args: Bof https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:96: def SetIdleTimeout(self, timeout): s/timeout/timeout_secs/ then you can strip the comment to a single line. but you can strip the comment even without changing the variable name, since "timeout" has a pretty standard meaning in python to imply "None or seconds in float" https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:106: def Connect(client, port=None): I'd prefer this to not be a static method, it's confusing https://codereview.chromium.org/890773003/diff/20001/testing/legion/common_li... File testing/legion/common_lib.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/common_li... testing/legion/common_lib.py:27: level = logging.DEBUG if args.verbose else logging.INFO (FYI) in generally we use ERROR as default. https://codereview.chromium.org/890773003/diff/20001/testing/legion/discovery... File testing/legion/discovery_server.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/discovery... testing/legion/discovery_server.py:40: cb = self._expected_clients[otp] You mean: cb = self._expected_clients.pop(otp) cb(ip) https://codereview.chromium.org/890773003/diff/20001/testing/legion/discovery... testing/legion/discovery_server.py:54: if not callable(callback): use an assert, it's not something normal. https://codereview.chromium.org/890773003/diff/20001/testing/legion/discovery... testing/legion/discovery_server.py:58: def Start(self, address=None, port=None): *args, **kwargs again https://codereview.chromium.org/890773003/diff/20001/testing/legion/discovery... testing/legion/discovery_server.py:65: address = address or SERVER_ADDRESS I don't like these to be sprinkled at multiple places. https://codereview.chromium.org/890773003/diff/20001/testing/legion/discovery... testing/legion/discovery_server.py:82: def Connect(host): What's the relationship with the class? Why is it a static method? https://codereview.chromium.org/890773003/diff/20001/testing/legion/examples/... File testing/legion/examples/hello_world/client_test.isolate (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/examples/... testing/legion/examples/hello_world/client_test.isolate:10: ['os=="Linux" or os=="Windows" or os=="Mac" and multi_machine == 1', { No need to check for os at all, just: ['multi_machine=1', { https://codereview.chromium.org/890773003/diff/20001/testing/legion/examples/... File testing/legion/examples/hello_world/host_test.isolate (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/examples/... testing/legion/examples/hello_world/host_test.isolate:10: ['os=="Linux" or os=="Windows" or os=="Mac"', { Remove lines 9, 10, 23, 24. https://codereview.chromium.org/890773003/diff/20001/testing/legion/examples/... File testing/legion/examples/hello_world/host_test.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/examples/... testing/legion/examples/hello_world/host_test.py:32: client.AddConfigVars('os', 'Linux') Why not pass all these in the constructor? It'd make the object more immutable and easier to manage. https://codereview.chromium.org/890773003/diff/20001/testing/legion/host_cont... File testing/legion/host_controller.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/host_cont... testing/legion/host_controller.py:37: def Test(self): I'd prefer if you used Task instead of Test. Sometimes it's not really running a "test". https://codereview.chromium.org/890773003/diff/20001/testing/legion/legion.is... File testing/legion/legion.isolate (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/legion.is... testing/legion/legion.isolate:6: 'conditions': [ Remove lines 6, 7, 22, 23.
Made some overall sweeping changes trying to keep your style recommendations in mind. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_co... File testing/legion/client_controller.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_co... testing/legion/client_controller.py:1: #!/usr/bin/env python On 2015/01/30 21:58:38, Marc-Antoine Ruel (Google) wrote: > Saved 19 lines, or 28% of lines, not too bad. :) > IMO, the new version is more comprehensible. Acknowledged. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... File testing/legion/client_lib.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:38: """The main client class. On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > Few issues: > - The class name is misleading, it's not one client. It's more akin a > ClientsManager. > - This first docstring line is information-less. Starts it with a verb about > what it's doing. Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:66: self._dimensions = [] On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > this must be a dict, since duplicate keys are not acceptable. Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:85: def _IncreaseCount(cls): On 2015/01/30 21:58:38, Marc-Antoine Ruel (Google) wrote: > Why a function if it is used exactly at one place? Changed it to type(self)._client_count += 1 in the __init__ method. Using a simple "self._client_count += 1" in __init__ results in the object getting the value rather than the class. This resulted in multiple Client1 objects. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:98: def _ParseArgs(self): On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > Why is this a method? Moved it into __init__ https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:112: key: The config vars key. On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > Frankly, I don't see the value of this documentation. It's pretty > self-explanatory. Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:117: self._config_vars.append((key, value)) On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > The fact previous keys are not overridden IS something worth documenting. I added a note, but should this also be a dict? https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:123: key: The dimension key. On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > same Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:130: def Create(self, wait=False, timeout=None): On 2015/01/30 21:58:38, Marc-Antoine Ruel (Google) wrote: > Why options? Will there be code that will use all the combinations? In thinking about it more this is probably over-engineered. It's probably cleaner just to have the calling code call Create() and WaitForConnection(timeout). https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:182: '--isolate=' + self.isolate_file, On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > Trust me, that's a recipe for bugs. Use instead: > '--isolate', self.isolate_file, > and the like. Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:187: cmd += ['--isolate-server', self._args.isolate_server] On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > I generally prefer cmd.extend(...) Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:192: if subprocess.call(cmd, stdout=subprocess.PIPE) != 0: On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > This may hang BTW since you are not reading stdout. Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:222: def _RegisterOnConnectCallback(self): On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > Same thing, it's used exactly at one place, I don't see value in making it a > function, add a comment at the call site instead if needed but I think that: > self._discovery_server.RegisterClientCallback(self._otp, self._OnConnect) > is completely self-explanatory. Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_li... testing/legion/client_lib.py:225: This callback is used to receive the client's IP address once it starts On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > What the callback does is not relevant documentation to this function. It should > be commented on the callback itself. > What the callback receives should be documented in RegisterClientCallback(). No > need to repeat it elsewhere unless the arguments are illnamed due to historical > reasons or something like that. Acknowledged. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... File testing/legion/client_rpc_methods.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_methods.py:4: """Client RPC Methods. On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > This is not informative. "client_rpc_methods.py" pretty much convey this > information. :) > Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_methods.py:38: p.wait() On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > This will hang. You need to use .communicate() Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... File testing/legion/client_rpc_server.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:35: """A custom handler class that provides simple authentication. On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > """Restricts access to only specified IP address. > > This call assumes server is RPCServer. > """ > > > This is short, simple, and explains much better. We know the context is a > request because the class is named "Request". Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:42: """Report a 403 Forbidden error.""" On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > Not necessary. Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:45: self.send_header("Content-type", "text/plain") On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > Did you copy-paste this from somewhere? It looks like. It's using different > quotes. Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:51: """Custom do_POST that verifies the client is authorized to perform RPCs.""" On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > """Verifies the client is ...""" Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:63: """Custom RPC server that supports authorized client addresses.""" On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > """Restricts all endpoints to only specified IP address.""" Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:65: def __init__(self, address=None, port=None): On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > def __init__(self, *args, **kwargs): I was able to completely get rid of all args. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:66: """ctor. On 2015/01/30 21:58:39, Marc-Antoine Ruel (Google) wrote: > Remove all docstring. Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:72: address = address or SERVER_ADDRESS On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > I don't like that. Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:75: self, (address, port), allow_none=True, logRequests=False, On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > super(RPCServer, self).__init__( > *args, allow_none=True, logRequests=False, requestHandler=RequestHandler, > **kwargs) > > (unless it's an old style class, then keep the previous calling convention) Yes its an old style class. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:91: Args: On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > Bof Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:96: def SetIdleTimeout(self, timeout): On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > s/timeout/timeout_secs/ > > then you can strip the comment to a single line. > but you can strip the comment even without changing the variable name, since > "timeout" has a pretty standard meaning in python to imply "None or seconds in > float" Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/client_rp... testing/legion/client_rpc_server.py:106: def Connect(client, port=None): On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > I'd prefer this to not be a static method, it's confusing Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/common_li... File testing/legion/common_lib.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/common_li... testing/legion/common_lib.py:27: level = logging.DEBUG if args.verbose else logging.INFO On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > (FYI) in generally we use ERROR as default. Unfortunately setting logging to error results in a swarming.py error if the host controller doesn't log an error. Triggered task: host_test/os=Linux/12c32ce015a1a2ac2b30f00fc4d65c3a55c0b1f8 ERROR 65767 swarming(619): No output found for task 255646b308400710 legion-bot-1: 255646b308400710 0 I had logging set to ERROR in that run. I can simply print the command line to make sure there is something in stdout. I also changed the parser so the user can simply pass --verbosity=INFO or any other level name to get that level of logging. The ClientController also supports this. https://codereview.chromium.org/890773003/diff/20001/testing/legion/discovery... File testing/legion/discovery_server.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/discovery... testing/legion/discovery_server.py:40: cb = self._expected_clients[otp] On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > You mean: > > cb = self._expected_clients.pop(otp) > cb(ip) Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/discovery... testing/legion/discovery_server.py:54: if not callable(callback): On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > use an assert, it's not something normal. Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/discovery... testing/legion/discovery_server.py:58: def Start(self, address=None, port=None): On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > *args, **kwargs again Changed this around. I hard coded the address and port to constants defined in common_lib. We can change this if needed but it doesn't seem like it right now. https://codereview.chromium.org/890773003/diff/20001/testing/legion/discovery... testing/legion/discovery_server.py:65: address = address or SERVER_ADDRESS On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > I don't like these to be sprinkled at multiple places. Moved all references to address and port to common_lib. https://codereview.chromium.org/890773003/diff/20001/testing/legion/discovery... testing/legion/discovery_server.py:82: def Connect(host): On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > What's the relationship with the class? Why is it a static method? Moved this and the other "Connect" method to common_lib. https://codereview.chromium.org/890773003/diff/20001/testing/legion/examples/... File testing/legion/examples/hello_world/client_test.isolate (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/examples/... testing/legion/examples/hello_world/client_test.isolate:10: ['os=="Linux" or os=="Windows" or os=="Mac" and multi_machine == 1', { On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > No need to check for os at all, just: > ['multi_machine=1', { Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/examples/... File testing/legion/examples/hello_world/host_test.isolate (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/examples/... testing/legion/examples/hello_world/host_test.isolate:10: ['os=="Linux" or os=="Windows" or os=="Mac"', { On 2015/01/30 21:58:41, Marc-Antoine Ruel (Google) wrote: > Remove lines 9, 10, 23, 24. Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/examples/... File testing/legion/examples/hello_world/host_test.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/examples/... testing/legion/examples/hello_world/host_test.py:32: client.AddConfigVars('os', 'Linux') On 2015/01/30 21:58:41, Marc-Antoine Ruel (Google) wrote: > Why not pass all these in the constructor? It'd make the object more immutable > and easier to manage. Done. https://codereview.chromium.org/890773003/diff/20001/testing/legion/host_cont... File testing/legion/host_controller.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/host_cont... testing/legion/host_controller.py:37: def Test(self): On 2015/01/30 21:58:41, Marc-Antoine Ruel (Google) wrote: > I'd prefer if you used Task instead of Test. Sometimes it's not really running a > "test". Good idea. I wasn't sure of the best name for it. I originally used Run but it seemed confusing as well. https://codereview.chromium.org/890773003/diff/20001/testing/legion/legion.is... File testing/legion/legion.isolate (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/legion.is... testing/legion/legion.isolate:6: 'conditions': [ On 2015/01/30 21:58:41, Marc-Antoine Ruel (Google) wrote: > Remove lines 6, 7, 22, 23. Done.
maruel@chromium.org changed reviewers: + maruel@chromium.org
minor things https://codereview.chromium.org/890773003/diff/20001/testing/legion/common_li... File testing/legion/common_lib.py (right): https://codereview.chromium.org/890773003/diff/20001/testing/legion/common_li... testing/legion/common_lib.py:27: level = logging.DEBUG if args.verbose else logging.INFO On 2015/02/03 01:18:09, Mike Meade wrote: > On 2015/01/30 21:58:40, Marc-Antoine Ruel (Google) wrote: > > (FYI) in generally we use ERROR as default. > > Unfortunately setting logging to error results in a swarming.py error if the > host controller doesn't log an error. > > Triggered task: host_test/os=Linux/12c32ce015a1a2ac2b30f00fc4d65c3a55c0b1f8 > ERROR 65767 swarming(619): No output found for task 255646b308400710 > legion-bot-1: 255646b308400710 0 > > I had logging set to ERROR in that run. I can simply print the command line to > make sure there is something in stdout. Ah! I can fix that later in the client code. It's an old historical reason that doesn't apply anymore. https://codereview.chromium.org/890773003/diff/60001/testing/legion/client_li... File testing/legion/client_lib.py (right): https://codereview.chromium.org/890773003/diff/60001/testing/legion/client_li... testing/legion/client_lib.py:170: '--priority', str(self._priority) I generally prefer to keep comma even on trailing item. This way if another item is added in a follow up, the previous line doesn't have to be touched. https://codereview.chromium.org/890773003/diff/60001/testing/legion/client_li... testing/legion/client_lib.py:185: '--idle-timeout', str(self._idle_timeout_secs) same https://codereview.chromium.org/890773003/diff/60002/testing/legion/client_li... File testing/legion/client_lib.py (right): https://codereview.chromium.org/890773003/diff/60002/testing/legion/client_li... testing/legion/client_lib.py:195: stderr.seek(0) Remove, just: raise Error(stderr) It's not a file like object, it's just a str instance. https://codereview.chromium.org/890773003/diff/60002/testing/legion/client_rp... File testing/legion/client_rpc_server.py (right): https://codereview.chromium.org/890773003/diff/60002/testing/legion/client_rp... testing/legion/client_rpc_server.py:122: # An RPC was received, reset the timeout IMHO I don't think the comment and the following one are necessary, the code is pretty clear. https://codereview.chromium.org/890773003/diff/60002/testing/legion/common_li... File testing/legion/common_lib.py (right): https://codereview.chromium.org/890773003/diff/60002/testing/legion/common_li... testing/legion/common_lib.py:13: LOGGING_FORMAT = ('%(asctime)s %(filename)s:%(lineno)s %(levelname)s] ' I'd probably wouldn't make it a named constant unless you plan to reuse it elsewhere. https://codereview.chromium.org/890773003/diff/60002/testing/legion/common_li... testing/legion/common_lib.py:30: logging_action = parser.add_argument('--verbosity', default='ERROR') I generally use instead action='count' then use it to index in: levels = [logging.ERROR, logging.INFO, logging.DEBUG] level = levels[min(options.verbosity, len(levels)-1)] So that the caller can just do: foo.py -vvv and it's simpler :) But I don't mind, just FYI. https://codereview.chromium.org/890773003/diff/60002/testing/legion/common_li... testing/legion/common_lib.py:39: def PrintCommandLine(): What's the use case? In particular I don't think the code inside is complex enough to not warrant just copy-pasting it. https://codereview.chromium.org/890773003/diff/60002/testing/legion/host_cont... File testing/legion/host_controller.py (right): https://codereview.chromium.org/890773003/diff/60002/testing/legion/host_cont... testing/legion/host_controller.py:54: except Exception, e: except Exception as e: everywhere
Also changed the otp format to be more deterministic. https://codereview.chromium.org/890773003/diff/60001/testing/legion/client_li... File testing/legion/client_lib.py (right): https://codereview.chromium.org/890773003/diff/60001/testing/legion/client_li... testing/legion/client_lib.py:170: '--priority', str(self._priority) On 2015/02/03 01:48:40, M-A Ruel wrote: > I generally prefer to keep comma even on trailing item. > This way if another item is added in a follow up, the previous line doesn't have > to be touched. Done. https://codereview.chromium.org/890773003/diff/60001/testing/legion/client_li... testing/legion/client_lib.py:185: '--idle-timeout', str(self._idle_timeout_secs) On 2015/02/03 01:48:40, M-A Ruel wrote: > same Done. https://codereview.chromium.org/890773003/diff/60002/testing/legion/client_li... File testing/legion/client_lib.py (right): https://codereview.chromium.org/890773003/diff/60002/testing/legion/client_li... testing/legion/client_lib.py:195: stderr.seek(0) On 2015/02/03 01:48:41, M-A Ruel wrote: > Remove, just: > raise Error(stderr) > > It's not a file like object, it's just a str instance. Done. https://codereview.chromium.org/890773003/diff/60002/testing/legion/client_rp... File testing/legion/client_rpc_server.py (right): https://codereview.chromium.org/890773003/diff/60002/testing/legion/client_rp... testing/legion/client_rpc_server.py:122: # An RPC was received, reset the timeout On 2015/02/03 01:48:41, M-A Ruel wrote: > IMHO I don't think the comment and the following one are necessary, the code is > pretty clear. Done. https://codereview.chromium.org/890773003/diff/60002/testing/legion/common_li... File testing/legion/common_lib.py (right): https://codereview.chromium.org/890773003/diff/60002/testing/legion/common_li... testing/legion/common_lib.py:13: LOGGING_FORMAT = ('%(asctime)s %(filename)s:%(lineno)s %(levelname)s] ' On 2015/02/03 01:48:41, M-A Ruel wrote: > I'd probably wouldn't make it a named constant unless you plan to reuse it > elsewhere. Done. https://codereview.chromium.org/890773003/diff/60002/testing/legion/common_li... testing/legion/common_lib.py:30: logging_action = parser.add_argument('--verbosity', default='ERROR') On 2015/02/03 01:48:41, M-A Ruel wrote: > I generally use instead action='count' then use it to index in: > levels = [logging.ERROR, logging.INFO, logging.DEBUG] > level = levels[min(options.verbosity, len(levels)-1)] > > So that the caller can just do: > foo.py -vvv > and it's simpler :) > > But I don't mind, just FYI. Acknowledged. https://codereview.chromium.org/890773003/diff/60002/testing/legion/common_li... testing/legion/common_lib.py:39: def PrintCommandLine(): On 2015/02/03 01:48:41, M-A Ruel wrote: > What's the use case? In particular I don't think the code inside is complex > enough to not warrant just copy-pasting it. Done. https://codereview.chromium.org/890773003/diff/60002/testing/legion/host_cont... File testing/legion/host_controller.py (right): https://codereview.chromium.org/890773003/diff/60002/testing/legion/host_cont... testing/legion/host_controller.py:54: except Exception, e: On 2015/02/03 01:48:41, M-A Ruel wrote: > except Exception as e: > > everywhere Old habits die hard. Not sure why I did this.
lgtm with nits https://codereview.chromium.org/890773003/diff/120001/testing/legion/client_c... File testing/legion/client_controller.py (right): https://codereview.chromium.org/890773003/diff/120001/testing/legion/client_c... testing/legion/client_controller.py:29: parser.add_argument('--otp') I'd prefer an help string just to succinctly explain what each is for, even if the script is meant to be used in an automated way. Nothing overly verbose, just a one liner. https://codereview.chromium.org/890773003/diff/120001/testing/legion/client_l... File testing/legion/client_lib.py (right): https://codereview.chromium.org/890773003/diff/120001/testing/legion/client_l... testing/legion/client_lib.py:120: creation_time = datetime.datetime.now().strftime('%H:%M:%S.%f') I prefer UTC everywhere; datetime.datetime.utcnow() https://codereview.chromium.org/890773003/diff/120001/testing/legion/client_r... File testing/legion/client_rpc_methods.py (right): https://codereview.chromium.org/890773003/diff/120001/testing/legion/client_r... testing/legion/client_rpc_methods.py:21: return 'echo ' + message I'd recommend return 'echo %s' % message in case someone passes junk in. https://codereview.chromium.org/890773003/diff/120001/testing/legion/examples... File testing/legion/examples/hello_world/host_test.py (right): https://codereview.chromium.org/890773003/diff/120001/testing/legion/examples... testing/legion/examples/hello_world/host_test.py:35: dimensions={'os': 'Linux', 'pool': 'legion'}, priority=0, Actually, this will use the highest priority, you want to keep it to 200 or something and it will be bumped automatically.
https://codereview.chromium.org/890773003/diff/120001/testing/legion/client_c... File testing/legion/client_controller.py (right): https://codereview.chromium.org/890773003/diff/120001/testing/legion/client_c... testing/legion/client_controller.py:29: parser.add_argument('--otp') On 2015/02/03 18:50:24, M-A Ruel wrote: > I'd prefer an help string just to succinctly explain what each is for, even if > the script is meant to be used in an automated way. Nothing overly verbose, just > a one liner. Done. https://codereview.chromium.org/890773003/diff/120001/testing/legion/client_l... File testing/legion/client_lib.py (right): https://codereview.chromium.org/890773003/diff/120001/testing/legion/client_l... testing/legion/client_lib.py:120: creation_time = datetime.datetime.now().strftime('%H:%M:%S.%f') On 2015/02/03 18:50:24, M-A Ruel wrote: > I prefer UTC everywhere; datetime.datetime.utcnow() Done. https://codereview.chromium.org/890773003/diff/120001/testing/legion/client_r... File testing/legion/client_rpc_methods.py (right): https://codereview.chromium.org/890773003/diff/120001/testing/legion/client_r... testing/legion/client_rpc_methods.py:21: return 'echo ' + message On 2015/02/03 18:50:24, M-A Ruel wrote: > I'd recommend return 'echo %s' % message in case someone passes junk in. Done. https://codereview.chromium.org/890773003/diff/120001/testing/legion/examples... File testing/legion/examples/hello_world/host_test.py (right): https://codereview.chromium.org/890773003/diff/120001/testing/legion/examples... testing/legion/examples/hello_world/host_test.py:35: dimensions={'os': 'Linux', 'pool': 'legion'}, priority=0, On 2015/02/03 18:50:24, M-A Ruel wrote: > Actually, this will use the highest priority, you want to keep it to 200 or > something and it will be bumped automatically. Done.
The CQ bit was checked by mmeade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/890773003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by mmeade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/890773003/180001
Message was sent while issue was closed.
Committed patchset #11 (id:180001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/abebda49c4269e96a48fe435272b3ddb6cccb942 Cr-Commit-Position: refs/heads/master@{#315080} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
