Hi Darren. On 09/ 4/11 12:13 PM, Darren Kenny wrote:
Hi Jack,I really appreciate your efforts on this and the quick turn around your making... I'm much happier with things now - Karen and Drew have pointed out several small issues, I don't have much else to add to them, other than a tiny nit: 320 + if progress_estimate == 0: 321 + progress_estimate = 1 Although it's possibly unlikely, I think it makes sense for this to read as "progress_estimate<= 0" to be sure we catch all possibilities...
OK. Changed. Thanks for working with me on this. Final changes, which are FYI: https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_5/webrev/ Jack
Thanks, Darren. On 04/09/2011 14:49, Drew Fisher wrote:Jack, I missed all of the discussion on Friday, but I wanted to throw my hat in the ring. Note: I've only looked at the most recent version 4 vs. slim source webrev. svr4.py ---------- nit: Almost all of the new logger messages are going to .info() For errors the user needs to be aware of, I'm fine with it, but the more we can move to .debug(), the better. It drastically improves readability of what goes to the screen. 66: change to: ROOT = os.environ.get("ROOT", "") so you don't have to do lines 67-68 85: Why not change [A-Za-z]* to \w* 107-109: If this exists solely for testing, it needs to move to the test file 251, 287: use list() 300: can you add a space after both ":" characters, for legibility. 320: change to: if not progress_estimate: 419, 425: align with previous line 587, 590, 691, 694: change both of these to a simple 4 space indent 603-608: I don't understand why this block of code is here and commented out. If it's only for testing, move it to the test file. If it needs to be supported by the DTD files, we should file a bug and get it fixed. -Drew On 9/3/11 4:12 PM, Jack Schwartz wrote:Hi Karen. Thanks again. Next iteration vs slim_source: https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_4 vs V3: https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_4_3 Replies below. Please bless. On 09/ 2/11 06:18 PM, Karen Tung wrote:Hi Jack, I reviewed https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_3 and here are my comments: ---------- - General comment about svr4.py: I see that you are using all the class constants (ie: all those that are defined before __init__()) as self.XXXX. I guess it works, but I don't think that's how it is typically used. I looked at all the other python code we have, and all the class constants are used as classname.constant_name, for example, AbstractSVR4.ADMIN_FILESome modules use self, some don't. For example, in the same dir, I see ips.py uses self. cpio.py uses a mixture, media_transfer users the class name. I'm happy to change this per your request.- Another general comment: I still don't think you need a lock for the parse_input. I understand your concern about multiple threads calling parse_input at the same time. Since we have full control of the CUD architecture, we know that get_progress_estimate() and execute() won't get called at the same time.Since it's apparent that you feel very strongly about this, I'll remove it and add a comment to the code that it is not thread-safe.- 89-91: since you are defining these as constants, it would make all the debugging statement much more readable if you define them as strings, so, I don't need to figure out what's "1". Also, I think all these should be capitalized.OK- line 364: should we check whether the admin file already exists? If so, should we just use it? If not, should we log a warning about the fact that we are overwriting the file. Also, I think the admin file should be removed when the checkpoint with it.Relocating the file to _execute and deleting it when through with it.- line 479: I don't think you should return here. Other values should still be checked.The rest of this method is a for loop on the empty list, functionality is the same with or without the return. I'll remove the return to make it easier in case someone modifies the routine adding stuff to the bottom of it though.- line 545-548: since the admin file is not used until execute() time, should we not generate it until we really need to use it?Relocating generation to execute() and deleting it when through with it. Thanks, Jack---------- Thanks, --Karen On 09/ 2/11 04:34 PM, Jack Schwartz wrote:Hi Darren. Hopefully this webrev addresses all of your concerns. Webrev vs slim_source: https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_3 Webrev vs V2 https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_3_2 This version does no downloads of remote files but uses http protocol to get remote datastream sizes. Only pkgadd does downloading. BTW, I can now also say that I've done a full AI install that includes an SVR4 package. It works in that the data shows up on disk, but I cannot login due to other issues. Responses below. On 09/ 2/11 04:11 AM, Darren Kenny wrote:Hi Jack, I have some more comments, sorry... - Nit: lines 173, 632 and 704 - "Insure" should probably be "Ensure" - line 300 (get_progress_estimate()) This causes a remote file to be downloaded - since this method is called really early on in the execution cycle (possibly long before the execute() method is called) - this would mean there is a possible gap between the download of the package, and the install of it - and if there is an intermediate other SVR4 type install, then it could overwrite the temporary file as it's named now! The assumption for more get_progress_estimate() methods is that they only should to a "best effort attempt" - hence the term estimate - and downloading the file is going too far at this point I think. I would still prefer if the size used here was that available in the HTTP header - it is correct to use since that is how much really needs to be downloaded regardless of how much in the data-stream you really want to install. If the size cannot be estimated, then I think a fall-back default value is all we can use. The estimation really shouldn't need to modify the filesystem in any way. I know I raised the idea of downloading the file, but given the size of the downloads involved in getting header information that you provided, this is overkill by us I feel. The only thing that really needs to do any downloading here is the pkgadd command itself.That's now how it is. Sizes are a little inconsistent but shouldn't be a problem. Remote datastreams use http protocol to get size of the whole image. Local datastreams do what was done earlier by looking at the datastream header. Local directory trees use dir_size() to get the size.Non-file related comments: - lines 528 to 531 This shouldn't be necessary since AI sets the "http_proxy" environment variable early on in the install process, and this should be available to the pkgadd command when called.Removed. I verified that pkgadd uses http_proxy environment variable as well.Temporary file related comments: NOTE: Before you do any of this, you might want to look at my comments on get_progress_estimate() first. - The use of "TMPDIR + svr4pkg + PID" for the temporary filename isn't that secure, and it might be worth considering using mktemp() or similar instead.N/A- I don't see anything to unlink() the temporary file after execution has completed - I do see it if there is an exception, but not after successful completion. Maybe should be done at end of _transfer() or execute() methods? (before you do this look at my comments below on get_progress_estimate() first)N/A- line 184: Why are you opening as "ab+" - this would imply you wish to append to any existing file, while I feel you really would want to truncate any existing file, and as such it possibly should be "wb+"?N/A- line 187: Maybe we should log what's happening there, like "Downloading remote package ...", and also maybe some message about the amount downloaded so far (would be good to state as "x of X" if the size was available in the HTTP header).N/A Thanks, JackThanks, Darren. On 02/09/2011 09:18, Jack Schwartz wrote:Hi Karen and Darren. Webrevs have been updated with both of your suggestions. There were lots of changes... Round 2 webrev vs slim_source: https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_2 Round 2 vs Round 1 https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_2_1 Karen, Thanks for your review. More below... On 09/ 1/11 03:04 PM, Karen Tung wrote:Hi Jack, Here are my comments. General: ------------ The SVR4 admin file is just a few lines of key value pairs. It's not something the user can customize, at this time, at least. Is it really necessary to deliver a file? Why not put all these key value pairs into a static dictionary in the svr4.py file, and create the file at execution time. This have the advantage of having everything the svr4.py module uses in 1 single place. It also have the advantage simplifying all the code in lines 473-496 of svr4.py, since you can just add or reset of the value of the "proxy" key, before writing out the file.I created a file so the user could replace it if desired. However, thinking more, if the user tinkers with it and causes an install or build to fail, it's just another thing we have to support, and having it doesn't seem to offer enough to outweigh this liability. What would the admin file help with? There's the scenario of a package with a conflicting file. Currently we quit on conflicts. The admin file would provide a way around this. For example, one using AI could change the admin file to complete their install. However if it is not changed back, the admin file could lead to unexpected results since it could end up being used by DC or another AI install. The thing the admin file was to help with is easily worked around, by the user just doing a manual pkgadd with custom admin file, after the AI install is competed, or by doing a custom script with a manual pkgadd in DC. I also like the idea of using a dictionary and keeping everything self-contained. Seems like this change is a good idea... I'll implement it.If you are really going to deliver the svr4 admin file, I believe you also need to modify the package manifest to add it there.N/Asvr4.py: ------------- - line 41-42: are Origin and Publisher used here? Sounds like IPS related things that's not needed here.These things are what the module originally used. The origin is the datastream file or directory the packages are coming from. Origin is a child of Publisher, in the general<software> genre.- Technically, the implementation of the get_pkgurl_status() is correct, but I found the way the code is currently written hard to understand. I am especially confused by lines 125-127. For me, I would add the check to see whether the given url is a file or a directory immediately after 115. If it is a file, open it. That way, you can eliminate the current lines 125-127, and just check for "if fd is not None".I rewrote this to download remote URLs once to save on network bandwidth, in response to Darren's comments. The replacement method is called open_pkg_src().- lines 157-159: Why catch the ValueError and raise it again, just with a message? If so, why you didn't do it for the open in line 153? My understanding with exception handling is that unless you are going to do something useful with the exception, just let it bubble up. If you are trying to provide information about the value of the URL, I think it will be easier to do it with logging debug statement.1) Line 153 won't fail due to 152. So there's a difference between there and 157. 2) I have changed most places which except one error and raise another, to call self.logger.error("custom message") followed by a raise of the original error.- line 171: Should we also check for line != None before we get into the while loop? I guess if line is None, the line.split call in line 172 will fail with a trace, do we want things to fail like that?readline will only return None if there is some internal python problem. It will return an empty string on EOF.- line 172: if line doesn't have 3 parts, this won't work, and the catching of ValueError you have in 176 doesn't include this line.a="1" a.split(" ", 2)['1'](b, c, d) = a.split(" ", 2)Traceback (most recent call last): File "<stdin>", line 1, in<module> ValueError: need more than 1 value to unpackThanks. Moved the split into the try below it.- line 176-178: again, I don't feel it is necessary to catch and re-raise the exception.Changed.- lines 238-256: you are trying to get the size of a directory here, right? If so, what about using the dir_size() function from osol_install.install_utils?Thanks. Didn't know about it. Changed.- line 297-312: why do we need a lock? The engine only runs 1 checkpoint at a time.There are two entry points into a transfer checkpoint which end up parsing the same manifest data and building the package list: get_progress_estimate() and execute(). I don't want to put the restriction on only one being called at a time, in case the engine becomes multithreaded in the future. The lock and the self.input_parsed flag insures that packages are accounted for only only once.- lines 381-400: all of this code can be replaced by calling run(cmd), where the run() function is defined in solaris_install. It will do much better logging and checking of return code...etc..Two reasons: 1) pkgadd can spew lots of output, and could fill up the buffer that Popen.check_call() uses. (run() uses Popen.check_call().) 2) Existing code allows to check for cancellation events. Popen.check_call() does not allow for this.- line 426-427: _transfer_list is an internal attribute for the AbstractSVR4 class. It's not something the user specified. I don't think it needs to be checked here. If it's empty, then, no installation is done. Perhaps we can log a warning if that's the case.OK. I log a warning now.- lines 472: DC allows a user to specify a http proxy too. I think we should handle that as well.I don't understand this request. I already get the http_proxy from the manifest.- line 525-528: is "pub" and "origin" used?Yes. Pub on 527 and origin on 529 (of original webrev)- line 589: I don't really see the point of introducing a "custom" action. From what I can see in other part of the code, we are just doing a pkgadd anyway with the special argument. So, why not just add the special argument and leave the action be "install"? You can put a debug/info logging statement about using the special args.OK. I now treat this case as a regular "install" except that I log an info message.- line 606: missing close ')'Thanks. Thanks, JackThanks, --Karen On 09/01/11 01:52, Jack Schwartz wrote:Hi everyone. Here is a code review to fix: 7084807<http://monaco.us.oracle.com/detail.jsf?cr=7084807> SVR4 package installs using the new autoinstaller don't work Webrev: https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_1 Bug report: http://monaco.us.oracle.com/detail.jsf?cr=7084807 Please review and bless by Friday 9/2 10 AM PST. I have done a lot of standalone testing with a special setup. Testing is still in progress and is looking good. I want to get the webrev out before I complete testing so I can get this into B174, pushing Friday COB. I don't anticipate any major changes at this point. Testing has included remote and local datastream files with 1 and>1 packages requested, local directories which are parents to groups of package trees. Testing still to be done: - A full AI install. Up until now I have bypassed IPS package installs to save time. I'll do this first thing in the morning. - Unit tests still need a little more work. Note: I introduce an SVR4 admin file to AI client /usr/share/install directory with this push. It is needed for non-interactive SVR4 installs. Thanks, Jack _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss_______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss_______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss_______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
