CompositionalIT / farmer

Repeatable Azure deployments with ARM templates - made easy!
https://compositionalit.github.io/farmer
MIT License
527 stars 159 forks source link

Support for Elastic Pools #166

Closed sWW26 closed 4 years ago

sWW26 commented 4 years ago

Would be nice to extend the SQL Azure builders to include elastic pools. https://docs.microsoft.com/en-us/azure/templates/microsoft.sql/2014-04-01/servers/elasticpools

Also would be nice to be able to build the SQL server separately from the databases similar to how you can create service plans separately from webapps.

isaacabraham commented 4 years ago

What's the use case for creating a server without a database?

sWW26 commented 4 years ago

Allows assigning multiple databases to the same server without creating a single database first then accessing its server property. Perhaps there is another way to do it but the way I'd like to do it is something along the lines of the following (I have about 20 databases in one elastic pool):

let sqlServerResource = sqlServer {
    name "server-name"
}

let elasticPoolResource = elasticPool {
    name "pool-name"
    link_to_server sqlServerResource.Name
}

let databases =
    dbNames
    |> List.map (fun n ->
        sql {
            name n
            link_to_server sqlServerResource.Name
            elastic_pool_name elasticPoolResource.Name
        })

However I've just spotted #79 so maybe you're trying to move away from this style?

isaacabraham commented 4 years ago

For an elastic pool, I wonder whether we can simply have a flag on the sql { } builder that has the pool settings?

isaacabraham commented 4 years ago

The "creating multiple items" is a good point though and one I hadn't considered.

isaacabraham commented 4 years ago

Something I don't want is to create several builders, one for each low-level ARM resource - that kind of defeats the object of the builders. We should be thinking in terms of logical, high-level use cases.

sWW26 commented 4 years ago

I quite like the pattern that's in place for web apps, where you can have the builder create the service plan for you or you can create it separately and link to it. Best of both worlds?

isaacabraham commented 4 years ago

Exactly what I was thinking of. I've started with the integrated version; how does this look:

let db = sql {
    server_name "isaacssuperDbServer"
    admin_username "isaac"
    sku Sql.PoolSku.Basic50 // specify PoolSku rather than DbSku - tells Farmer to create a pool and link this DB to it.
    name "isaacssuperDb"
}

let db2 = sql {
    link_to_server db.Server
    link_to_elastic_pool // connect to the pool in the server
    name "isaacssuperDb2"
}

let deployment = arm {
    location Location.NorthEurope
    add_resource db
    add_resource db2
}
sWW26 commented 4 years ago

Looks good for the all in one approach, would want elastic_pool_name available too.

Would you be open to a PR that would also allow for the more manual approach, perhaps a single SqlServerBuilder that follows the same pattern with the PoolSku?

isaacabraham commented 4 years ago

yes, sure, as long as it works cohesively :-) I'll finish off this and merge in, you can then work off of that?

sWW26 commented 4 years ago

Sounds good

isaacabraham commented 4 years ago

@sWW26 The initial work will be merged in shortly. I'm thinking it's worth looking at a few ways of modelling this. The way we currently have things done is that all of these "many-to-one" resources sit in a kind of denormalized format where both resources are collapsed into one. I'm still not sure the best way to go on this as best practice advice. For getting started - particularly when there's just a single instance of the "child" resource - the collapsed approach seems good. But if there are many children, it doesn't seem to fit so well.

Could be worth looking at moving the db into the sql instance e.g.

let server = sql {
   name "dbserver"
   add_databases [
      sqlDb { name "mydb" }
   ]
}

The pool could sit inside the sql { } block too (can you have many pools per server? can a pool support multiple servers?).

We could also look at create an elastic pool as a separate builder completely:

let pool = elasticPool {
   name "mypool"
}

let server = sql {
   name "dbserver"
   add_databases [
      sqlDb { name "mydb" }
      elastic_pool pool
   ]
}

etc..

sWW26 commented 4 years ago

Having poked around in the Azure portal it looks like it is possible to have more than one elastic pool on a single server although I've never used that feature.

Could possibly have that add_databases function on both the sql builder and the elastic pool builder? So if you are using a pool you could go with something like:

let server = sql {
    name "dbserver"
    add_elastic_pool
        elasticPool {
            name "mypool"
            add_databases [
                sqlDb { name "mydb" }
            ]
        }
}
isaacabraham commented 4 years ago

Realistically, how often is this likely to happen? I would be inclined to target 80% of the most likely scenarios i.e. zero or one elastic pool per server in order to keep things simple. Then we can use this model:

let server = sql {
   name "dbserver"
   use_elastic_pool Sql.PoolSku.Basic50 // specify PoolSku at the server level to create a pool
   add_databases [
      sqlDb {
         name "mydb"
         attach_to_pool // something like this? either that or we simply say "if you have a pool, all databases automatically become part of the pool"? Now no need to set sku at DB level.
      }
   ]
}

This has the knock on effect of simplifying the builder for SQL in that there's no need to do the "attach" thing.

sWW26 commented 4 years ago

Yeah, not sure why you'd have multiple pools on a single server. I think there could be a case for having some databases in the pool and some separate but ones outside the pool would need to have a SKU set. So I guess the validator could make sure that the db was either attached to a pool or had a SKU specified?

isaacabraham commented 4 years ago

Exactly.

  1. If you set the elastic_pool SKU at the top level then databases could implicitly sit inside it, unless you specific a sku (in which case they sit outside the pool). OR
  2. we could be more explicit and have a flag at the DB level which is that attach_to_pool. But this still leaves the possibility to get things "wrong" anyway.

In the interests of keeping things simple from an API point of view, I would go with option 1.

sWW26 commented 4 years ago

Yep sounds good to me. For my case I'd also need a way to specify a specific name for the pool, I guess that could be an overload that takes a name in addition to the Sku?

isaacabraham commented 4 years ago

@sWW26 I don't think computation expression keywords allow overloads of different numbers of arguments, so that might not work

isaacabraham commented 4 years ago

Maybe a second keyword - elastic_pool_name or something?

sWW26 commented 4 years ago

I was looking at what else I use in my current ARM template and declare these properties:

{
    "name": "[variables('elasticPoolName')]",
    "type": "elasticPools",
    "apiVersion": "2014-04-01",
    "location": "[resourceGroup().location]",
    "properties": {
        "edition": "[parameters('elasticPoolEdition')]",
        "dtu": "[parameters('elasticPoolDtu')]",
        "storageMB": "[parameters('elasticPoolStorageMB')]",
        "databaseDtuMax": "[parameters('elasticPoolDtu')]"
    }
}

I'm wondering if its going to be cleaner to have the use_elastic_pool take an elastic pool from a builder, e.g.

use_elastic_pool
    elasticPool {
        name "myPool"
        sku Standard
        storageMB 250000
        dtu 400
        databaseDtuMax 200
    }
isaacabraham commented 4 years ago

If you think that those properties are commonly modified ones, yes - the alternative is to have them as standard keywords on the server itself and set some sane defaults there.

I'm not a massive fan of CEs as arguments to keywords as you can't put them inline without parentheses etc. sadly.

sWW26 commented 4 years ago

I'm not sure which are commonly modified I'm afraid, only what I've used 😄

Don't really have a preference on which way to go, although I wasn't aware of the CE argument issue 😞, so maybe having them straight on the server builder is cleaner.

isaacabraham commented 4 years ago

@sWW26 not sure if you're actively working on this - I could have a go at at least moving the sql server and db to their own builders as a first step?

isaacabraham commented 4 years ago

@sWW26 All done, I think. It wasn't a lot of work in the end to get it working fully with pools once I'd split out the server and database elements. Can you review the PR and tell me whether you think it works for you? I've done an end-to-end, it seems to function. Note, there are some pretty complex combinations of min/max/capacity depending on the SKU: https://docs.microsoft.com/en-us/azure/azure-sql/database/resource-limits-dtu-elastic-pools. I haven't done validation checks for this!