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

massdriver-application/azure/scale-sets-docker #88

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

WillBeebe
Copy link
Contributor

@WillBeebe WillBeebe commented Jan 3, 2023

Azure VM runtime

Blocked by

  • managed-certificate - PR
  • endpoint - PR

TODO

  • add autoscaling
  • add monitoring

@linear
Copy link

linear bot commented Jan 3, 2023

@WillBeebe WillBeebe marked this pull request as draft January 3, 2023 21:41
@WillBeebe WillBeebe changed the title Will/orc 160 azure vm tf module massdriver-application-azure-scale-sets-docker Jan 27, 2023
@WillBeebe WillBeebe changed the title massdriver-application-azure-scale-sets-docker massdriver-application/azure/scale-sets-docker Jan 27, 2023
@WillBeebe WillBeebe marked this pull request as ready for review February 15, 2023 23:38
@WillBeebe
Copy link
Contributor Author

Marking as "Ready for review" without autoscaling or monitoring. Those should be easy adds, just trying to get a majority of this merged in.

I've provisioned it a bunch with Bloby and not-bloby. This uses the latest massdriver-application module with the non-sensitive env-vars

Comment on lines +6 to +16
data "azurerm_virtual_network" "main" {
name = local.vnet_name
resource_group_name = local.vnet_resource_group
}

data "azurerm_subnet" "default" {
# does it have to be called default?
name = "default"
virtual_network_name = data.azurerm_virtual_network.main.name
resource_group_name = data.azurerm_virtual_network.main.resource_group_name
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these are needed, because we're creating a subnet for the VMSS in subnet.tf, and using auto-cidr to configure it.

What this seems to be doing is using the default subnet coming from the VNet bundle and using it in ip_configurations block within VMSS. I think what we want is to use the subnet that gets created in this deployment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I know that file subnet.tf. We use it for the endpoint, we might be able to use it for the VM too.

for_each = var.dns.enable_dns ? [1] : []
content {
name = var.name
subnet_id = data.azurerm_subnet.default.id
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
subnet_id = data.azurerm_subnet.default.id
subnet_id = azurerm_subnet.main.id

Same thing here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the complexity here is the networking. I'd like to follow up on this with another PR if we'd like to adjust.

I designed it this way so ingress comes through the VM network. We can draw all the lines and figure out all the networking if you want.

resource_group_name = azurerm_resource_group.main.name
location = azurerm_resource_group.main.location
disable_password_authentication = false
admin_username = "placeholder"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we expose this field in postgres and mysql bundles to the customer to select their own password

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we don't want people to log into the VM, we won't expose this to the user. 👍

Copy link
Contributor

@mclacore mclacore left a comment

Choose a reason for hiding this comment

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

Looking really good so far! A few comments about some items that immediately popped out to me.

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