-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
New Resource: Autoscale Settings #135
Conversation
Fixes #50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @whiskeyjay
Thanks for this PR - I've taken a look and left some comments in-line - but this is off to a good start :)
Thanks!
"profile": { | ||
Type: schema.TypeList, | ||
Required: true, | ||
MinItems: 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given this is Required
- MinItems: 1
is inferred and this line isn't needed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
Schema: map[string]*schema.Schema{ | ||
"name": { | ||
Type: schema.TypeString, | ||
Required: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be ForceNew?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this "name" is more like a label for the profile, it can be changed without having to create a new profile. It has the same behavior on Azure portal.
"capacity": { | ||
Type: schema.TypeSet, | ||
Required: true, | ||
MinItems: 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(as above) this line isn't needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
Schema: map[string]*schema.Schema{ | ||
"minimum": { | ||
Type: schema.TypeInt, | ||
Required: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if memory serves there's limits to each of these, of ~1000 hosts - is that still valid? if so - should we add validation here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. The value should be in the range of [1, 100] - yes, for VMSS you can go up to 1000 (but actually limited by the allowed core numbers of the subscription), but for Autoscale Settings, somehow it is limited to 100. On portal there is a input validation and the REST API returns error if the value exceeds 100.
"maximum": { | ||
Type: schema.TypeInt, | ||
Required: true, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check added.
recurrenceSet := config["recurrence"].(*schema.Set).List() | ||
|
||
if recurrenceSet == nil || len(recurrenceSet) == 0 { | ||
return recurrence, fmt.Errorf("recurrence not defined") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not return nil here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
fixedDateSet := config["fixed_date"].(*schema.Set).List() | ||
|
||
if fixedDateSet == nil || len(fixedDateSet) == 0 { | ||
return timeWindow, fmt.Errorf("fixed_date not defined") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not return nil here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
func listTimeZoneNames() []string { | ||
return []string{ | ||
"Dateline Standard Time", "UTC-11", "Hawaiian Standard Time", "Alaskan Standard Time", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to use a regex for ex. UTC+11
here instead, rather than listing each of these out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why but unfortunately these are the only values accepted. Tried to pass "UTC+8" which is not in the list and the REST call returned error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated doc to point to the REST API doc page where all allowed values can be found.
|
||
`email` supports the following: | ||
|
||
* `send_to_subscription_administrator` - (Required) Specifies whether to send email notifications to the subscription administrator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be defaulted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default behavior on portal is not sending emails, and since email property is optional, I think it is fine not to set a default value in the schema - if users do need email notifications, they specify email properties.
|
||
* `send_to_subscription_administrator` - (Required) Specifies whether to send email notifications to the subscription administrator. | ||
|
||
* `send_to_subscription_co_administrator` - (Required) Specifies whether to send email notifications to the subscription co-administrator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be defaulted?
page_title: "Azure Resource Manager: azurerm_autoscale_setting" | ||
sidebar_current: "docs-azurerm-resource-autoscale-setting" | ||
description: |- | ||
Create an autoscale setting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest: Create an autoscale setting that can be applied to an Azure Virtual Machine Scale Set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this also cover other resources like Web Apps etc - or is it specific to VMSS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Virtual Machine Scale Sets, Cloud Services, and App Service - Web Apps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 could we update this to mention those, similar to Stephen's suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left suggestions
page_title: "Azure Resource Manager: azurerm_autoscale_setting" | ||
sidebar_current: "docs-azurerm-resource-autoscale-setting" | ||
description: |- | ||
Create an autoscale setting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a pointer from VMSS docs to here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a link to Autoscale Settings doc.
## Example Usage | ||
|
||
```hcl | ||
variable "time_zone" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, removed.
enabled = true | ||
resource_group_name = "${azurerm_resource_group.test.name}" | ||
location = "${azurerm_resource_group.test.location}" | ||
target_resource_uri = "${azurerm_virtual_machine_scale_set.test.id}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URI or ID? Seems like target_resource_id would be better. I think that consistency in the rest of azurerm terraform provider and in how it's used trumps consistency with the REST API in this case. That's my opinion at any rate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ARM template also uses "uri". Although Azure PowerShell call it "id". I'll just keep it aligned with ARM template. As long as the doc tells what value to specify it should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Terraform we tend to use id
rather than uri
/url
- so I'd suggest it'd be better to change this to match the convention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to id.
rule { | ||
"metric_trigger" { | ||
"metric_name" = "Percentage CPU" | ||
"metric_resource_uri" = "${azurerm_virtual_machine_scale_set.test.id}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
the number of cores that are available in your subscription. | ||
|
||
* `default` - (Required) Specifies the number of instances that are available for scaling if metrics are not available for | ||
evaluation. The default is only used if the current instance cout is lower than the default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
count
azurerm/config.go
Outdated
@@ -13,6 +13,7 @@ import ( | |||
"github.com/Azure/azure-sdk-for-go/arm/containerservice" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any mistake whatsoever in the values sent to Azure causes 400 unknown with no info on where/what the problem is. Can you check with fiddler to see if that's really all we're getting, and report an error to Azure (if it is) or go sdk (if sdk is not giving us the error info).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same on portal, when something goes wrong with autoscale settings, it just says "bad request". :(
"days": { | ||
Type: schema.TypeList, | ||
Required: true, | ||
Elem: &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringInSlice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List doesn't support this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add a custom validation method for this? does this also need to be case insensitive for the values in Azure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing values like "sunday" will not result in any REST API errors, however, the web portal UI will not show "Sunday" as checked (sigh). And by custom validation method do you mean custom logic when reading the values from configuration? Because the adding the ValidateFunc here will result in runtime error: ValidateFunc is not yet supported on lists or sets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
however, the web portal UI will not show "Sunday" as checked (sigh)
It's minor, but I'd suggest it might be worth filing a bug with the Portal team about that - in the meantime we should be able to cast this value to Title Case internally before calling CreateOrUpdate
, which should solve the immediate issue? I've not tested it - but I'm assuming a DiffSuppressFunc
to ignore the case will work here?
* `hours` - (Required) Specifies a collection of hours that the profile takes effect on. Values supported are 0 to 23 on the 24-hour | ||
clock (AM/PM times are not supported). | ||
|
||
* `minutes` - (Required) Specifies a collection of minutes at which the profile takes effect at. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at which the profile takes effect.
|
||
* `time_zone` - (Required) Specifies the time zone of the start and end times for the profile. | ||
|
||
* `start` - (Required) Specifies the start time for the profile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format for start/end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
}, | ||
}, | ||
}, | ||
Set: resourceAzureRmAutoscaleDefaultHash, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tombuildsstuff Tom, can you comment on this? My understanding is that you should just be able to not specify a set function, and in fact that that is recommend, and a default will be used for you.
I know Jay said he had troubles getting that to work.
Made a bunch of changes to address the comments. Also re-ran the acc tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @whiskeyjay
Sorry for the delay in re-reviewing this!
Thanks for the continued effort here - I've taken another look and this is looking really good - I've commented a few minor issues, and if we can fix them up this should be good to merge :)
Thanks!
Type: schema.TypeString, | ||
Required: true, | ||
// ValidateFunc: validation.StringInSlice(listTimeZoneNames(), false), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add this back in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
"days": { | ||
Type: schema.TypeList, | ||
Required: true, | ||
Elem: &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add a custom validation method for this? does this also need to be case insensitive for the values in Azure?
|
||
parameters := insights.AutoscaleSettingResource{ | ||
Name: &name, | ||
Type: &resourceType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from memory, we shouldn't need to specify this (in every other API it's pre-filled by Azure)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I removed it.
Type: schema.TypeBool, | ||
Required: true, | ||
}, | ||
"target_resource_uri": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we make this target_resource_id
to match the convention within Terraform?
d.SetId("") | ||
return nil | ||
} | ||
return fmt.Errorf("Error making Read request on Autoscale Setting %s: %s", name, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the second argument here needs to be %+v
as it's an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Fixed.
} | ||
|
||
resource "azurerm_autoscale_setting" "test" { | ||
name = "myAutoscaleSetting" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 could we give this a unique name?
} | ||
|
||
resource "azurerm_autoscale_setting" "test" { | ||
name = "myAutoscaleSetting" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 could we give this a unique name?
website/azurerm.erb
Outdated
@@ -174,6 +174,15 @@ | |||
</ul> | |||
</li> | |||
|
|||
<li<%= sidebar_current("docs-azurerm-resource-autoscale-setting") %>> | |||
<a href="#">Monitoring And Diagnostics Resources</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this Monitoring Resources
for now, until some Diagnostic specific resources are supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
page_title: "Azure Resource Manager: azurerm_autoscale_setting" | ||
sidebar_current: "docs-azurerm-resource-autoscale-setting" | ||
description: |- | ||
Create an autoscale setting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 could we update this to mention those, similar to Stephen's suggestion?
"email" { | ||
"send_to_subscription_administrator" = true | ||
"send_to_subscription_co_administrator" = true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't need the quote's around the keys? (also goes for the tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quotes removed.
@tombuildsstuff I have addressed most of the new comments. Had a few questions though, could you take a look and clarify? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @whiskeyjay
Thanks for the continued effort on this - apologies for the delay in reviewing this, I'd reviewed but not hit submit 🤦♂️
I've taken another look through and commented on a few minor things - however this is nearly there! The main remaining question for me is around this line which I think might want an update to the Schema so the user doesn't have to input escaped JSON?
Thanks!
location = "${azurerm_resource_group.test.location}" | ||
target_resource_id = "${azurerm_virtual_machine_scale_set.test.id}" | ||
profile { | ||
name = "{\"name\":\"defaultProfile\",\"for\":\"Weekends\"}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this valid? If so - should we be sending this field as {name="foo"}
in all requests / should we be updating the schema to match?
|
||
"regexp" | ||
|
||
"strings" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really minor could we run goimports -w ./azurerm/
to tidy this up?
Type: schema.TypeSet, | ||
Optional: true, | ||
MaxItems: 1, | ||
ConflictsWith: []string{"profile.recurrence"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this work? I have a feeling this syntax isn't supported?
"days": { | ||
Type: schema.TypeList, | ||
Required: true, | ||
Elem: &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
however, the web portal UI will not show "Sunday" as checked (sigh)
It's minor, but I'd suggest it might be worth filing a bug with the Portal team about that - in the meantime we should be able to cast this value to Title Case internally before calling CreateOrUpdate
, which should solve the immediate issue? I've not tested it - but I'm assuming a DiffSuppressFunc
to ignore the case will work here?
}, | ||
"enabled": { | ||
Type: schema.TypeBool, | ||
Required: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 is it worth making this Optional and Defaulting it to true
?
name := id.Path["autoscalesettings"] | ||
|
||
if name == "" { | ||
return fmt.Errorf("Cannot find resource name in Resource ID for Autoscale Setting") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue it'd be worth returning the ID at the end of this error message to make it easier to diagnose?
capacity := make(map[string]interface{}) | ||
capacity["minimum"], _ = strconv.Atoi(*profile.Capacity.Minimum) | ||
capacity["maximum"], _ = strconv.Atoi(*profile.Capacity.Maximum) | ||
capacity["default"], _ = strconv.Atoi(*profile.Capacity.Default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening that, this isn't a blocker by any means - but worth getting fixed for a future release if it's a bug :)
return fmt.Errorf("Bad: Autoscale Setting %q (resource group: %q) does not exist", autoscaleSettingName, resourceGroup) | ||
} | ||
|
||
return fmt.Errorf("Bad: Get on autoscaleSettingsClient: %s", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this formatting argument %+v
?
return fmt.Sprintf(` | ||
variable "time_zone" { | ||
type = "string" | ||
default = "Pacific Standard Time" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reflecting on this, maybe not, I was wondering if we could get away with the user inputting UTC-8
rather than Pacific Standard Time
- but that'd actually just complicate things
page_title: "Azure Resource Manager: azurerm_autoscale_setting" | ||
sidebar_current: "docs-azurerm-resource-autoscale-setting" | ||
description: |- | ||
Create an autoscale setting that can be applied to Virtual Machine Scale Sets, Cloud Services and App Service - Web Apps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be wrong, but aren't Cloud Services part of ASM? In which case - do they belong in this list?
minor: can we match the convention seen in the CosmosDB Resource by updating App Service - Web Apps
to App Service (formally "Web Apps")
Hi Folks, any idea if / when this feature will get progressed? This is something we could really use! Cheers |
Hi! Thanks for working on this feature! Any chance you have time to finish it @whiskeyjay? |
@tombuildsstuff Tom, are you guys going to take this PR over since @whiskeyjay and I are no longer working on terraform? |
@StephenWeatherford sure. Given that @whiskeyjay is no longer working on this PR I'm going to close this PR for the moment (but leave the branch around so it can be picked up) - and I'll pick this up in the near future. |
@tombuildsstuff, is this still in progress? If not, I'm interested in picking it up and working on it. Can you point me to the latest branch and remaining issues? |
@saada it's on our roadmap but we've not got to it yet - but if you're able to pick it up that'd be awesome :) Given a lot's happened since this branch was started, I'd probably suggest pulling the resources out (linked below) into a new branch off master and fixing the build: The most important thing to note since this PR was first created is that the Azure SDK now has switched to exposing multiple API versions - rather than just pinning to one explicitly. As such you'll need to switch any imports currently using I'd suggest the most prudent thing to do regarding reviewing would be to first get this compiling and then to open a PR from which we can re-review; the comments above should be valid - but given a lot of time has passed it's probably simplest to re-review it Hope that helps :) |
Does this features almost done ? I need that for one customer, how may I help ? |
Locking this PR in favour of #1140 - since this is where this is being tracked. |
This PR adds a new resource: azurerm_autoscale_setting. It enables auto-scaling for VM Scale Sets and Compute Availability Sets.
Acceptance test results: