Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2237403002:
Add cloudtail start/stop in recipe_modules/goma (Closed)
DescriptionAdd cloudtail start/stop in recipe_modules/goma
* add cloudtail_utils.py in resources directory
BUG=544330
Committed: https://chromium.googlesource.com/chromium/tools/build/+/2c034e723e150b21ed4ddcd346e3d9746e4c622a
Patch Set 1 #Patch Set 2 : use raw_io instead of json #
Total comments: 18
Patch Set 3 : do waitpid, add todo for cipd cloudtail #Patch Set 4 : modify help #Patch Set 5 : remove --log-id and --filepath from cloudtail_utils.py #Patch Set 6 : remove unused code #Patch Set 7 : remove import os #
Total comments: 4
Patch Set 8 : remove --log-id, sleep 3 seconds #
Total comments: 8
Patch Set 9 : generate *.json #
Total comments: 10
Patch Set 10 : two functions become private #
Total comments: 10
Patch Set 11 : do not catch exception in cloudtail start/stop #Patch Set 12 : update docstring about Raises: #
Total comments: 9
Patch Set 13 : do not wait killed_pid #
Total comments: 8
Patch Set 14 : use subparser #Patch Set 15 : rebase #
Total comments: 12
Patch Set 16 : fix arg parser #Patch Set 17 : upload_logs on finally #Patch Set 18 : remove unused import #Patch Set 19 : let upload_logs become private #Patch Set 20 : missed CL #
Total comments: 2
Patch Set 21 : do _stop_cloudtail anyway #
Total comments: 6
Patch Set 22 : use defer_results #Patch Set 23 : change #Patch Set 24 : remove a try-finally #Messages
Total messages: 45 (13 generated)
tikuta@chromium.org changed reviewers: + phajdan.jr@chromium.org, shinyak@chromium.org, ukai@chromium.org, yyanagisawa@chromium.org
tikuta@chromium.org changed required reviewers: + phajdan.jr@chromium.org
The CQ bit was checked by tikuta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:12: from common import goma_utils goma_utils in common? it is in slave? https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:28: parser = argparse.ArgumentParser(description='cloudtail utility for goma recipe module.') long line?
https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/api.py:28: return '/opt/infra-tools/cloudtail' It it possible for you to download cloudtail from cipd? https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:21: '--path', goma_utils.GetLatestGomaCompilerProxyInfo()], I want to know the design choice why --log-id is passed as an argument but GetLatestGomaCompilerProxyInfo is not? Since I thought the id goma_compiler_proxy is only valid for goma's compiler_proxy.INFO, I thought either of three designs. 1. also pass compiler_proxy info as command argument. 2. won't pass either of --log-id or compiler_proxy.INFO filename. 3. pass --log-id via command line argument, and set --path goma_utils.GetLatestGomaCompilerProxyInfo only if the id goma_compiler_proxy is set. I prefer 3rd case but in 3rd case, it might be better to print pid to JSON file instead of stdout? To run multiple cloudtail at once? https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:37: help='pid that is killed') Is it allowed to set both start-cloudtail and killed-pid? https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:45: os.kill(int(args.killed_pid), signal.SIGTERM) Don't you wait the program termination? (also SIGKILL if not stopped in certain duration?)
https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/api.py:28: return '/opt/infra-tools/cloudtail' On 2016/08/15 07:19:30, Yoshisato Yanagisawa wrote: > It it possible for you to download cloudtail from cipd? I added code to donwload cloudtail. But that changes many *.json, so let me make another CL to enable downloading cloudtail from cipd. https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:12: from common import goma_utils On 2016/08/15 06:54:00, ukai wrote: > goma_utils in common? it is in slave? Done. https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:21: '--path', goma_utils.GetLatestGomaCompilerProxyInfo()], On 2016/08/15 07:19:30, Yoshisato Yanagisawa wrote: > I want to know the design choice why --log-id is passed as an argument but > GetLatestGomaCompilerProxyInfo is not? > > Since I thought the id goma_compiler_proxy is only valid for goma's > compiler_proxy.INFO, I thought either of three designs. > 1. also pass compiler_proxy info as command argument. > 2. won't pass either of --log-id or compiler_proxy.INFO filename. > 3. pass --log-id via command line argument, and set --path > goma_utils.GetLatestGomaCompilerProxyInfo only if the id goma_compiler_proxy is > set. > > I prefer 3rd case but in 3rd case, it might be better to print pid to JSON file > instead of stdout? To run multiple cloudtail at once? Thank you for comment. But I'm not sure why JSON is better than raw_io. It is possible to run multiple cloudtail if we use raw_io? https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:28: parser = argparse.ArgumentParser(description='cloudtail utility for goma recipe module.') On 2016/08/15 06:54:00, ukai wrote: > long line? Done. https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:37: help='pid that is killed') On 2016/08/15 07:19:30, Yoshisato Yanagisawa wrote: > Is it allowed to set both start-cloudtail and killed-pid? I wrote assertion and added a message to help. https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:45: os.kill(int(args.killed_pid), signal.SIGTERM) On 2016/08/15 07:19:30, Yoshisato Yanagisawa wrote: > Don't you wait the program termination? > (also SIGKILL if not stopped in certain duration?) Done.
https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/api.py:28: return '/opt/infra-tools/cloudtail' On 2016/08/16 02:16:56, tikuta wrote: > On 2016/08/15 07:19:30, Yoshisato Yanagisawa wrote: > > It it possible for you to download cloudtail from cipd? > > I added code to donwload cloudtail. > But that changes many *.json, so let me make another CL to enable downloading > cloudtail from cipd. Sorry for going back and forward but if you actually do not download cipd, will you remove the code and just leave TODO? I believe it confusing to have the code that won't be used. https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:21: '--path', goma_utils.GetLatestGomaCompilerProxyInfo()], On 2016/08/16 02:16:56, tikuta wrote: > On 2016/08/15 07:19:30, Yoshisato Yanagisawa wrote: > > I want to know the design choice why --log-id is passed as an argument but > > GetLatestGomaCompilerProxyInfo is not? > > > > Since I thought the id goma_compiler_proxy is only valid for goma's > > compiler_proxy.INFO, I thought either of three designs. > > 1. also pass compiler_proxy info as command argument. > > 2. won't pass either of --log-id or compiler_proxy.INFO filename. > > 3. pass --log-id via command line argument, and set --path > > goma_utils.GetLatestGomaCompilerProxyInfo only if the id goma_compiler_proxy > is > > set. > > > > I prefer 3rd case but in 3rd case, it might be better to print pid to JSON > file > > instead of stdout? To run multiple cloudtail at once? > > Thank you for comment. > But I'm not sure why JSON is better than raw_io. > It is possible to run multiple cloudtail if we use raw_io? As we talked offline, it should be better for cloudtail script uses constant value goma_compiler_proxy because we only upload compiler_proxy.INFO here. https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:32: parser.add_argument('--log-id', i.e. I suggest to remove this command line argument.
https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/api.py:28: return '/opt/infra-tools/cloudtail' On 2016/08/16 02:50:10, Yoshisato Yanagisawa wrote: > On 2016/08/16 02:16:56, tikuta wrote: > > On 2016/08/15 07:19:30, Yoshisato Yanagisawa wrote: > > > It it possible for you to download cloudtail from cipd? > > > > I added code to donwload cloudtail. > > But that changes many *.json, so let me make another CL to enable downloading > > cloudtail from cipd. > > Sorry for going back and forward but if you actually do not download cipd, will > you remove the code and just leave TODO? > I believe it confusing to have the code that won't be used. Done. https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:21: '--path', goma_utils.GetLatestGomaCompilerProxyInfo()], On 2016/08/16 02:50:10, Yoshisato Yanagisawa wrote: > On 2016/08/16 02:16:56, tikuta wrote: > > On 2016/08/15 07:19:30, Yoshisato Yanagisawa wrote: > > > I want to know the design choice why --log-id is passed as an argument but > > > GetLatestGomaCompilerProxyInfo is not? > > > > > > Since I thought the id goma_compiler_proxy is only valid for goma's > > > compiler_proxy.INFO, I thought either of three designs. > > > 1. also pass compiler_proxy info as command argument. > > > 2. won't pass either of --log-id or compiler_proxy.INFO filename. > > > 3. pass --log-id via command line argument, and set --path > > > goma_utils.GetLatestGomaCompilerProxyInfo only if the id goma_compiler_proxy > > is > > > set. > > > > > > I prefer 3rd case but in 3rd case, it might be better to print pid to JSON > > file > > > instead of stdout? To run multiple cloudtail at once? > > > > Thank you for comment. > > But I'm not sure why JSON is better than raw_io. > > It is possible to run multiple cloudtail if we use raw_io? > > As we talked offline, > it should be better for cloudtail script uses constant value goma_compiler_proxy > because we only upload compiler_proxy.INFO here. Done. https://codereview.chromium.org/2237403002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:32: parser.add_argument('--log-id', On 2016/08/16 02:50:10, Yoshisato Yanagisawa wrote: > i.e. I suggest to remove this command line argument. Done.
https://codereview.chromium.org/2237403002/diff/120001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:73: '--log-id', 'goma_compiler_proxy', remove this? https://codereview.chromium.org/2237403002/diff/120001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:50: pid, retcode = os.waitpid(killed_pid, os.WNOHANG) Don't you need to retry for a while?
https://codereview.chromium.org/2237403002/diff/120001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:73: '--log-id', 'goma_compiler_proxy', On 2016/08/16 03:45:23, Yoshisato Yanagisawa wrote: > remove this? Done. https://codereview.chromium.org/2237403002/diff/120001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:50: pid, retcode = os.waitpid(killed_pid, os.WNOHANG) On 2016/08/16 03:45:23, Yoshisato Yanagisawa wrote: > Don't you need to retry for a while? Done.
lgtm w/ nit. https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:78: def stop_cloudtail(self): nit action for self._cloudtail_pid is None https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/example.expected/linux.json (right): https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/example.expected/linux.json:76: "--log-id", might need to update JSON file? https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:62: nit one extra space?
https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:74: stdout=self.m.raw_io.output()) What happens if this fails?
https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:74: stdout=self.m.raw_io.output()) On 2016/08/16 04:22:09, shinyak wrote: > What happens if this fails? If it fails to start cloudtail, I stop goma and raise exception. https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:78: def stop_cloudtail(self): On 2016/08/16 04:16:24, Yoshisato Yanagisawa wrote: > nit action for self._cloudtail_pid is None Done. https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/example.expected/linux.json (right): https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/example.expected/linux.json:76: "--log-id", On 2016/08/16 04:16:24, Yoshisato Yanagisawa wrote: > might need to update JSON file? Done. https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/140001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:62: On 2016/08/16 04:16:24, Yoshisato Yanagisawa wrote: > nit one extra space? Done.
https://codereview.chromium.org/2237403002/diff/160001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/160001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:65: def start_cloudtail(self): doc string? _start_cloudtail, if it is not expected to be called by module user? https://codereview.chromium.org/2237403002/diff/160001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:68: step_result = self.m.python( when it failed, stop goma? https://codereview.chromium.org/2237403002/diff/160001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:78: def stop_cloudtail(self): doc string? _stop_cloudtail? https://codereview.chromium.org/2237403002/diff/160001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:89: name='stop_goma (cloudtail start failure)', cloudtail stop failure?
https://codereview.chromium.org/2237403002/diff/160001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/160001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:79: assert self._cloudtail_pid is not None If start_cloudtail failed, this assert can be triggered? Is it expected?
https://codereview.chromium.org/2237403002/diff/160001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/160001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:65: def start_cloudtail(self): On 2016/08/16 05:46:06, ukai wrote: > doc string? > > _start_cloudtail, if it is not expected to be called by module user? Done. https://codereview.chromium.org/2237403002/diff/160001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:68: step_result = self.m.python( On 2016/08/16 05:46:06, ukai wrote: > when it failed, stop goma? Done. https://codereview.chromium.org/2237403002/diff/160001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:78: def stop_cloudtail(self): On 2016/08/16 05:46:06, ukai wrote: > doc string? > > _stop_cloudtail? Done. https://codereview.chromium.org/2237403002/diff/160001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:79: assert self._cloudtail_pid is not None On 2016/08/16 05:48:39, shinyak wrote: > If start_cloudtail failed, this assert can be triggered? Is it expected? I missed the location to reflect the comment about start_cloudtail failing from you. Now, if it fails to start cloudtail, build stops. https://codereview.chromium.org/2237403002/diff/160001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:89: name='stop_goma (cloudtail start failure)', On 2016/08/16 05:46:07, ukai wrote: > cloudtail stop failure? Done.
https://codereview.chromium.org/2237403002/diff/180001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:66: """Start cloudtail to upload compiler_proxy.INFO""" Raises: ? https://codereview.chromium.org/2237403002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:89: """Stop cloudtail started by _start_cloudtail""" Raises: ? https://codereview.chromium.org/2237403002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:100: self.m.python( already stop goma before _stop_cloudtail? https://codereview.chromium.org/2237403002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:136: self._goma_started = True might be better to put _start_cloudtail here and remove try-except from _start_cloudtail ? https://codereview.chromium.org/2237403002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:167: self._stop_cloudtail() if stop_cloudtail failed, ninja log won't be uploaded?
https://codereview.chromium.org/2237403002/diff/180001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:66: """Start cloudtail to upload compiler_proxy.INFO""" On 2016/08/16 07:27:09, ukai wrote: > Raises: > ? Done. https://codereview.chromium.org/2237403002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:89: """Stop cloudtail started by _start_cloudtail""" On 2016/08/16 07:27:09, ukai wrote: > Raises: > ? Done. https://codereview.chromium.org/2237403002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:100: self.m.python( On 2016/08/16 07:27:09, ukai wrote: > already stop goma before _stop_cloudtail? Ah, I stop to catch exception here. https://codereview.chromium.org/2237403002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:136: self._goma_started = True On 2016/08/16 07:27:09, ukai wrote: > might be better to put _start_cloudtail here > and remove try-except from _start_cloudtail ? Done. https://codereview.chromium.org/2237403002/diff/180001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:167: self._stop_cloudtail() On 2016/08/16 07:27:09, ukai wrote: > if stop_cloudtail failed, ninja log won't be uploaded? Done.
https://codereview.chromium.org/2237403002/diff/220001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:153: It is user's responsibility to handle failure of stopping compiler_proxy. stop_goma, and upload_logs never raises exception? or if one of them raised exception, does it stop cloudtail? (or can user make it sure?) https://codereview.chromium.org/2237403002/diff/220001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:36: parser.add_argument('--killed-pid', type=int ? https://codereview.chromium.org/2237403002/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:54: pid, retcode = os.waitpid(killed_pid, os.WNOHANG) check OSError? https://codereview.chromium.org/2237403002/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:54: pid, retcode = os.waitpid(killed_pid, os.WNOHANG) os.waitpid won't work with the process id that is not child process of the process? >>> os.waitpid(38645, os.WNOHANG) Traceback (most recent call last): File "<stdin>", line 1, in <module> OSError: [Errno 10] No child processes https://codereview.chromium.org/2237403002/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:55: if pid == 0 and retcode == 0: if pid == 0 and retcode ==0 is still running, so should check os.waitpid again? otherwise, can break the loop. https://codereview.chromium.org/2237403002/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:58: if pid != 0 or retcode != 0: if pid == 0 and retcode == 0?
https://codereview.chromium.org/2237403002/diff/220001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:153: It is user's responsibility to handle failure of stopping compiler_proxy. On 2016/08/17 01:13:15, ukai wrote: > stop_goma, and upload_logs never raises exception? > or if one of them raised exception, does it stop cloudtail? (or can user make it > sure?) Done. https://codereview.chromium.org/2237403002/diff/220001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:36: parser.add_argument('--killed-pid', On 2016/08/17 01:13:15, ukai wrote: > type=int > ? Done. https://codereview.chromium.org/2237403002/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:54: pid, retcode = os.waitpid(killed_pid, os.WNOHANG) On 2016/08/17 01:13:15, ukai wrote: > os.waitpid won't work with the process id that is not child process of the > process? > > >>> os.waitpid(38645, os.WNOHANG) > Traceback (most recent call last): > File "<stdin>", line 1, in <module> > OSError: [Errno 10] No child processes I decided to send kill signal only for killed_pid.
lgtm
https://codereview.chromium.org/2237403002/diff/240001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:51: nit: Remove redundant empty line. https://codereview.chromium.org/2237403002/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:81: infra_step=True) Consider adding default step_test_data=lambda: self.m.raw_io.test_api.output(...) here so it's not needed in GenTests. https://codereview.chromium.org/2237403002/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:172: self._stop_cloudtail() Why not use "finally" to call _stop_cloudtail instead? https://codereview.chromium.org/2237403002/diff/240001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:36: parser.add_argument('--killed-pid', type=int, This can easily get messy. Could you refactor this into a set of subcommands instead? See https://docs.python.org/2.7/library/argparse.html#sub-commands .
Thank you for your comments, Pawel. I can know python and recipes more. https://codereview.chromium.org/2237403002/diff/240001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:51: On 2016/08/17 06:27:01, Paweł Hajdan Jr. wrote: > nit: Remove redundant empty line. Done. https://codereview.chromium.org/2237403002/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:81: infra_step=True) On 2016/08/17 06:27:01, Paweł Hajdan Jr. wrote: > Consider adding default step_test_data=lambda: > self.m.raw_io.test_api.output(...) here so it's not needed in GenTests. Done. https://codereview.chromium.org/2237403002/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:172: self._stop_cloudtail() On 2016/08/17 06:27:01, Paweł Hajdan Jr. wrote: > Why not use "finally" to call _stop_cloudtail instead? Done. https://codereview.chromium.org/2237403002/diff/240001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:36: parser.add_argument('--killed-pid', type=int, On 2016/08/17 06:27:01, Paweł Hajdan Jr. wrote: > This can easily get messy. > > Could you refactor this into a set of subcommands instead? See > https://docs.python.org/2.7/library/argparse.html#sub-commands . Done.
https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:25: f.write(str(proc.pid)) This is a weird structure. Either hardcode sys.stdout here (which is fine - could even be just print for simplicity), or go all the way and allow specifying any output file (https://docs.python.org/2.7/library/argparse.html#filetype-objects). You can still make it default to sys.stdout. https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:36: parser_start.add_argument('--cloudtail-path', Is this a required argument? If so add required=True. https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:41: parser_stop.add_argument('--killed-pid', type=int, Same here. https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:46: if args.cloudtail_path: This is close, but for detecting which command to run, it should really use the subparser command. See https://github.com/luci/recipes-py/blob/master/recipes.py for a good example: fetch_p.set_defaults(command='fetch') Then use args.command to dispatch. https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:50: # only send SIGKILL signal, let init process do wait for killed_pid nit: Start with capital letter, end with a dot.
lgtm https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:190: self.upload_logs(ninja_log_outdir, ninja_log_compiler, ninja_log_command, want upload ninja log even if stop goma fails?
Thank you for review. https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:190: self.upload_logs(ninja_log_outdir, ninja_log_compiler, ninja_log_command, On 2016/08/18 04:04:53, ukai wrote: > want upload ninja log even if stop goma fails? Done. https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:25: f.write(str(proc.pid)) On 2016/08/17 13:57:29, Paweł Hajdan Jr. wrote: > This is a weird structure. Either hardcode sys.stdout here (which is fine - > could even be just print for simplicity), or go all the way and allow specifying > any output file > (https://docs.python.org/2.7/library/argparse.html#filetype-objects). You can > still make it default to sys.stdout. Done. https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:36: parser_start.add_argument('--cloudtail-path', On 2016/08/17 13:57:29, Paweł Hajdan Jr. wrote: > Is this a required argument? If so add required=True. Done. https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:41: parser_stop.add_argument('--killed-pid', type=int, On 2016/08/17 13:57:29, Paweł Hajdan Jr. wrote: > Same here. Done. https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:46: if args.cloudtail_path: On 2016/08/17 13:57:29, Paweł Hajdan Jr. wrote: > This is close, but for detecting which command to run, it should really use the > subparser command. > > See https://github.com/luci/recipes-py/blob/master/recipes.py for a good > example: > > fetch_p.set_defaults(command='fetch') > > Then use args.command to dispatch. Done. https://codereview.chromium.org/2237403002/diff/280001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:50: # only send SIGKILL signal, let init process do wait for killed_pid On 2016/08/17 13:57:29, Paweł Hajdan Jr. wrote: > nit: Start with capital letter, end with a dot. Done.
lgtm https://codereview.chromium.org/2237403002/diff/380001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/380001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:176: self._stop_cloudtail() ok not stop cloudtail when upload logs failed?
https://codereview.chromium.org/2237403002/diff/380001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/380001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:176: self._stop_cloudtail() On 2016/08/18 08:19:41, ukai wrote: > ok not stop cloudtail when upload logs failed? Done.
"not lgtm" https://codereview.chromium.org/2237403002/diff/400001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/400001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:174: exception = e Whoa, this error handling code seems to become quite unwieldy. Could you use api.step.defer_results instead? You can use codesearch and see some examples. The idea is it'll accumulate exceptions and only raise them at the end, while continuing to run all steps. https://codereview.chromium.org/2237403002/diff/400001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/400001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:51: # Only send SIGKILL signal, let init process do wait for killed_pid. nit: Is this comment really useful? https://codereview.chromium.org/2237403002/diff/400001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:52: os.kill(killed_pid, signal.SIGKILL) nit: Just use args.killed_pid here, no need for temp variable.
Thank you for review. https://codereview.chromium.org/2237403002/diff/400001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/api.py (right): https://codereview.chromium.org/2237403002/diff/400001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/api.py:174: exception = e On 2016/08/19 09:39:04, Paweł Hajdan Jr. wrote: > Whoa, this error handling code seems to become quite unwieldy. > > Could you use api.step.defer_results instead? You can use codesearch and see > some examples. The idea is it'll accumulate exceptions and only raise them at > the end, while continuing to run all steps. Ah, yeah. step.defer_results is what we want to use. https://codereview.chromium.org/2237403002/diff/400001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right): https://codereview.chromium.org/2237403002/diff/400001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:51: # Only send SIGKILL signal, let init process do wait for killed_pid. On 2016/08/19 09:39:04, Paweł Hajdan Jr. wrote: > nit: Is this comment really useful? Done. https://codereview.chromium.org/2237403002/diff/400001/scripts/slave/recipe_m... scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:52: os.kill(killed_pid, signal.SIGKILL) On 2016/08/19 09:39:04, Paweł Hajdan Jr. wrote: > nit: Just use args.killed_pid here, no need for temp variable. Done.
LGTM
Thank you for review. I'll commit this after passing dry run.
The CQ bit was checked by tikuta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tikuta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yyanagisawa@chromium.org, shinyak@chromium.org, ukai@chromium.org Link to the patchset: https://codereview.chromium.org/2237403002/#ps460001 (title: "remove a try-finally")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add cloudtail start/stop in recipe_modules/goma * add cloudtail_utils.py in resources directory BUG=544330 ========== to ========== Add cloudtail start/stop in recipe_modules/goma * add cloudtail_utils.py in resources directory BUG=544330 Committed: https://chromium.googlesource.com/chromium/tools/build/+/2c034e723e150b21ed4d... ==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001) as https://chromium.googlesource.com/chromium/tools/build/+/2c034e723e150b21ed4d... |
