On 14/06/2011 16:33, Kristina Tripp wrote: > Darren, > > I assume it's okay for me to check-in these changes now. I've updated the > webrev. > See below for some additions comments
I'm happy... :) > > On 06/14/11 02:11 AM, Darren Kenny wrote: >> Hi Kristina, >> >> On 13/06/2011 20:15, Kristina Tripp wrote: >>> Thanks for the review. Comments inline >>> >>> On 06/ 9/11 02:01 AM, Darren Kenny wrote: >>>> Hi Kristina, >>>> >>>> Apologies for not getting to this sooner, it's code I'm not familiar with >>>> in >>>> any way. >>>> >>>> Overall things are looking good, but I do have some questions/comments: >>>> >>>> - In conv.py: >>>> >>>> - The method __create_any_boot_disk() doesn't appear to be of any use, >>>> should it be still there? Lines 1710-1716 suggest that something >>>> should be >>>> done. >>> Removed >>>> - In __add_device, line 705-708, you appear to be creating a diskname >>>> node >>>> in-line rather than using the method __create_diskname_node(), why not >>>> use >>>> the method? > Forgot in my last mail fixed. >>>> - In __add_device, line 709, you seem to add a partition, but this should >>>> only be done for an x86 based profile, if it's SPARC based the >>>> generated >>>> manifest will fail, on SPARC slices are direct children of the <disk> >>>> node. While the DTD will validate, AI itself will fail. >>> Interesting. This isn't something I was aware of and a problem since we >>> typically have no notion of what platform the system is intended for. A few >>> of >>> the Jumpstart keywords are targeted for are for sparc or x86 and I can >>> certainly >>> address it when I have a notion of the architure of the system. But it's >>> possible to generate a Jumpstart profile that requires me to generate a disk >>> entries for the root ZFS pool that I'll have no notion of the architecture. >>> >>> Are there any options available to me? >> I've sent an e-mail to caiman-discuss about this issue, to see what other >> people >> think is a reasonable approach for handling this in AI. >> >> I could add a specific exception on SPARC to not fail, but just ignore things >> for this specific case, or I could ad a more general "ignore all partitions". >> >>> If your agreeable I'll write a CR to address this since this is going to >>> require >>> some large changes if there is no common way to make this work for both >>> platforms. Instead of >>> trying to fold this into this current set of changes. >> I think a separate CR would be fine - since it's looking like the best way to >> resolve this would be to change AI's behaviour. >> > Ok. It still looks like I should modify it to not output the partition when I > can determine > the profile is meant for a sparc system. I filed CR7054528 to capture the > problem. > Thanks. >>>> If you've not done it already, it would really be worth running the >>>> generated XML manifests, on snv_167+, through a call to: >>>> >>>> auto-install -i -m <manifest> >>>> >>>> The -i will stop before doing anything destructive, of course this >>>> would >>>> also assume that the machine has the right devices. >>>> >>>> Or alternatively, in the tests, you could take a look at the >>>> $SRC/cmd/auto-install/test/test_target_selection_*.py and mimic what it >>>> does in calling select_targets, passing your XML to it to ensure that >>>> it >>>> will get through that validation at least. >>> We currently validate the manifests and sc profiles as part of the >>> conversion >>> process. >>> All the unit tests all perform validation of the generated xml code. >>> >>> The current validation occurs via the ManifestParser. Ethan did question >>> whether >>> we wanted to do the validation against the image associated with a service. >>> >>> This may be desirable since then I could use the validation code used by >>> installadm. The ManifestParser code base is causing me problems on the >>> backport >>> since it wants to pull in all the target/libbe >>> code. >>> >> Maybe you need to also set the LD_LIBRARY_PATH to the image's libraries >> directory to pick up the image's libbe? Would that work? > Not really. The case I'm referring to is for the Solaris 10 backport. I've > been unable > to get the Manifest parser backported to Solaris 10 since it's pulling in > stuff > that doesn't > exist. If there's other components that are part of the AI Install Server > that > require > libbe then I'll end up have to backport that but for now I cann't see anything > other than > js2ai. Although installadm is the only command I've got fully working on > Solaris 10 at > this point in time. > Ah, get you now. > Switching the validation used by installadm looks like it would solve my > problem. I just > need to explore it to see if it will. It will change the behavior since it > appears that > a service needs to be created that the profiles can be generated against. > This > in itself > creates problems for me since we currently have no notion of versions of the > manifests > and sc_profiles to account for changes at various stages. > > Just a problem that needs to get solved at some point. Well there are two types of validation: 1) Syntactic / Well-formed XML You could do this using xmllint --validate --noout <file> 2) Semantic validation, unfortunately this really will only ever get done at run-time on the install client since we're using DTDs rather than something like RNG now. This would be the one that would check if someone is doing some simple mistakes like creating a partition with a name 'fred' rather than a numeric value between 1 and 32. Although, it could be worth looking at having an RNG (or some other similar semantic capable mechaism) just for this purpose - but I'm not 100% sure this would work, but it just might work in the S10 case... Thanks, Darren. _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
