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.

Sign up for GitHub

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

Jump to bottom

Add boot diagnostics option #7

Merged
merged 17 commits into from
Oct 11, 2017
Merged

Add boot diagnostics option #7

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
50a869c
Add boot diagnostics option
Oct 3, 2017
092753e
Add vm_hostname to nic interface name for more comprehension
Oct 3, 2017
ed66005
Add vm_hostname to os_disk name
Oct 3, 2017
84600bb
Allow to delete data disk on termination
Oct 3, 2017
67a4dd3
Add storage type variable for boot diag storage account
Oct 3, 2017
29d98c7
Fixed issue where Azure/compute/azurerm module was called instead of …
Oct 3, 2017
21f40f3
PR #12 2nd example correction
dtzar Oct 3, 2017
0e6b530
PR#10 enhance nic_names
dtzar Oct 9, 2017
5203610
PR #11 enhance os_disk_name add delete_os_disk_on_termination
dtzar Oct 9, 2017
4cc1a4a
Fix issue when boot_diagnostics is set to false
Oct 10, 2017
ee7d8db
Miss to add the fix on windows boxes
Oct 10, 2017
a7fcd85
Add boot diagnostics option
Oct 3, 2017
ffae7f4
Add storage type variable for boot diag storage account
Oct 3, 2017
a265ddc
Fix issue when boot_diagnostics is set to false
Oct 10, 2017
61941e0
Miss to add the fix on windows boxes
Oct 10, 2017
9ee01ee
Merge
Oct 11, 2017
ca49841
auto generated name for bootdiag sa
Oct 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,15 @@ resource "azurerm_resource_group" "vm" {
tags = "${var.tags}"
}

resource "azurerm_storage_account" "vm-sa" {
count = "${var.boot_diagnostics == "true" ? 1 : 0}"
name = "${lower(replace(var.vm_hostname,"/[[:^alpha:]]/",""))}"
Copy link
Contributor

@dtzar dtzar Oct 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This storage account name needs to be unique per subscription, so I worry it will easily become a problem for people using the vm_hostname.

* azurerm_storage_account.vm-sa: Error creating Azure Storage Account 'myvm': storage.AccountsClient#Create: Failure responding to request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=409 Code="StorageAccountAlreadyExists" Message="The storage account named myvm already exists under the subscription."

If we use the public_ip_dns value then we'd require people use a public IP address and we don't want that. If we add another variable with a default value then people would have to change it manually and that's not good. I'll just approve this as-is. Only thing which would be nice (not sure if its possible with hcl) is to append a random number to the storage account after the hostname. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

The random value seems to be a good idea. I'll try to do something with the random module : https://www.terraform.io/docs/providers/random/index.html

Copy link
Author

Choose a reason for hiding this comment

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

I've done some tests, and we can add a random id on the storage account name.
But, the storage account name is still base on vm hostname and we may go through the character limitation (between 3 and 24). What do you think about a full random name with a prefix like bootdiagsa ?

Copy link
Contributor

@dtzar dtzar Oct 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The random provider looks like it will work perfect! You can control the byte length so exceeding the characters won't be a problem. The safest route would be to use something like bootdiag plus the random string, which I'd be happy with. Only if you wanted to be fancy (not required ;)) then you could do something like format(var.vm_hostname, arg which uses only first # of characters) + diag + random id.

Copy link
Author

Choose a reason for hiding this comment

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

I've choose the safest way :)

resource_group_name = "${azurerm_resource_group.vm.name}"
location = "${var.location}"
account_type = "${var.boot_diagnostics_sa_type}"
tags = "${var.tags}"
}

resource "azurerm_virtual_machine" "vm-linux" {
count = "${contains(list("${var.vm_os_simple}","${var.vm_os_offer}"), "WindowsServer") ? 0 : var.nb_instances}"
name = "${var.vm_hostname}${count.index}"
Expand Down Expand Up @@ -52,6 +61,10 @@ resource "azurerm_virtual_machine" "vm-linux" {
key_data = "${file("${var.ssh_key}")}"
}
}
boot_diagnostics {
enabled = "${var.boot_diagnostics}"
storage_uri = "${azurerm_storage_account.vm-sa.primary_blob_endpoint}"
}
}

resource "azurerm_virtual_machine" "vm-windows" {
Expand Down Expand Up @@ -83,6 +96,10 @@ resource "azurerm_virtual_machine" "vm-windows" {
admin_username = "${var.admin_username}"
admin_password = "${var.admin_password}"
}
boot_diagnostics {
enabled = "${var.boot_diagnostics}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, it seems this code is not working for me because when I use the default of setting boot_diagnostics to false, I still get these errors:

Error running plan: 1 error(s) occurred:

* module.mycompute.azurerm_virtual_machine.vm-windows: 2 error(s) occurred:

* module.mycompute.azurerm_virtual_machine.vm-windows[1]: Resource 'azurerm_storage_account.vm-sa' not found for variable 'azurerm_storage_account.vm-sa.primary_blob_endpoint'
* module.mycompute.azurerm_virtual_machine.vm-windows[0]: Resource 'azurerm_storage_account.vm-sa' not found for variable 'azurerm_storage_account.vm-sa.primary_blob_endpoint'

When I set it to true, it works as expected. I'm using the latest terraform v0.10.7 with Azure module 0.2.2.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your review. I fixed this issue in commit 4cc1a4a.

storage_uri = "${azurerm_storage_account.vm-sa.primary_blob_endpoint}"
}
}

resource "azurerm_availability_set" "vm" {
Expand Down Expand Up @@ -134,4 +151,4 @@ resource "azurerm_network_interface" "vm" {
private_ip_address_allocation = "Dynamic"
public_ip_address_id = "${count.index == 0 ? azurerm_public_ip.vm.id : ""}"
}
}
}
10 changes: 10 additions & 0 deletions variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,13 @@ variable "public_ip_address_allocation" {
description = "Defines how an IP address is assigned. Options are Static or Dynamic."
default = "static"
}

variable "boot_diagnostics" {
description = "(Optional) Enable or Disable boot diagnostics"
default = "false"
}

variable "boot_diagnostics_sa_type" {
description = "(Optional) Storage account type for boot diagnostics"
default = "Standard_LRS"
}