Hi Alok,
Here is my review of section [1].
In regards to VMC, this means VMC support is essentially discontinued
until the follow-on project completes, since the old DC will be gone?
Were any other DC bugs fixed/made irrelevant by these changes? 13005,
10421, 16403 possibly? Might be worth looking over existing DC bugs to
see what else can be closed when this goes back (and to catch any major
'low-hanging' fruit).
I'm planning on trying to review section [2] tomorrow or Wednesday, but
if you'd prefer I examine a different section to get better coverage,
let me know.
- Keith
usr/src/cmd/distro_const/Makefile:
37-41: Please alphabetize, with __init__.py first.
RBAC:
Looks good. Glad to see we'll have a "DC" profile for use with pfexec!!
manifest/Makefile:
33-44: Alphabetize
46-48: Remove before putback?
install-distribution-constructor.mf:
33-46: Can you verify that these dependencies must be explicitly
defined, by removing them and running a build, then seeing if all the
depended upon packages showed up? I think pkgdepend should be able to
pick up most or all of them automatically, and we should take advantage
of that.
Nit: Alphabetize the file section (usr/lib before usr/share)
cli.py:
37: Nit: I can't imagine this definition being useful, since you'd want
to use the python builtin?
distro_const.py:
33: Nit: doesn't need an explicit import ("import os;
os.path.do_stuff()" works)
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.
67-68: Since nothing is done here but a call to the parent class, just
skip overriding it.
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)?
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.
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.
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.
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.)
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)
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).
172: Would be *great* if we could add a command-line flag to modify the
verbosity of the stdout handler.
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...
184: Unused variable - dataset (globals again!)
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.
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).
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.
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.
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)
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.
281: This code isn't in an "except" clause, so logger.error/critical
should be used.
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).
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?
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).
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).
307, 309: Are both data dumps needed and valuable? Seems like they could
clutter the detailed 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).
330: "if not execute:"
345-348: Seems like this should be an "acquire_lockfile()" function
counterpart to remove_lockfile()
346-7: Using logging and raising an exception would be preferred here.
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
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.
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.
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.
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?
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?
565-6: This assignment was already done towards the top of the main section.
570-1: Ditto
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.
588: RuntimeError *is* a BaseException (everything is a BaseException).
Also, nit, use "as msg" instead of ", msg"
Other:
No unit tests directly against any of the functions in distro_const.py?
On 10/19/10 03:46 PM, Alok Aggarwal wrote:
This is a code review request for the rewrite of DC
to make use of CUD. The implementation here closely
follows the design document that can be found here:
http://cvs.opensolaris.org/source/xref/caiman/caiman-docs/distro_constructor/dc_cud_design.pdf
Note that this wad does not include the support for
VMC. The support for VMC will be a separate, follow on
project.
Since the changes are fairly substantial we are breaking
up the review into the following sections:
DC app, RBAC, Makefiles and packaging [1]
Manifests and XML serialization/de-serialization [2]
Pre pkg image mod, boot archive configuration and archive archive
checkpoints [3]
All the other remaining checkpoints [4]
Please sign up to review one or more of the sections
and let us know exactly which ones you will be reviewing.
Webrev location is:
http://cr.opensolaris.org/~aalok/cud_dc
Please have your comments in by COB November 5th.
Thanks,
Drew and Alok
[1] usr/src/Makefile.master
usr/src/Targetdirs
usr/src/cmd/Makefile.targ
usr/src/cmd/distro_const/Makefile
usr/src/cmd/distro_const/distro_const.py
usr/src/cmd/distro_const/cli.py
usr/src/cmd/rbac/Makefile
usr/src/cmd/rbac/exec_attr.distribution-constructor
usr/src/cmd/rbac/prof_attr.distribution-constructor
usr/src/cmd/distro_const/checkpoints/Makefile
usr/src/cmd/distro_const/manifest/Makefile
usr/src/cmd/distro_const/profile/Makefile
usr/src/pkg/manifests/install-distribution-constructor.mf
usr/src/cmd/distro_const/checkpoints/defaultfiles/Makefile
usr/src/cmd/distro_const/checkpoints/defaultfiles/coreadm.default
usr/src/cmd/distro_const/checkpoints/defaultfiles/rtc_config.default
[2] usr/src/cmd/distro_const/__init__.py
usr/src/cmd/distro_const/configuration.py
usr/src/cmd/distro_const/distro_spec.py
usr/src/cmd/distro_const/execution_checkpoint.py
usr/src/cmd/distro_const/manifest/boot_archive_contents_sparc.xml
usr/src/cmd/distro_const/manifest/boot_archive_contents_x86.xml
usr/src/cmd/distro_const/manifest/dc_ai_sparc.xml
usr/src/cmd/distro_const/manifest/dc_ai_x86.xml
usr/src/cmd/distro_const/manifest/dc_livecd.xml
usr/src/cmd/distro_const/manifest/dc_text_sparc.xml
usr/src/cmd/distro_const/manifest/dc_text_x86.xml
usr/src/cmd/distro_const/profile/ai.xml
usr/src/cmd/distro_const/profile/generic.xml
usr/src/cmd/distro_const/profile/livecd.xml
usr/src/cmd/distro_const/profile/text.xml
[3] usr/src/cmd/distro_const/checkpoints/pre_pkg_img_mod.py
usr/src/cmd/distro_const/checkpoints/boot_archive_configure.py
usr/src/cmd/distro_const/checkpoints/boot_archive_archive.py
usr/src/cmd/distro_const/checkpoints/test/test_pre_pkg_img_mod.py
usr/src/cmd/distro_const/checkpoints/test/test_boot_archive_configure.py
usr/src/cmd/distro_const/checkpoints/test/test_boot_archive_archive.py
[4] usr/src/cmd/distro_const/checkpoints/ai_publish_pkg.py
usr/src/cmd/distro_const/checkpoints/create_iso.py
usr/src/cmd/distro_const/checkpoints/create_usb.py
usr/src/cmd/distro_const/checkpoints/custom_script.py
usr/src/cmd/distro_const/checkpoints/grub_setup.py
usr/src/cmd/distro_const/checkpoints/pkg_img_mod.py
usr/src/cmd/distro_const/checkpoints/test/test_ai_publish_package.py
usr/src/cmd/distro_const/checkpoints/test/test_create_iso.py
usr/src/cmd/distro_const/checkpoints/test/test_create_usb.py
usr/src/cmd/distro_const/checkpoints/test/test_custom_script.py
usr/src/cmd/distro_const/checkpoints/test/test_grub_setup.py
usr/src/cmd/distro_const/checkpoints/test/test_pkg_img_mod.py
usr/src/cmd/distro_const/checkpoints/test/testlib.py
_______________________________________________
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
