We should * make the tooltool client able to query multiple sources for its builds, in case one is down * build multiple sources for it to query (maybe just relengweb2)

The second part of this is the relops part, and is in progress with bug 774354. The first part is a releng task. See also, bug 768879. Once the script is modified, we can build a way for it to access the webheads directly when the zeus-backed access method (http://tooltool.*.build.mozilla.org) fails (so punt the bug back to me for that).

Progress can be tracked here: https://github.com/simone-mozilla/tooltool/tree/bug_768123 Some more testing is needed before uploading a patch. The proposed implementation is designed to be fully backward compatible with the previous version of tooltool, which supported a single url. More than one URLs can now be provided as input to tooltool via command line using multiple --url arguments, e.g.: --url <url1> --url <url2> --url <url3> If no --url argument is provided via command line, urls are read in the config file, where alternative urls can be specified as in the following example: base_url=<default url> alternative_base_url_1=<first alternative, in case base_url is not accessible> alternative_base_url_2=<second alternative> ... Some notes: - the base_url key is still a valid key for the sake of backward compatibility - additional urls can be specified adding a key for each alternative url in the format "alternative_base_url_<number>"; tooltool will try to access them in the order explicitly specified by the number in the key itself (the order in which they are written in the config file is irrelevant).

Sounds good!

The diff is against revision 5aa22f53a78ac3011508c91eb52d69c1c60f57ce in repository https://github.com/jhford/tooltool/commit/master.

The attached examples of usage, kind of rudimentary manual tests, document the changes in the tooltool interface and show the resulting output. These commands have been run in the exact order they appear in the attachment file.

Comment on attachment 733604 [details] [diff] [review] Patch to add management of multiple urls to tooltool Hi Rail! Can you please review my patch? I selected you as a reviewer since I noticed you already landed tooltool related bugs in the past (namely, in the puppet repositories). Thanks a lot! Simone

Comment on attachment 733604 [details] [diff] [review] Patch to add management of multiple urls to tooltool Review of attachment 733604 [details] [diff] [review]: ----------------------------------------------------------------- First of all, thanks a lot for grabbig this outstanding bug! I have some comments, mostly PEP8 related. I'll be happy to review the next patch. ;) BTW, feel free to pep8tify this file. And feel free to poke me on IRC. ::: tooltool.py @@ +35,5 @@ > > log = logging.getLogger(__name__) > > +SUPPORTED_CONFIG_FILES=['/etc/tooltool', os.path.expanduser('~/.tooltool'), > + os.path.join(os.getcwd(), '.tooltool')] Use spaces around "=", align the second line properly. No, let's just remove it. See my comments below. @@ +379,4 @@ > # Let's bail! > return False > > + Trailing space, please delete. @@ +405,5 @@ > + if size != file_record.size: > + log.error("transfer from %s to %s failed due to a difference of %d bytes" % (url, > + file_record.filename, file_record.size - size)) > + else: > + log.info("Success! File %s fetched from %s" % (file_record.filename,base_url )) A space after coma, no space after base_url. @@ +409,5 @@ > + log.info("Success! File %s fetched from %s" % (file_record.filename,base_url )) > + return True > + except (urllib2.URLError, urllib2.HTTPError, ValueError) as e: > + log.info("..failed to fetch '%s' from %s" % (file_record.filename, base_url), > + exc_info=False) exc_info is False by default. Less code, faster reviews. :) @@ +414,5 @@ > + log.debug("%s" % e) > + except IOError: > + log.info("failed to write to '%s'" % file_record.filename, > + exc_info=True) > + return False Trailing space. @@ +469,4 @@ > elif cmd == 'add': > return add_files(options['manifest'], options['algorithm'], cmd_args) > elif cmd == 'fetch': > + trailing space. @@ +469,5 @@ > elif cmd == 'add': > return add_files(options['manifest'], options['algorithm'], cmd_args) > elif cmd == 'fetch': > + > + if not options.has_key('base_url') or options.get('base_url') is None or len(options.get('base_url'))==0: "if not options.get('base_url'):" replaces all 3 conditions. @@ +470,5 @@ > return add_files(options['manifest'], options['algorithm'], cmd_args) > elif cmd == 'fetch': > + > + if not options.has_key('base_url') or options.get('base_url') is None or len(options.get('base_url'))==0: > + log.critical('fetch command requires at least one url provided using the url option in the command line, or specifying urls in one of the supported config files %s' % str(SUPPORTED_CONFIG_FILES)) Can you wrap the line at 79? str() is redundant because of %s. @@ +495,5 @@ > + try: > + hmap=dict(cfg_file.items(section)) > + except ConfigParser.NoSectionError as e: > + log.debug("%s in config file" % e, exc_info=True) > + return [hmap[key] for key in sorted([key for key in hmap.keys() if p.match(key) ], key=lambda x: int(p.match(x).group(1))) ] I'm not going to decrypt the line above. :) That reminds me. I'd prefer to drop support of configuration files since we have never used them. The code wil be much more readable ana maintainable. Let's doo eet! :) BTW, if you have more than 2 empty lines, I'm going to point you to http://www.python.org/dev/peps/pep-0008/ :) @@ +524,5 @@ > # Set up logging, for now just to the console > + import sys > + > + log.setLevel(logging.DEBUG) > + ch = logging.StreamHandler(sys.stdout) Why do you need to log to stdout? Using stderr is quite common. @@ +569,5 @@ > > + > + > + > + # read main base_url from config file if not provided in command line, then adds any alternative_base_url provided trailing spaces and too many empty lines ^^ @@ +583,5 @@ > + options['base_url'].append(main_url) > + options['base_url'].extend(read_alternative_base_urls(cfg_file, log)) > + > + else: > + log.error("skipping config files") Let's just remove this block to get rid of config support.

(In reply to Rail Aliiev [:rail] from comment #7) > Comment on attachment 733604 [details] [diff] [review] > Patch to add management of multiple urls to tooltool > > Review of attachment 733604 [details] [diff] [review]: > ----------------------------------------------------------------- > > First of all, thanks a lot for grabbig this outstanding bug! > > I have some comments, mostly PEP8 related. I'll be happy to review the next > patch. ;) BTW, feel free to pep8tify this file. > And feel free to poke me on IRC. I'll just say, the "run through autopep8 /pep8ify" suggestion I'd like to be a functionally separate patch, whether that is prior to the actual functional changes or after doesn't matter to me, just makes blame/comprehension of diffs in future 20x better! I'm also not sold on completely removing the config-file support, but I don't care enough to [try to] block's rail's opinion.

(In reply to Justin Wood (:Callek) from comment #8) > I'll just say, the "run through autopep8 /pep8ify" suggestion I'd like to be > a functionally separate patch, whether that is prior to the actual > functional changes or after doesn't matter to me, just makes > blame/comprehension of diffs in future 20x better! Yeah, having 2 commits would be better. However, the patch shouldn't introduce new PEP8 violations. > I'm also not sold on completely removing the config-file support, but I > don't care enough to [try to] block's rail's opinion. If the code is not used it should die. If we really want to use configs, I'd say use JSON instead to avoid dirty hacks to support lists in INI files.

Thanks a lot Rail and Callek! I am going to upload three patches to: - pep8tify tooltool - apply the other non pep8 changes suggested by Rail - remove support for config files (in case this will need to be added again in the future, I agree with Rail's proposal to use JSON )

Existing violations of the E501 recommendation (line too long) have not been fixed (yet). This patch has been created with the command: git diff e15684dbee5840d90787246eef776c098d61ae12 e6934e3786d770b37dd96bbe912f54a7c67bfdbb > bug_768123_pep8.diff The mentioned commit ids have been pushed to https://github.com/simone-mozilla/tooltool/tree/bug_768123

This patch has been created with the command: git diff e6934e3786d770b37dd96bbe912f54a7c67bfdbb 2256e5069c30f510cacb53292f50fdceee739b6f > bug_768123_miscellaneous.diff The mentioned commit ids have been pushed to https://github.com/simone-mozilla/tooltool/tree/bug_768123

This patch has been created with the command: git diff 2256e5069c30f510cacb53292f50fdceee739b6f 8a5d67620dc90f260ef7e53ae2b2fc70f79a58e7 > bug_768123_removed_configfile_support.diff The mentioned commit ids have been pushed to https://github.com/simone-mozilla/tooltool/tree/bug_768123

Woo! I'll review the patches in a bit. For the future, can you use "git diff -U8" to include more context. It's not an issue right now because I can regenerate the patches using your git repo. See also https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Configuration for Mozilla's recommended hg configuration.

The patches look good. But they can't be applied without the one I r-'ed. How are you going to land the patches? I'd say use git rebase and squash some commits, but since the pep8 patch was applied in the middle of changes, it'll be hard to separate it from other changes... Also, can you incorporate this fix to make the unittests work (they are broken even without your patch). Evil mutable [] as a default! --- a/tooltool.py +++ b/tooltool.py @@ -172,8 +172,8 @@ class Manifest(object): valid_formats = ('json',) - def __init__(self, file_records=[]): - self.file_records = file_records + def __init__(self, file_records=None): + self.file_records = file_records or [] def __eq__(self, other): if self is other:

Also, I wonder if we should use https://github.com/mozilla/build-tooltool (404 now) as an official repo.

(In reply to Rail Aliiev [:rail] from comment #16) > Also, I wonder if we should use https://github.com/mozilla/build-tooltool > (404 now) as an official repo. I'd be ok with this, I'd also be happy[ier] if we use hal's tool to mirror it to our hg.m.o (even if RoR stays on github since it started there).

https://github.com/mozilla/build-tooltool is the new location. I'll file a bug to mirror it to hg and update wiki with the new URL.

Comment on attachment 733797 [details] [diff] [review] Incremental patch to remove support for config files We talked on IRC with Simone. He is going to resubmit the patches: one for real changes and one for pep8.

git diff -U8 5aa22f53a78ac3011508c91eb52d69c1c60f57ce 68a8b74b8ca337e0c54633c7d80bee043c0ef7a1 >patch1.diff These commit ids have been pushed to https://github.com/simone-mozilla/tooltool/tree/bug_768123

git diff -U8 68a8b74b8ca337e0c54633c7d80bee043c0ef7a1 205d849bd9b577cdc822bcc473d121947ae242f2 >patch2.diff The mentioned commit Ids have been pushed to https://github.com/simone-mozilla/tooltool/commits/bug_768123

Comment on attachment 733878 [details] [diff] [review] Enabled multiple urls via command line, removed support for config files and other minor changes Review of attachment 733878 [details] [diff] [review]: ----------------------------------------------------------------- R= with the following comments need to be addressed: ::: tooltool.py @@ +435,2 @@ > > +# TODO: write tests for this function This TODO is related to the next function, can you move it down? @@ +497,5 @@ > return False > > + > + > + Please, don't add new empty lines for no reason. @@ +520,5 @@ > > > def main(): > # Set up logging, for now just to the console > + log.setLevel(logging.DEBUG) Please drop this hunk. You have -v for this.

git diff -U8 54448bb0347d81fc9aa08e5eec31a7eeb8b80ef7 3bb9a01b97b0c487c73e3a9702a32d5f6f65111b > patch4.diff Now the pep8 cleanup should be completed (apart from E501): Simones-MacBook-Pro:tooltool sbruno$ /usr/local/share/python/pep8 tooltool.py --statistics -qq 42 E501 line too long (80 > 79 characters) Simones-MacBook-Pro:tooltool sbruno$

Log levels cleanup as recommended by Rail - hunk removed! git diff -U8 54448bb0347d81fc9aa08e5eec31a7eeb8b80ef7 3bb9a01b97b0c487c73e3a9702a32d5f6f65111b > patch4.diff

Just as a reminder to myself, tooltool in production is still using http://runtime-binaries.pvt.build.mozilla.org/tooltool, and this work is blocking moving tooltool off of that hostname.

Comment on attachment 733877 [details] [diff] [review] Patch to pep8tify tooltool. https://github.com/mozilla/build-tooltool/commit/199ae0a6c77702176ffed8fc762ba199a8d5d7c9

Comment on attachment 733878 [details] [diff] [review] Enabled multiple urls via command line, removed support for config files and other minor changes https://github.com/mozilla/build-tooltool/commit/fdcf3c88717da9e2a37b3671cf1b1c37bbacbfb4

Comment on attachment 733923 [details] [diff] [review] Further pep8tification https://github.com/mozilla/build-tooltool/commit/8c9cd3936feb00491ead9f256a4e50e8c292798d

Comment on attachment 733927 [details] [diff] [review] Log levels cleanup https://github.com/mozilla/build-tooltool/commit/86cd1b89e3d6493076499f22ea85774b511f7bd1

The patches for the two repos puppet and puppet-manifests are identical, so I am attaching only one. This patch will sync the tooltool version in those repos with https://github.com/mozilla/build-tooltool/commit/86cd1b89e3d6493076499f22ea85774b511f7bd1 (apart from the shebang line)

Comment on attachment 743675 [details] [diff] [review] Patch to port the changes to both puppet and puppet-manifests repositories Let's deploy it tomorrow?

Hi Rail, tomorrow (May 1st) is holiday in Germany so I will not be at work.

OK. Thursday then?

Comment on attachment 743675 [details] [diff] [review] Patch to port the changes to both puppet and puppet-manifests repositories https://hg.mozilla.org/build/puppet-manifests/rev/22b8f942937e https://hg.mozilla.org/build/puppet/rev/66a869e5d7c1

Deployed on the prehistorical puppet masters.

AIUI, in production this is still using runtime-binaries.pvt.b.m.o. So, so far tooltool is *capable* of more resiliency, but not actually resilient. Is there a bug to switch it to use a wider set of URLs?

Now there is: bug 882712.