On Thu, Dec 08, 2011 at 10:51:07AM +0100, Agata Murawska wrote:
> On Wed, Dec 7, 2011 at 9:02 PM, Iustin Pop <[email protected]> wrote:
> > On Tue, Dec 06, 2011 at 06:52:10PM +0100, Agata Murawska wrote:
> >> Introduction of instance policy - the required defaults and types are
> >> provided, along with the syntax check and changes to config writer.
> >
> >> +# instance specs
> >> +MEM_COUNT_SPEC = "memory-count"
> >
> > Hmm, this seems badly chosen (was it like this in the design?). I would
> > rather expect MEM_SIZE.
> Ack, changed
>
> >
> >> +CPU_COUNT_SPEC = "cpu-count"
> >> +DISK_COUNT_SPEC = "disk-count"
> >> +DISK_SIZE_SPEC = "disk-size"
> >> +NIC_COUNT_SPEC = "nic-count"
> >> +
> >> +ISPECS_PARAMETER_TYPES = {
> >> + MEM_COUNT_SPEC: VTYPE_INT,
> >> + CPU_COUNT_SPEC: VTYPE_INT,
> >> + DISK_COUNT_SPEC: VTYPE_INT,
> >> + DISK_SIZE_SPEC: VTYPE_INT,
> >> + NIC_COUNT_SPEC: VTYPE_INT,
> >> + }
> >
> > also, the _SPEC suffix is somewhat opposite to a more common SPEC_
> > prefix, but that's fine (at least it has a suffix).
> Ack, I will submit a separate patch for that and for MIN_ISPECS ->
> ISPECS_MIN, if that is fine (since these are used in many patches in
> this series)
Yep, fine.
> >> +IPOLICY_EMPTY = {
> >> + MIN_ISPECS: {},
> >> + MAX_ISPECS: {},
> >> + STD_ISPECS: {},
> >> + }
> >
> > This is somewhat dangerous. Especially when you do this:
> >
> >> + # instance policy added before 2.6
> >> + if self.ipolicy is None:
> >> + self.ipolicy = constants.IPOLICY_EMPTY
> >
> > This should be constants.IPOLICY.copy(), but that's not enough since you
> > have embedded dicts.
> >
> > I would suggest a different way, replace IPOLICY_EMPTY with
> > makeEmptyIPolicy: return dict(...). Similar to how we build defaults in
> > the lib/ht.py code.
> Ack
>
> >
> > thanks,
> > iustin
>
> Interdiff:
LGTM, thanks.
iustin
