Code review comment for ~mpontillo/maas:default-dns-ip--bug-1776604

Revision history for this message
Mike Pontillo (mpontillo) wrote :

Let me explain a little more about what is going on here.

The get_hostname_ip_mapping() function basically takes in a subnet or domain, and returns a dictionary of {hostname: (ttl, [ip])} mappings.

Most of the function is a self-described "SQL horror". The reason for that is, we want to be able to do this in a single query for performance reasons. (Doing this in Django would be even worse.)

Previously, the SQL query itself tried to ensure that only one mapping would be returned per (fqdn, address_family) pair. It did this by using a SELECT DISTINCT to try to determine whether or not the interface was the boot interface. But this wasn't enough, because we have to account for a few different scenarios:

 - There might be many boot interfaces per machine, depending on the interface hierarchy. For example, if we have a physical interface 'eth0' and we boot from it, that's great, but what if the user also configured an 'eth1' and bonded {eth0, eth1} together? We won't see a proper IP address assigned to eth0 or eth1 in that case, so we need to dig deeper.

 - In addition to the above point, you might have a bridge interface whose parent is the bond (where your IP addresses are actually assigned). That means you have to check if the parent's parent is a boot interface before determining if it should get the default hostname for the machine. So the SQL had to be changed to account for the parent-of-a-parent-being-a-boot-interface scenario.

 - If we don't include the preference in the SELECT DISTINCT, we can't include it in the ORDER BY statement, otherwise the following error is raised:

    django.db.utils.ProgrammingError: SELECT DISTINCT ON expressions must match initial ORDER BY expressions

   ... therefore, the SELECT DISTINCT must include the preference, and we must filter out the rows we don't want in the Python code. (That's what the Python portion of this change does.)

« Back to merge proposal