Hi Glenn, Thanks for your responses. Please see my a few comments inline.
Glenn Lagasse wrote: > Hi Karen, > > >> General comments: >> --------------------- >> - Question: Is it true that when this project integrates as it is coded >> now, users will not be able to specify what is included in the resulting >> VM image? The content of the VM image is dictated by whatever is >> the content of the default AI manifest? Since the default AI manifest >> installs from http://pkg.opensolaris.org/dev, the only way to >> change the repo from which the bootable AI install is done is to >> generate my own AI image with a different default AI manifest? >> If that's the case, what's the plan to support specifying what >> I want to include in the VM image and from where I want the AI install >> to happen "easily"? >> > > That is correct. As currently delivered, the VMC project does not make > any content changes to the bootable AI image passed to it (other than > modifying the grub menu to boot the default manifest entry). > > A useful modification of course would be to allow VMC to modify the > default AI client manifest on whatever bootable AI image is passed in > with one that the user supplies. I'm currently planning to add this > after the initial delivery. I'll also likely be adding in support to > supply a custom AI client manifest during construction of the bootable > AI image (it currently does not allow that either). > > Thanks for the explanation. >> - All the 4 existing manifests - The list of finalizer scripts used >> to be in the position of the file below the warning about not >> modifying certain fields in the manifest. They are now moved >> to a spot before that warning. That might confuse people. >> I think it is better to move them back to be below the warning. >> > > Well, the finalizer section is now part of the output_image tag which is > part of the distro_constr_params tag. Previously, it was part of the > img_params tag. > > ah, I see.. In that case, I think it would be better to have the finalizer scripts section be the last thing in the distro_constr_params section. Then, put a warning there about people shouldn't need to modify it. >> Specific Comments: >> -------------------- >> >> usr/src/cmd/distro_const/DC-manifest.rng >> >> - line 57-67: why is the nm_img_params optional? Shouldn't it be either >> nm_img_params or nm_vming_params? >> >> - Would be more clear to name the 2 choices as "im_live_img_params" and >> "nm_vm_img_params"? >> > > This actually was an oversight. See my response to Jean and Sundar. > Though I'm currently wondering if it might not be a better idea (than > what I outlined to them) to deliver an empty nm_vm_img_params so that we > can get some manifest validation (ie a manifest must have iehter > nm_img_params or nm_vmimg_params). > I think it is better to deliver an empty nm_vm_img_params for the time being. I think it is better that we enforce either live image parameters or VMC parameters are specified. >> - lines 294-305: why is this commented out? >> > > This is related to my comment for lines 57-67. I had planned to remove > it completely. > > >> usr/src/cmd/distro_const/DC-manifest.defval.xml >> >> - Did you verified that the appropriate default value >> is assigned after you added all the skip_if_no_exist things? >> For example, if I have a manifest that have the img_params section, >> but I didn't specify some of the values in the manifest, >> did the "right" value get assigned? >> > > Actually I did not. I'll double check that. Do you suspect that it > doesn't work? > I am not very familiar with that piece of code, Jack would know for sure. However, like others who have noticed that we added a lot of skip_if_no_exist to this file, I wonder how it will work. Additionally, if you enforce either img_params or vmc_params must be specified, would we be able to not have all these skip_if_no_exist things then? > >> usr/src/cmd/distro_const/utils/im_pop.py >> >> - This just moves the "original" im_pop step into a finalizer script. >> The bug (6366) requests for breaking up the im_pop step. Does this mean >> your changes will not address the bug? >> > > That is correct. It was going to add significant time to the VMC > schedule to break this up as outlined in 6366 which I deemed out of > scope for VMC since that's not what VMC needed. > > OK. I will leave 6366 open for that work then. I was going to assign you to the bug. :-) Thanks Glenn. --Karen
