Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make load balancers optional for MIGs #108

Merged
merged 4 commits into from
Dec 3, 2024
Merged

Make load balancers optional for MIGs #108

merged 4 commits into from
Dec 3, 2024

Conversation

nkinkade
Copy link
Contributor

@nkinkade nkinkade commented Nov 21, 2024

Related to: m-lab/epoxy-images#277

We are experimenting with having VMs leverage the Autojoin API to get a DNS name, meaning that they will no longer need to be recorded in siteinfo. Additionally, this change obviates the need to load balance VMs, since each VM in an instance group will get its own DNS name and join the cluster independently.

This change adds a new "loadbalanced" field to the "migs" map which takes a boolean. For now, I'm just experimenting with "mlab1-chs0t", leaving the other sandbox VM loadbalanced.

All resources in the loadbalancers.tf file now use a modified for_each loops, which eliminates MIG that have loadbalanced=false.

Additionally, the change plumbs a new "loadbalanced" VM metadata attribute. The write-metadata.sh script on each machine that runs at boot will use the value of this to determine whether it needs to record the VM's external IPs or the external IPs of the load balancer.


This change is Reviewable

Updates the write-metadata.sh script to write out the IATA code to
/var/local/metadata
If M-Lab-managed VMs can use the Autojoin API to get a DNS entry, then they
don't need a load balancer. Not only do load balancers turn out to be
expensive, but they add additional complexity to the system. This commit adds
the necessary changes to make load balancers optional.
Copy link
Contributor

@robertodauria robertodauria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@nkinkade
Copy link
Contributor Author

@stephen-soltesz: FYI, in case you see something that @robertodauria and I are not. It seems to WAI in with mlab-chs01 in sandbox.

This is just to prepopulate the value explicitly, and in case Terraform would
exit with an error if we are trying to check the value of a non-existent field.
VMs definitions that don't have a probability set explicitly will get the new
default probability of 1.0.
Copy link
Contributor Author

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertodauria: After you reviewed this PR a week ago, I realized that I had forgot to add loadbalanced=(true|false) to the platform-cluster.tf files for mlab-staging and mlab-oti.

Additionally, @stephen-soltesz brought up a concern that there was a statically configured 1.0 probability for all VMs, which isn't how we run the production platform and isn't flexible enough. Since the probability feature was very much related to the intention of this PR, I went ahead and added a new feature to this PR which allows us to plumb probability from the TF configs through to the autonode-register container.

Since there are a fair number of changes since your last review, could I request that you take another look at this PR?

Reviewable status: :shipit: complete! 1 of 1 approvals obtained

Copy link
Contributor

@robertodauria robertodauria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:!

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@nkinkade nkinkade merged commit 9cfd8fc into main Dec 3, 2024
3 checks passed
@nkinkade nkinkade deleted the sandbox-kinkade branch December 3, 2024 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants