Hi Karen,
Thanks for your review.
On Tue, 2 Nov 2010, Karen Tung wrote:
Hi Alok and Drew,
Here are my comments on files in section 4. If you want me to take
a look at the other sections, just let me know.
----------
General:
----------
- In all the checkpoints I reviewed, there's a block of code in the
parse_doc() function that gets the pkg_img_path, ba_build, and temp_dir,
and media_dir. I think it would be cleaner to retrieve
all these values with a function that returns all the retrieved values
so we don't need to repeat the same code everywhere.
I don't see a huge benefit in extracting the lookup of
these four items into a separate function. If you don't
feel strongly about, I'm not going to make this change.
- In most of the checkpoints, the dry_run flag in execute_checkpoints()
is ignored, please handle them accordingly. If it is ignored, please
put a comment, and say why it is OK for all the operation to happen
even in dry_run mode.
Added a comment that looks like this:
"DC does not support the dry_run flag. dry_run is set to False"
- In get_progress_estimate() of all checkpoints, it just return a single
hard coded value. I understand that we have not
finalize which machine will be used for generating
the numbers, but I think we should have code in place
to return values based on the size of the image/data we try to operation
on instead of returning the same value regardless of size.
Progress reporting for DC is a non-goal for this project.
I have filed CR 6997987 to track this enhancement in the future.
------------
usr/src/cmd/distro_const/checkpoints/ai_publish_pkg.py
------------
- The create_repository() function uses pkg(5) CLI calls to
create the repo. Is it better to use pkg python interfaces
to do the same?
Yep, Joe will be working on making this checkpoint use the
python interfaces instead.
-----------------
usr/src/cmd/distro_const/checkpoints/create_iso.py
-----------------
- line 97-98, 203-204: duplicate code to check for incremental build.
Perhaps just check once and store the value.
Done.
- line 132: Does the file description need to be closed after reading?
I changed this to:
with open(vpath, "r") as vpath_fh:
self.volsetid = vpath_fh.read().strip()
that does an implicit close() when done.
- line 132: do we also need to make sure the self.volsetid that's
read from the file is not ""?
I don't see the current DC do this check and I'm not
really sure if it's needed. I mean if .volsetid is ""
then there's something really wrong in one of the other
checkpoints.
- create_incremental_media() function makes a copy of the ISO so
USB images can be created. I think this method is too heavy
duty. Would it be easier to store the name of the ISO in
the DOC. In the create_usb.py script, you just look
up the name used for the ISO.
I've changed it so that the name of the incremental iso is
passed via the DOC. create_usb.py has also been adjusted
appropriately.
- line 202: perhaps you need an "else" here to throw an exception.
I know architecture can be only "i386" or "sparc", but the way
things are coded right now makes it look like an "else" is needed.
changed the 'elif self.arch == "sparc"' to an 'else'
-----------------
usr/src/cmd/distro_const/checkpoints/create_usb.py: OK
-----------------
-----------------
usr/src/cmd/distro_const/checkpoints/custom_script.py
-----------------
- Line 30: when is the "CalledProcessError" used?
Removed.
- line 65: I think it is better for this to be done in
the __init__() part of this checkpoint, and we check and make
sure the "command_to_execute" is not None. No point fail later.
Agreed. Changed.
- lines 72-74: why do we want to redirect stdout/stderr to NULL?
I think it is important to log all messages/errors. You can use
the exec_cmd_outputs_to_log() function in lib/install_utils/install_utils.py.
We don't want to clutter the console with the messages from
stdout/stderr, that is why they're redirected to /dev/null.
In the case of an error, distro_const.py will actually
print out the return code from the command as well as
os.strerror() of that return code.
--------------
usr/src/cmd/distro_const/checkpoints/grub_setup.py
--------------
- Lines 85-90: Instead of having hard coded defaults here, I think
it is better for them to be defined as constants in the class so
they are easy to find and change in the future.
- Line 85-90: I noticed that you didn't assign a default value for
self.grub_mods.title. I think it would be easist to check that
no special title is specified here, and retrieve the value from /etc/release.
Then, use a flag to keep track of whether a special title is specified.
That would avoid the 2 places later that has the duplicate code to
get the value from /etc/release.
- Question: in existing DC, there's an entry in the manifest called
<title> within the <grub_menu_modifications> section.
The value specified in <title> is used for populating
all the grub menu entries. So, if I want to do the same
thing with the new DC and manifest, I would just do
the following:
<grub_mods min_mem="0" title="myentry" default_entry="0" timeout="5">
</grub_mods>
Note that I didn't specify a <grub_entry> because I don't want another
grub entry. Have we tested this case to make sure it works?
If it does, I think we need to put better comment in the manifests than
what we have now. The way it looks right now, it looks like I will have to
specify a <grub_entry>
- Line 145-162: In the update_img_info_path() function,
the GRUB_TITLE line is always written to the .image_info file.
The algorithm in the current DC grub_setup.py file only
writes the GRUB_TITLE line to the .image_info file if a special
title is specified. If I remember right, there's special logic
in installadm(1M) to deal with the GRUB_TITLE line being present or not.
Please check installadm(1M) to make sure this new behavior is
OK or change it back to the way it is done in the current DC.
- LiveCDGrubSetup.build_entries() function: What happened to all
the "special" entries that are in
the existing all_lang_slim_cd_x86.xml? Such as the livessh=enable
and all the entries to support accessibility. I checked the
dc_livecd.xml, and don't see them there either.
- LiveCDGrubSetup class: for the the Live CD, the accessibility
entries MUST be the last entry on the grub menu. I don't see any
special code in this class to deal with that.
- Line 236-256: AIGrubSetup.update_img_info_path().
The code to get the grub title from etc/release is duplicate.
Furthermore, as with the comment above, the GRUB_TITLE should
not always be written out to the file.
- Line 253: Need to remove this comment?? :-)
Drew will be handling these comments.
--------------
usr/src/cmd/distro_const/checkpoints/pkg_img_mod.py
--------------
- line 105-108: shouldn't this check before done before line 93,
where the value for self.pkg_img_path is first used?
Makes sense, changed.
- lines 118-196: The mkisofs and lofiadm commands and options
for creating and compressing the user and misc archives are very similar,
except the ISO sort file for the solaris.zlib. I think they
should be combined to avoid the duplicate code.
I don't think it's really code duplication, it's just
two invocations of the same command with almost the same
options but not exactly same. I don't think I'm going to
be making this change unless you feel strongly about it.
- lines 195-196: I think it will be very useful to put a comment here
to talk about exactly why the "usr" directory have to be removed
here. Otherwise, somebody who tries to clean up the code later
might want to move the removing of the "usr" directory to
the create_user_archive() function, which is where it seems to belong.
Good point. Added this comment:
# the removal of /usr must be deferred to until
# solarismisc.zlib has been created because the
# contents of solarismisc.zlib actually come from /usr
- line 225-228, 304-307: why do you need to get these 2 values again? Aren't
they already retrieved in the parent class's __init__()?
Good point, removed.
- strip_platform(), strip_x86_platform(), and strip_sparc_platform() all have
code
to walk the platform directory and remove everything except a few things.
I think it might be cleaner to define a function that contains the the "walk"
code
and remove code, and pass in the list of things you don't want to remove.
I think that might make this code harder to read so I'm
inclined to leave this as-is.
- line 384: The text installer will need to call create_livecd_content_file()
function, since the text installer is a cpio based install. It is not
identical the AI.
That's a bug! Thanks for catching it.
---------
usr/src/cmd/distro_const/checkpoints/test/test_ai_publish_package.py - OK
---------
---------
usr/src/cmd/distro_const/checkpoints/test/test_create_iso.py - OK
---------
---------
usr/src/cmd/distro_const/checkpoints/test/test_create_iso.py - OK
---------
---------
usr/src/cmd/distro_const/checkpoints/test/test_create_usb.py
---------
- What about testing generating USB using an ISO created with incremental
option?
too hard for a small gain :)
------------
usr/src/cmd/distro_const/checkpoints/test/test_custom_script.py
------------
- Lines 151-153: no code for this test case? :-)
Apparently not. Removed.
- What about a test case where the custom script specified is not found?
That's a good test, will add.
------------
usr/src/cmd/distro_const/checkpoints/test/test_grub_setup.py - OK
------------
------------
usr/src/cmd/distro_const/checkpoints/test/test_pkg_img_mod.py - OK
------------
------------
usr/src/cmd/distro_const/checkpoints/test/testlib.py - OK
------------
Again, thanks so much for your review!
Alok
Thanks,
--Karen
On 10/20/10 09:28 AM, Karen Tung wrote:
I will take section 4.
Thanks,
--Karen
On 10/19/10 15:46, 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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
