canonical / terraform-provider-maas

Terraform MAAS provider
Mozilla Public License 2.0
64 stars 46 forks source link

Many functions are calling the machine listing to retrieve a single machine, overloading the MAAS regions #239

Open r00ta opened 3 weeks ago

r00ta commented 3 weeks ago

I found out that in TF there are many calls to

func getMachine(client *client.Client, identifier string) (*entity.Machine, error) {
    machines, err := client.Machines.Get(&entity.MachinesParams{})
    if err != nil {
        return nil, err
    }
    for _, m := range machines {
        if m.SystemID == identifier || m.Hostname == identifier || m.FQDN == identifier || m.BootInterface.MACAddress == identifier {
            return &m, nil
        }
    }
    return nil, fmt.Errorf("machine (%s) not found", identifier)
}

which means that in order to get a single machine the machine listing is called. The machine listing is the most expensive operation and for this reason it should be avoided as much as possible.

Instead, these functions should specify the identifiers in the machine listing so to perform the lookup on the server and improve a lot the server performances. If some identifiers are not supported by the current API, we should improve the API so to make it possible.

sempervictus commented 3 weeks ago

can we cache the machines list from the first call for the duration of the run?

sempervictus commented 3 weeks ago

the "short circuit" approach for the >40 calls across the provider for that would be something like

diff --git i/maas/resource_maas_machine.go w/maas/resource_maas_machine.go
index fff14e0..da5c63f 100644
--- i/maas/resource_maas_machine.go
+++ w/maas/resource_maas_machine.go
@@ -351,6 +351,10 @@ func waitForMachineStatus(ctx context.Context, client *client.Client, systemID s
 }

 func getMachine(client *client.Client, identifier string) (*entity.Machine, error) {
+       machine, err := client.Machine.Get(identifier)
+       if err == nil {
+               return machine, nil
+       }
        machines, err := client.Machines.Get(&entity.MachinesParams{})
        if err != nil {
                return nil, err

which should help those lookups a lot if the subordinate resources pass the system.ID to that call (what the gomaas client expects for client.Machine.Get vs Machines.Get)

sempervictus commented 3 weeks ago

240 addresses this for most call patterns we see. Not signing a CLA without legal review, not paying lawyers to review FOSS work. If Canonical wants FOSS developers to enter into contract to contribute, it should be prepared to compensate them for the work, the hassle, and the time to deal with that nonsense.