MAAS

Code review comment for lp:~julian-edwards/maas/models-for-static-dhcp

models-for-static-dhcp
Merge into trunk

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2014-06-02:

Thanks for reviewing jtv.

On 01/06/14 23:14, Jeroen T. Vermeulen wrote:
> Review: Approve
>
> Please document IPAddress. It's really a static IP address, not just any dynamic-or-static IP address, right?

Woops, I had meant to do that and forgot. I've expanded the docstring
now, thanks for spotting!

> Why the foreign key from IPAddress to MACAddress in the first place? Wasn't that going to be a many-to-many relationship? Or is this transitional?

See the maas-devel thread for gory details, but the upshot is that we
potentially need to have many IPs per MAC (VLANs, NIC aliases etc), but
we don't need to model many MACs per IP (VIPs) as we'll do that by
having a per-user IP.

> I'm not holding this up with my vote — whatever comes out, no harm done at this stage. Better to land and improve than to fork and languish. But to me, the "type" field affects the behaviour of the foreign key in too profound a way.
>
> The biggest difference I see between the four types of relationship is what happens when a node is deallocated. If an IPAddress is "auto" or "extra," it goes down with the ship. If it's "unmanaged" or "sticky," it survives. I think those are different structural relationships, so maybe they should be two alternative foreign keys, with different cascading behaviours.

Again please see the maas-devel thread for politics behind this :(

> Of course my "biggest difference" only gets us to 2 types, out of 4. What are the other differences? It seems to me that:
>
> * "Auto" is like "extra" except it can't be deallocated separately. Could be a separate relationship, or just a boolean attribute.

Whenever you have a boolean attribute, an enum is nearly always what you
want instead, and better. Bools as column types are for mugs IMO, they
restrict what you can do, and I've seen many times where the LP schema
had migrations to move bools to enums as the original design had not
been thought through properly.

> * "Unmanaged" is like "sticky" except the machine on the other end is not a node. But does that matter beyond creation and display? If you set an IP address for a machine's MAC address, then registered that machine as a node, ISTM the IP address would implicitly go from "unmanaged" to "sticky" without affecting the world in any way.

You might have a point here. Let's leave it for now since it doesn't
affect the model itself and is a small implementation detail. We can
always change it later very easily and we'll encapsulate all this detail
inside the manager class anyway.

> There may also be cardinality differences. I can't imagine attaching two MACs to the same "auto" address, but it might make sense for "extra" addresses. Splitting out the relationships may make the rules clearer, and easier to enforce as constraints.

There is no way to attach more than one MAC to an IP with this model.

Thanks for reviewing jtv.

On 01/06/14 23:14, Jeroen T. Vermeulen wrote:
> Review: Approve
> 
> Please document IPAddress.  It's really a static IP address, not just any dynamic-or-static IP address, right?

Woops, I had meant to do that and forgot.  I've expanded the docstring
now, thanks for spotting!

> Why the foreign key from IPAddress to MACAddress in the first place?  Wasn't that going to be a many-to-many relationship?  Or is this transitional?

> I'm not holding this up with my vote — whatever comes out, no harm done at this stage.  Better to land and improve than to fork and languish.  But to me, the "type" field affects the behaviour of the foreign key in too profound a way.
> 
> The biggest difference I see between the four types of relationship is what happens when a node is deallocated.  If an IPAddress is "auto" or "extra," it goes down with the ship.  If it's "unmanaged" or "sticky," it survives.  I think those are different structural relationships, so maybe they should be two alternative foreign keys, with different cascading behaviours.

Again please see the maas-devel thread for politics behind this :(

> Of course my "biggest difference" only gets us to 2 types, out of 4.  What are the other differences?  It seems to me that:
> 
>  * "Auto" is like "extra" except it can't be deallocated separately.  Could be a separate relationship, or just a boolean attribute.

Whenever you have a boolean attribute, an enum is nearly always what you
want instead, and better.  Bools as column types are for mugs IMO, they
restrict what you can do, and I've seen many times where the LP schema
had migrations to move bools to enums as the original design had not
been thought through properly.

>  * "Unmanaged" is like "sticky" except the machine on the other end is not a node.  But does that matter beyond creation and display?  If you set an IP address for a machine's MAC address, then registered that machine as a node, ISTM the IP address would implicitly go from "unmanaged" to "sticky" without affecting the world in any way.

You might have a point here.  Let's leave it for now since it doesn't
affect the model itself and is a small implementation detail.  We can
always change it later very easily and we'll encapsulate all this detail
inside the manager class anyway.

> There may also be cardinality differences.  I can't imagine attaching two MACs to the same "auto" address, but it might make sense for "extra" addresses.  Splitting out the relationships may make the rules clearer, and easier to enforce as constraints.

There is no way to attach more than one MAC to an IP with this model.

« Back to merge proposal