On Wed, May 22, 2013 at 2:04 PM, Bernardo Dal Seno <[email protected]>wrote:
> On 22 May 2013 12:33, Thomas Thrainer <[email protected]> wrote: > > > > > > > > On Mon, May 20, 2013 at 5:11 PM, Bernardo Dal Seno <[email protected]> > > wrote: > >> > >> The requested number of spindles is used to allocate PVs when creating > new > >> LVs. > >> > >> Signed-off-by: Bernardo Dal Seno <[email protected]> > >> --- > > > > > > As far as I've understood, the spindles parameter can only ever be used > by > > LogicalVolume and is ignored by all the other sub-classes of BlockDev. > The > > documentation of BlockDev.Create's params parameter says "...params: > > device-specific options/parameters". So why do we introduce a new > (required) > > parameter here instead of putting the spindles option in params, where it > > could be LogicalVolume specific? > > > > This comment goes beyond this patch, as previous ones have already added > > "spindles" to the Disk class. But the reasoning is the same: should we > add a > > specific field, which is only valid for a specific disk type, to the Disk > > class, which IMHO should only hold generic disk information? > > I asked myself the same question. Spindles are currently used only for > LVM volumes, but there is no technical reason they couldn't be used > for DRBD (and in fact they might someday) or a RAID-like storage > backend. When e_s is enabled, spindles will be used in place of disk > size for balancing, allocation, ipolicy checks, and space planning. So > I thought it would make more sense to have a "first-class" parameter. > Besides, the way the parameters are used today implies that they can > be inherited from the instance, cluster, node... and have a default > value, which make them a different kind of animals. What do you think? > > Well, from an OOD point of view, the Disk class is not really a shining example. It contains a couple of pseudo-switches (if dev_type in ... or similar) in order to compute stuff, where polymorphism could be used (those "switches" are littered all over the disk related code, actually). It has multiple responsibilities (serialization/deserialization vs. methods like StaticDevPath and GetNodes, it inspects opaque fields like in result = [self.logical_id[0], self.logical_id[1]], etc.), where this functionality should really be extracted in helpers, maybe in a parallel hierarchy. And it already contains at least one field, "children", which is AFAIK only used by DRBD. Having said that, I think that it's far from trivial to model a concept like "disks" in our domain properly. And as long as we don't feel enough pain caused by this such that we clean up the things I mentioned above, it's probably OK to add another field to the class. Especially as the "multi-purpose" parameter has different semantics (which is hopefully documented somewhere). So LGTM then, Thanks, Thomas > Bernardo > -- Thomas Thrainer | Software Engineer | [email protected] | Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Katherine Stephens
