Module to upgrade servers

[Juanadelacuesta]

Description

This PR introduces a module to upgrade the servers one by one. It also adds it to the scenario.

Testing & Reproduction steps

Links

Contributor Checklist

• Changelog Entry If this PR changes user-facing behavior, please generate and add a
  changelog entry using the make cl command.
• Testing Please add tests to cover any new functionality or to demonstrate bug fixes and
  ensure regressions will be caught.
• Documentation If the change impacts user-facing functionality such as the CLI, API, UI,
  and job configuration, please update the Nomad website documentation to reflect this. Refer to
  the website README for docs guidelines. Please also consider whether the
  change requires notes within the upgrade guide.

Reviewer Checklist

• Backport Labels Please add the correct backport labels as described by the internal
  backporting document.
• Commit Type Ensure the correct merge method is selected which should be "squash and merge"
  in the majority of situations. The main exceptions are long-lived feature branches or merges where
  history should be preserved.
• Enterprise PRs If this is an enterprise only PR, please add any required changelog entry
  within the public repository.

[tgross]

It's probably less "best practices" and more "what our packaging does".

[tgross]

Without the -stale flag, this is always getting forwarded to the leader.

[Juanadelacuesta]

Let me test with the stale flag and specifying the server address

[gulducat]

looks great!

just a few comments for your consideration, but no blockers.

[gulducat]

this won't hurt anything, but I don't believe daemon-reload is required unless a systemd unit file is being changed. here only the thing changing is the binary, right? so only need to restart the agent.

[gulducat]

so the purpose of the snapshot is just to compact the state store, so that no log replay is necessary when the agent starts up?

[tgross]

Not just "no log replay necessary" but that we force the server not to use the logs, which don't pass thru the FSM restore code path in the same way.

[gulducat]

sure would be neat if TF gave us a way to serialize resource creation, alas. hashicorp/terraform#10802

[Juanadelacuesta]

declarative language..blah..blah...

[gulducat]

nit-picky: lots of redundant code around here, which is pretty normal for terraform, but...

I get why these have to be separate module calls, but the only thing that's changing here is the server_address, right?

what if all the other variables were a single object, so that a map of them all could be stored in a local? something like this in upgrade_instance/variables.tf:

variable "server_address" {
  description = "IP address of the server that will be updated"
  type        = string
}

variable "stuff" {
  description = "the rest of the stuff"
  type = object({
    # this object definition ensures all the required stuff is passed in
    nomad_addr                 = string
    ca_file                    = string
    cert_file                  = string
    key_file                   = string
    nomad_token                = string
    platform                   = string
    nomad_local_upgrade_binary = string
    ssh_key_path               = string
  })
}

and here in upgrade_servers/main.tf:

locals {
  stuff = {
    nomad_addr                 = var.nomad_addr
    ca_file                    = var.ca_file
    cert_file                  = var.cert_file
    key_file                   = var.key_file
    nomad_token                = var.nomad_token
    platform                   = var.platform
    nomad_local_upgrade_binary = var.nomad_upgraded_binary
    ssh_key_path               = var.ssh_key_path
  }
}

module upgrade_second_server {
  depends_on = [enos_local_exec.take_second_cluster_snapshot]

  source = "../upgrade_instance"

  server_address = var.servers[1]
  stuff          = local.stuff
}

I really like this pattern (sometimes, especially with sub-modules like this) -- like passing in a reusable config struct in golang, instead of a bunch of separate func parameters.

[Juanadelacuesta]

Did my best to group things that made sense :)

[gulducat]

these calls could also be shortened with a local env map:

locals {
  nomad_env = {
    NOMAD_ADDR        = var.nomad_addr
    NOMAD_CACERT      = var.ca_file
    NOMAD_CLIENT_CERT = var.cert_file
    NOMAD_CLIENT_KEY  = var.key_file
    NOMAD_TOKEN       = var.nomad_token
  }
}

resource "enos_local_exec" "second_leader_verification" {
  depends_on = [module.upgrade_second_server]

  environment = local.nomad_env

[Juanadelacuesta]

Im new to terraform, I will take a look :) Thank you!

[tgross]

LGTM 👍

[tgross]

Do we want to get rid of these now that we have that top-level tls object variable? (Which I like better for sure 👍 )