Hi Jack, Thank you for reviewing this. My comments inline..
> Hi Sarah. > > Here are my comments on the "New Solaris Install Architecture > Specification". Seems like you aptly summarized the impossible here, > with a good balance of one-size-fits-all generality to suit the engine > combined with enough details to describe it. > > General: Driver Update is not mentioned anywhere, but it is ancillary to > getting the system to an installable state. Does it need to be? > I don't think it does need to be mentioned. This architecture describes the 'core installation' only. Driver update, while important to getting a system that can be installed, is an outside player. > Section 1: > > second to last bullet: "bootable media formats" instead of "media > formats"? I need to think about this a bit. I get what you are saying, I am just not sure that all the 'media' we product has to be 'bootable'. > second to last PP: semicolon after "installing a set of packages" > Ok, will fix. > second to last PP: 1st sentence was confusing: Could the first type of > components be described as "... a set of application components that are > used to implement higher-level functionality..."? (I assume that > installing a set of packages is an example here because this > functionality is layered on top of the pkg(5) command itself.) What we intend to describe by this is that the these components(the first type), are very specific to the application that uses them. For example, we will have a 'Media Creation' component for the distro constructor that is specific to the distro constructor. It isn't about higher lever functionality(although that is certainly true), it's about it being application specific functionality, that isn't common across the Caiman installer apps. If you have suggestions on how to describe this in a less confusing way, let me know. > > Section 2: > > I would restructure the first PP since "various components provided by > this project" leaves a hole which is filled in later, which made it > confusing to me. I would say something like "The core engine classes > provide functionality needed by an application's specific components > (provided by various subprojects) as well as the common application > classes. Not sure how to integrate the installation flow concept into > this if you want to keep it though. We need to keep some mention of the installation flow, because really the Core Engine classes provide the hooks to the other classes to instantiate this install flow. Let me think on this a bit and try to reword. > > I've noted next to the three bullets: > - first bullet is "pieces of an installation" > - second bullet is "communication between the pieces" > - third bullet is "external communication from pieces to the outside" > Including this may be useful, to help simplify what is being explained. Ok, this is good data. I will rework this section. > Section 2.2: > > second bullet: sounds too specific to AI. Leave AI out of it, and just > say "manifest". Actually, it is specific to an AI manifest. The idea is that we want to generate an AI manifest so that the user can recreate the install they just performed using liveCD or text installer, using AI. So, they have an automated, hands-off way to replicate the install. Does this make sense? > retrieve_data(): This methods provides -> This method provides Fixed. > Below last clump of three bullets: This method -> These methods Actually, it is just referring to the last bullet,dump_cache(). I will add descriptions to the other two bullets to make this clear. > Section 2.3: > > Anal nit: just above the bullets: A Logger -> A Logger object fixed. > Section 3: > > Second PP: Why limit yourself: second sentence: "AI and DC currently > utilize XML manifests." For reference to the readers who may not know about all of the Caiman installers. I will reword to make this clear as a reference point only. Not to limit the use of an XML parser. > Third PP: clarify what a target is: "Target Discovery provides the > initial system discovery of disks and partitions available as potential > installation targets." Ok, will reword. > Fourth PP: I don't think common application class needs to be > capitalized anywhere. If you decided to capitalize anyhow, "class" is > inconsistently capitalized. Fair point, I will make it consistent. > > Section 3.2: > > First PP: I would delete the word "logical", as it hasn't been defined > yet. (It is defined in the next PP.) I don't want to delete the use of 'logical'. I understand what you are getting at, but I don't think that moving on paragraph down is a big leap in the understanding of 'logical'. > > Second PP: Add "boot environment" to the list of logical objects listed > in the first sentence. This sentence isn't intended to list all logical objects, but I can see where this might look like that's what we are trying to do. I will reword. > > Last PP before 3.2.1: First sentence seems to detailed to me. I think > you are saying that the objects need to be initialized to represent a > certain type of target. This is done with a seed value which is a > DeviceTarget or a LogicalTarget specification. I like this wording better. I will rework this. > The last sentence is "See section 3.2" but we're already in section 3.2. Ah.. good point. I will fix. > Section 3.2.1: > > Second sentence: > "These targets are represented by two class types: those..." Will fix. > Section 3.2.1.1: > > General: > > Do we want to assume people know the connection between disks, > partitions and slices? No, I can add wording that describes this for novice users. Good suggestion. > > Disk class: It is unclear what set/get_assoc_parts() and > set/get_assoc_slices() do. Other functions are clearer only because of > my background. Others may not know about them either. Probably a good > idea to have a quick description of what all methods do (though not > necessarily an individual description for each one). > > OK... now I see it. May be less confusing to drop the "assoc" and just > call set/get_parts() and set/get_slices(). Let me think on this a bit. I actually had get_parts/set_parts(), etc.. but felt that having the method describe the association was clearer. Apparently not :-). > What is the difference between get_assoc_parts() and get_part_map()? > set_assoc_parts() and set_part_map()? > set_part_map() and create_part_map()? The get_assoc_parts() method returns a set of partition objects. the get_part_map() method returns a partition map, which is simply a description of the partition layout. Not the actual partitions. This was added to enable the creation of the partition table when doing an install. > Slice class: > > Does get/set_assoc_part() make more sense than get/set_assoc_parts()? > It does in terms of the functionality but I was trying to keep the method names the same. It seems to me that this method should be in the super class and it should be implemented, as required, by the subclasses. > 3.2.1.2: > > The listed logical device base classes are out of order from the rest of > the document, and Dataset class is missing. I will reorder. Filesystem is the super class for Dataset. I didn't list it because it isn't a base class. > > Zpool > LogicalVolume > BootEnv > Filesystem > Dataset > VMEnv > Zone > > LogicalTarget interface methods: > get_assoc_container() PP: last sentence: "This method allows us..." Fixed. > Zpool class: > PP below set_props: It says "Thus one method to manage these is > sufficient." Yet, there are two methods listed :) > Fixed:-). > Dataset class: > First PP: > First sentence: get rid of the "s" in datasets. > Third sentence: "interfaces" -> "methods" to be consistent. Fixed. > VMEnv class: > The content of this PP seems tilted toward xVM. What about Virtual Box? > LDoms? > > The VMEnv class has no methods. Is the point of the VMEnv class to > "mark" an environment as a virtual machine environment? Is it an > interface spec which can be combined with other interface specs in this > section (e.g. LogicalVolume that defines a lofi-mounted file enveloping > the VM environment)? We don't create VMEnv's as part of our installation, so this class is simply a class that allows for discovery of VM's so that the installer can show the details of the existing environments. It wasn't my intent that it be combined with other interfaces, it is simply a way to distinguish the existing environment's targets. However, in responding to you I realized that some of the details of the associations for VMEnv's are not specified. I don't intend to include VBox(type 2) in this class. LDoms are type 1, as is xVM. So, in this case there is a 'storage' device mapped to the domain and that is something we need to be able to show the associations for. Storage can be zvol, file, or physical device. anything that looks like a block device. With Vbox, they manage the 'storage' via a file, we can define its location, but it is simply a file. So, when the 'storage' for a VM is not a physical device, the associations will show up via a Zpool class. The file must reside somewhere so any storage allocated to the datasets will be shown as 'in use' When 'storage' is a physical device we want to show that the allocated device is part of a VM. I need to work on this class a bit more since it is not complete(as I talk through my response). Why not type 2 hypervisors? Because they don't matter in terms of target discovery, or target instantiation(because we don't create them). They are backed by a file, which will be contained in a dataset, contained in a pool, so we don't have to manage this association knowledge. > > If VMEnv can be combined with other interfaces, it seems unique in this > respect. Other interfaces, even logical ones, define "physical" > attributes (e.g. snapshots of boot environments and zfs datasets). > VMEnv would rely on the "physical" attributes of what it is combined > with to define its location characteristics, and VMEnv would serve only > to mark an environment as a Virtual Machine. > If this is the case, you may want to call this out in the document. If > not, please clarify in the document how it is used. > > 4.1: > > This is a public document, and the link to securewiki is a sun-private > link. I didn't even realize that was in there. I will move the diagram to openoslaris. Good catch! > 4.2.1: > Drawing 4.2: > > The sequence diagram appears to have several mismatched or unmatched > parentheses after method names. Some methods have [], some have (], > some have only (. Not sure why that would be. I let netbeans do the drawing, but I will take a look at this. > I don't understand the store_data() arrow going from Target > Instantiation (TI) to Data Collection coming before the > get_data_collection() arrow going from TI to the Execution Engine? > Ditto for Transfer, Instance Configurator. (Looks like Target Discovery > got it right, though.) It's not supposed to do that :-). Looks like what is missing is that this is for a 'get_data()' call, and that I need to add the get_data_collection() call before it. Target Instantiation, and the others you mention need to get data from data collection before proceeding, and that isn't correctly shown in the diagram. I will fix. > Looks like Instance Configurator block got pasted twice. > It did.. I will fix. > 4.2.2: > Drawing 4.3: > > The "P" of "public" is missing or partly missing in many places. This has to do with the way the UML diagram copied to an image. And, the way the classes are separated(not far enough). I will see if I can make this better. > All methods have no closing ). Same reason as above. > I'm a newbie at UML. That said, I interpret the rightmost :disk, > :partition and :slice columns to be loop parameters (i.e. :disk means > "loop for each disk found"). If this is incorrect, please let me know > and disregard the following: > I think that when you look at the columns, the partition loop envelopes > the slice loop. (That is, the right most :partition column represents > the looping through all partitions, and following that column down, we > enter the slice loop for that partition.) This seems correct to me. > But the box enveloping the slice loop is outside the box enveloping the > partition loop. Shouldn't the slice loop box be inside the partition > loop box, much like the partition loop box is inside the disk loop box? The outermost loop box is for all of 'Target Discovery'. Just to be clear. The looping, for slice discovery comes from the 'Partition' object. If you look at the diagram, the creation of the Slice object happens from a Partition object. It is hard to see, but the intent is that a Partition starts discovery for its slices. The loop boxes do not have to be overlapping or embedded in each other. The loop box is intended to show what the constraint is, discover != NULL, that's it really. It's hard to show this with UML. > 4.2.3: > Drawing 4.4: > > Is the reason why there is no instantiate() under the :disk loop because > the disk is a physical medium? To me, instantiate() creates an object > representation of something (e.g. an object representing a disk), as > opposed to the thing itself (e.g. the disk). If this is the correct > interpretation, there is a missing instantiate() in the :disk loop. > Yes, basically it's because we don't create a disk target during install, we use them, but they are set. The instantiate() method takes the populated instance data and creates the corresponding device on the system. Probably a bad choice of naming, which is why you are confused. I was going to use 'create()' but that seemed wrong as well. Any suggestions? So, no, there does not need to be an 'instantiate()' method for a Disk since it isn't something the installer creates. > 5.1: > Item 4: It may be that this level of detail is needed, but to say > "reverse any media-specific customizations" sounds very specific (e.g. > undoing what slim-utils does to set up "jack"). Would it be better to > say "customize the installation to the application's requirements"? Fair point, I will change. > Drawing 5.1: I think the XML Manifest shown in the picture represents a > saved file. If this is the case, remove "DOM". DOM pertains to the > representation of the data in memory, not in a file. > Ok, will fix. It is a saved file. > 5.2: > > Nit: extra space between "via" and "a" in first sentence. Fixed. > Item 1: First sentence: "...Manifest Retrieval, Manifest Parsing and > Target Discovery." (Target Discovery needs to be added.) > Fixed. > Second sentence: Do you mean the manifest name is fetched from a service > property, and the service having that property is specified in the grub > boot properties? If yes, can that be made more clear? yes, that's what we mean. I will clarify. > Drawing 5.2: Same comment about "DOM" as for drawing 5.1 > Ok, will fix. > 5.3: > Item 1: May wish to clarify that the "target" being instantiated here > refers to where the image will be built. Ok, I will add. > Drawing 5.3: Target discovery not needed in the diagram. You are right. We initially were planning on doing some discovery of the zfs dataset but decided we didn't need target discovery for this. > 5.4: > Seems target discovery is needed to find where to plop down the boot > image. It is missing from the text. (It is in drawing 5.4 though.) I don't think we need target discovery actually. I would think the user of installadm would specify the dataset, much like we do for DC. I think it is incorrect in the diagram. Thanks again for your detailed review. Much appreciated! sarah **** > Thanks, > Jack > > On 12/01/09 14:18, Sarah Jelinek wrote: >> Hi Caimaniacs, >> >> I know you have been waiting for this bestseller to hit the shelves! :-). >> >> The Caiman team has been working on an updated architecture and we >> have the architecture document ready for review. The >> opensolaris-caiman project architecture page is located here(formerly >> known as Caiman Unified Design): >> >> http://hub.opensolaris.org/bin/view/Project+caiman/CUD >> >> The Caiman architecture document open for review is located here: >> >> http://hub.opensolaris.org/bin/download/Project+caiman/CUD/caimanarchitecture.pdf >> >> >> >> Please send comments/questions to caiman-discuss by 12/18/09. If you >> need more time please let us know, with the holidays coming up we may >> have to extend the review period. >> >> Thank you for your time. >> >> Regards, >> sarah >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
