Keith,
I'm going to cover replying to your comments on cli.py and dc.py
itself. Alok gets to deal with everything else. Lucky him!
cli.py:
37: Nit: I can't imagine this definition being useful, since you'd
want to use the python builtin?
I walked all of DC and removed everything not being used by the
checkpoints.
distro_const.py:
33: Nit: doesn't need an explicit import ("import os;
os.path.do_stuff()" works)
Fixed
38-39: Group with other *_install imports on 44-57
41: For consistency (with e.g., line 62's use of logging.Formatter),
use logging.StreamHandler instead of importing the class separately.
42: Group with the other stdlib imports on 30-36
52: Combine with line 50.
All of the imports have been fixed. NOTE: I typically use the PEP8
standard on imports:
Imports should be grouped in the following order:
1. standard library imports
2. related third party imports
3. local application/library specific imports
Take another look and let me know if you have further issues with the
imports.
67-68: Since nothing is done here but a call to the parent class, just
skip overriding it.
Removed.
133: Have we considered deprecating the need for including "build" as
part of the command? If that's not in the cards, have we considered
having a second, separate subcommand for list (rather than 'build -l',
which doesn't actually build anything)?
We need to talk to the CLI folks before we can make a change like this.
Alok will take care of it, but for now it's still using build -l for
listing checkpoints.
133: Could just be "if 'build' not in args:" - I believe OptionParser
always returns at least an empty list for args? Should also check to
make sure "build" is the first item.
All of the argument stuff has been reworked to be a lot smarter.
137-138: Between this and the previous line, it looks like if you
specified: "distro_const build" the error message you'd get is "build
is not a valid manifest", which seems off. Similarly, "distro_const
build one two three" appears to be valid. I think the length of args
needs to be explicitly checked.
Fixed.
138: Requiring the file to end in ".xml" seems decidedly
"Windows-like" to me. I can store XML data in a file by any name; why
must it end in .xml? Let ManifestParser choke on it if the file
doesn't *contain* valid XML.
Fixed.
149: Use of global variables is disconcerting, particularly since
they're not in ALL_CAPS form. (Once I get to the EOF I may have a
suggestion or two on how to refactor appropriately.)
There are still a handful of globals, but they're now done properly (ALL
CAPS). This specific issue has been addressed by explicitly passing the
variable.
152: On the *rare* chance that this file can't be remo
try:
os.remove(lf_path)
except BaseException as err:
print "Could not remove '%s' because: %s" % (lf_path, err)
Fixed.
Since this code needs to run
155: DC doesn't need the default log created by the InstallLogger.
This tells me that creating the default log automatically, without
having an option to *not* create it, is a mistake. There should
probably be a flag to the logging module (and/or the engine) to skip
using the default log (the flag can default to either True or False,
but I think there should be one).
*ALL* of the logging code has been dramatically changed.
- We only log one file now (instead of two).
- We use the default log file as provided by the logging module
- I wrote a method to transfer the log in logger.py, thusly removing the
need for the transfer_logs() function
172: Would be *great* if we could add a command-line flag to modify
the verbosity of the stdout handler.
Done. -v/--verbose now sets the screen log level to .DEBUG. Default is
.INFO still.
184: It'd be nice to see part of this code as a method of
DCFileHandler (close stream, change file location, re-open stream and
flush pending items). Bonus points if this class moves to the logging
module proper and gets integrated with the transfer_logs method there...
Class has been completely removed. See above for logging changes.
184: Unused variable - dataset (globals again!)
Gone. No globals anyway :)
195: Should be set to logging.INFO, though if this could be refactored
per the above comment (so that, instead of creating a new handler, the
old one is re-used), that won't be needed.
Contained withing logging changes.
216-219, 221: As I recall from DOC behavior, each function will either
return None or raise an exception for the "not found" case - so only
one of these should be needed. If an exception is raised, do you
really need this try/except clause? If None is returned, you should
raise an exception on 222/223 instead of calling sys.exit.
227: If the schema prevents this, can you just use the
"get_first_child" syntax of the DOC instead of using the prior logic?
234: If the schema doesn't prevent this, then remove the mention of
DOC and explain what's wrong with the XML. If the schema DOES prevent
this, then don't try/except (since it would occur as a programming
error, and the developer that introduces a bug causing it might find
the full stacktrace more valuable).
All addressed by moving to get_first_child() and a small update to
dc.dtd (forced the manifest to have one and only one Execution section).
237: Shouldn't this be:
eng.stop_on_error = (execution_list[0].stop_on_error.capitalize() ==
"True")
The engine expects a boolean, not a string.
Fixed.
250: Might be better as:
<logger>.exception("Error registering checkpoint <...>. Full traceback
data is available in the log."
Since custom checkpoints are feasible, if a user adds their own custom
checkpoint, they'll probably appreciate getting the full traceback
information.
Fixed.
253: I don't think you want to remove the lockfile at this point in
DC's execution, do you? (Seems like you'd want to hold the lock until
right before DC exits in most cases - a try/finally clause around
main() might be best)
The lockfile mechanics have ALSO been totally redone as a context
manager. Huzzah Python!
279: If there's more than 1 error in the error service, it should be
retrieved and dumped to a log at the minimum, I think.
Fixed to dump as many errors as recorded in the error service.
281: This code isn't in an "except" clause, so logger.error/critical
should be used.
Fixed.
282-288: Should *not* be part of the 'for' loop. (Don't need to tell
them where the log file is for *every* failed checkpoint).
Fixed.
284: Is there supposed to be an else clause here that prints out the
location of the logs when the base_dataset (global!) is set?
The printout of the final location of the logs happens after
setup_build_datasets(), once DC has either created the needed datasets
or has found them already in existence. Also, global removed.
282, 286, 288: Should use logger.critical/error for consistency with
other statements in this clause (consider what happens when the log
level is set to something other than the default).
Fixed. Moved to .info()
306-310 (and elsewhere): When using format strings with logging
statements, please do the following:
- Have the logging code apply the formatting by using the form:
logger.debug("some format %s", msg)
(i.e., pass in the format parameters as arguments to the logger call)
- Do not call str(some_object). ("%s" will automatically call str() on
the object passed in)
- If a function is called to get data for the output string, enclose
the call in a statement of the form: "if
logger.isEnabledFor(logging.<level>):"
While these enhancements are not strictly necessary at the moment,
they are best practices when it comes to logging and thus good habits
to follow. They allow the logging module to completely skip the
relatively performance intensive steps of formatting in situations
when the logging message will be ignored due to the logger levels
being above the statement (i.e., if logger's level is logging.ERROR,
format strings won't be processed for logging.debug() statements).
I'm not real sure what you mean by this. Let's speak offline.
307, 309: Are both data dumps needed and valuable? Seems like they
could clutter the detailed log.
Having both data dumps has been invaluable for debugging where I screwed
up the DataObject subclasses in DC. I'd prefer to leave both in the
debug log.
313: The amount of logic here makes me wonder if perhaps there's some
way to make better use of, or enhance, the engine to help out. Perhaps
DC could "inject" a checkpoint called "empty" at the beginning of the
checkpoint lists (i.e., it goes after Manifest Parser and Target
Instantiation, but before any user-defined checkpoints).
There has ALSO been a sizable rewrite of the "get mountpoint" code in
dc.py. We now have a _get_mountpoints function to retrieve them. The
resulting code has slimmed down dc.py considerably.
330: "if not execute:"
Fixed.
345-348: Seems like this should be an "acquire_lockfile()" function
counterpart to remove_lockfile()
Context manager takes care of the acquire() portion.
346-7: Using logging and raising an exception would be preferred here.
Fixed.
350-355: Combine to single "if" clause, and then combine 358-365 to:
build_data_node.action = base_action
logs_node.action = base_action
media_node.action = base_action
Fixed.
439: I'm finding that it's generally better to use the following
paradigm for python executables:
def main():
# do stuff
if __name__ == '__main__':
main()
You'll find that using that set-up will break your global variables
(being in the "if" clause means your in the global scope; relocating
that to a function constrains the scope in a way that's more obvious
to anyone maintaining the code). The only variables in this code that
should be global are DC_LOGGER and maybe the ENGINE.
Fixed. 99% of distro_const.py has been relocated into __init__.py What
was the "if __name__ == '__main__': section is now a main() function in
__init__.py
467: Seems unnecessary? If there is some situation where the error
service already has an error at this point, it'd certainly be bad to
clear it out.
The error service stuff is cleared first thing into walking into main().
492ish: It appears that, based on the current schema and usage of
target instantiation, one could write a DC manifest that "creates"
rpool, or one that "creates" a new, second pool. Those seem like edge
case scenarios, and in both cases could have disastrous consequences -
a combination that lends itself to either disallowing those
possibilities, requiring a "-f" flag, or *prompting* the user to
confirm that they really want to do such a thing, before continuing.
This is DC, not a generic zpool / ZFS dataset generator (as evidenced
by such things as limiting the number of pools to 1, and number of
datasets to 1). Certainly, one could use the engine with the TI
checkpoint to create a simple, XML driven pool/dataset maker, but I
don't think that's a requirement here.
497-506: Use:
zpool_ds = zfs_lib.Dataset(zpool_name)
zpool_mp = zpool_ds.mountpoint
(You may need to catch AttributeError if there's no guarantee of the
dataset being existent)
509: I think recreating the logic that zfs uses to determine the
default mountpoint for a non-existent dataset is extremely fragile at
best. For example, with the existing logic, if the user has the
following datasets:
rpool
rpool/mystuff (mountpoint = /export/me/mystuff)
If the user gives DC "rpool/mystuff/dc_build" as a dataset, it looks
like DC will think the mountpoint is going to be
/rpool/mystuff/dc_build, when in reality, it will be
/export/me/mystuff/dc_build.
I think the best approach will be to wait until the target
instantiation step is complete, then get the mountpoint data based on
what the actual dataset mountpoints are. Any processing based on
mountpoints that's done before then will need to be refactored -
either moved to a later point, or adjusted to consider whether or not
the parent DC build dataset exists yet.
Considerable code has gone into DC to fix all of these issues including
your suggested method for retrieving the dataset's mountpoint.
515-524: I like how clean the "if" side of this if/else is. Could the
"else" portion be refactored into a separate function like
list_checkpoints is?
With all the refactoring, It's now ~ a dozen python statements. Take a
look and if it's still not up to snuff, we can refactor again.
546: Come to think of it, "set_http_proxy()" might be valuable in
other installers too (I'm thinking AI in particular). Can it be moved
somewhere common?
Moved to install_utils.
565-6: This assignment was already done towards the top of the main
section.
570-1: Ditto
Fixed.
572-576: Is there any reason to not let the engine do this check for
you? If there is a reason, it probably means the engine could use a
slight enhancement.
Fixed. The engine now tells us when a checkpoint doesn't exist to pause at.
588: RuntimeError *is* a BaseException (everything is a
BaseException). Also, nit, use "as msg" instead of ", msg"
Fixed.
Other:
No unit tests directly against any of the functions in distro_const.py?
Will be there shortly. Needed to refactor dc.py for changes first.
Thanks again, Keith!
-Drew
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
