• Is basing on HTTP health check more reliable than counting running tasks with aws ecs describe-services the same way you did below?
• Does it even work if you previously revoked security group ingress traffic?

Setting primary node desired count to 0, then back to 1 is not necessary when doing --force-new-deployment afterwards. I think you can get rid of these steps.

The "force new deployment" replaces old task with the new one for you.
However it does a no-downtime update (the old is killed after the new task is ready). If this is not desired, you can try merging --desired-count 1 and --force-new-deployment into one command

• I originally used aws ecs describe-services but found that it was still possibly for the health check to be successful even when aws ecs describe-services resulted supposedly in no running tasks. Using the HTTP health check ensures that the below check for primary service health doesn't immediately launch the secondary nodes while the primary service is being relaunched

• If allowed_ip is properly configured then yes. I ensure that only traffic from the allowed_ip is accepted with the below ingress rule:

ingress {
  from_port   = 443
  to_port     = 443
  protocol    = "tcp"
  cidr_blocks = ["${var.allowed_ip}/32"]
}

From what I understand, by setting the maximum deployment percent to 100 and minimum to 0, I've disabled no-downtime updates. With my current config, I monitored task deployment on console during the running of the script and confirmed there was only 0 or 1 tasks in progress at any moment

I do believe your single command is cleaner. Including in next rebase

I'm a bit confused about the term "migration" here. Is it referring to MariaDB migrations, and implying the script only needs to be used when the deployer knows that there is a pending MariaDB migration?

If that's the case, I have a couple of questions:

1. What's the workflow for deploying an update when there isn't a pending MariaDB migration?
2. How do we mitigate the risk of somebody deploying an update with a pending MariaDB migration without using this script? It seems like this could cause some DB corruption issues, so I'm a bit concerned

On the other hand, if this script is meant to be run for all AWS keyserver deployments, then perhaps it should be renamed to use "deploy" instead of "migrate". Maybe the script could be named aws-deploy.sh?

I might call this num_desired_secondary_nodes... it's unclear from context that it's a number, and could be interpreted as a boolean (whether we desire secondary nodes)

Are these copy-pasted from Terraform somewhere? Prefer to avoid copy-paste when possible, but if it's difficult to factor out given Terraform vs. bash, we should at least add a reference to both sides making sure that a dev doesn't update one without updating the other

I'm not 100% sure, but my impression was that it's safer to add quotes when making an assignment in bash

That said if Shellcheck didn't complain maybe it's safe?

You use a mix of bash conditionals: (( )), [ ], and [[ ]]. Do we really need all three? It seems like we should at least drop [ ] (my impression was that it's replaced by [[ ]]). Would be good to understand why you chose each of them

Nice work figuring this out!

Question for you – if somebody runs a terraform apply when this script is running, will that mess things up? And vice versa – what happens if terraform apply is running, and then somebody runs this script?

Nit: seems like extraneous newline

Much of this feels copy-pasted from check_health. Is there a way to factor it out?

Setting primary node desired count to 0, then back to 1 is not necessary when doing --force-new-deployment afterwards. I think you can get rid of these steps.

A bit confused reading this – is @bartek's implication that we can skip line 71?

Do we really need to wait for all of the secondary nodes to come online before re-enabling public access to the load balancer?

Seems like we could just break here, and move these lines to the bottom of the script. I think that would improve readability

What's this for? Are you basically making sure that the calling IP can get through the load balancer, even when we've otherwise disabled ingress? If that's the case, wondering if you considered hitting the primary node directly for the health check instead of going through the load balancer

Talked to Ashoat about this. Instead of copy and pasting this, we might be able to use the aws-cli to determine what the names of these services are.

Going to go back through the script and add quotations where necessary on next rebase

Will pick one kind in the next rebase

To mitigate the risk of users running terraform apply and the script incorrectly at the same time, we've decided to remove the option of the user running terraform apply and only have them use the script

We're planning to resolve the code duplication by putting all of this in its own function and then passing some kind of comparator function which will return a boolean depending on whether the http status code is valid.

The problem I was encountering not setting primary node desired count to 0 was it automatically restarting the primary node after I shut down the task. My understanding is that we need to keep this in order to run the redeployment explicitly ourselves with

aws ecs update-service --cluster $cluster_name --service $primary_service_name --force-new-deployment --desired-count 1 > /dev/null

After discussion with @ashoat, established that we can actually reenable internet traffic right after we've ensured that the primary node health check passes, triggering the start of the secondary nodes and reducing downtime.

To explain the original concern, we depend on a health check to the primary service to deploy the secondary nodes (this ensures that the migration has run).

We were worried that by having the primary node accessible on startup, the moment the primary node starts up, the health check is lost in the flood of traffic and the secondary nodes never deploy.

What I did in this current iteration is have the flood of traffic only hit the load balancer once all the secondary and primary nodes started up. This is unnecessary and we should have users able to access the primary node, just that it should happen after the health check is successful

including in next rebase

Yes, you're correct. This ensures the user is always able to access the load balancer even after we've turned off the load balancer

Taking down the load balancer and having users directly hit the primary node is definitely a possible solution. The reason I went with the ingress approach was just due to ease of implementation. Might be overkill, but I'll include a follow up task where we consider going with a direct hit to the primary node. Otherwise, I think the ingress approach is okay for now

Addressing this in https://linear.app/comm/issue/ENG-8835/resolve-risks-in-user-deploying-an-update-with-a-pending-mariadb.

For anyone reading, high level overview is:

1. Having the user always go through the script instead of ever running terraform apply themselves
2. Running terraform apply and image version control is handled by the script instead of the user

Addressing in this issue:
https://linear.app/comm/issue/ENG-8851/remove-health-check-duplication-code

That is correct. Fixing this in last rebase