Skip to content

Comments

Adding scaffold command in autoscale-settings#4

Merged
vishrutshah merged 2 commits intomonitor-clifrom
simplify-autoscale-create
Mar 3, 2017
Merged

Adding scaffold command in autoscale-settings#4
vishrutshah merged 2 commits intomonitor-clifrom
simplify-autoscale-create

Conversation

@vishrutshah
Copy link
Owner

@vishrutshah vishrutshah commented Mar 3, 2017 •
edited
Loading

Help

(env) vishrut@mac azure-cli (simplify-autoscale-create) $ az monitor autoscale-settings -h

Group
    az monitor autoscale-settings: Commands to manage autoscale settings.

Commands:
    create  : Creates or updates an autoscale setting.
    delete  : Deletes and autoscale setting.
    list    : Lists the autoscale settings for a resource group.
    scaffold: Scaffold fully formed autoscale-settings parameters structure template.
    show    : Gets an autoscale setting.
    update  : Updates an autoscale setting.

Get Scaffold json template to be filled-in:

(env) vishrut@mac azure-cli (simplify-autoscale-create) $ az monitor autoscale-settings scaffold

{
  "autoscale_setting_resource_name": "{MyAutoscaleSettings}",
  "enabled": false,
  "location": "West US",
  "notifications": [],
  "profiles": [
    {
      "capacity": {
        "default": "3",
        "maximum": "5",
        "minimum": "1"
      },
      "name": "{AutoscaleProfile}",
      "rules": [
        {
          "metric_trigger": {
            "metric_name": "{name}",
            "metric_resource_uri": "{FullyQualifiedAzureResourceID}",
            "operator": "{Equals|NotEquals|GreaterThan|GreaterThanOrEqual|LessThan|LessThanOrEquals}",
            "statistic": "{Average|Min|Max|Sum}",
            "threshold": 100,
            "time_aggregation": "{Average|Minimum|Maximum|Total|Count}",
            "time_grain": "(duration in ISO8601 format)PT5M",
            "time_window": "(duration in ISO8601 format)PT45M"
          },
          "scale_action": {
            "cooldown": "(duration in ISO8601 format)PT20M",
            "direction": "{None|Increase|Decrease}",
            "type": "{ChangeCount|PercentChangeCount|ExactCount}",
            "value": "2 (number of instances that are involved in the scaling)"
          }
        }
      ]
    }
  ],
  "tags": {},
  "target_resource_uri": "{FullyQualifiedAzureResourceID}"
}

Use json file for creating atuoscale-settings profile

(env) vishrut@mac azure-cli (simplify-autoscale-create) $ az monitor autoscale-settings create -n settings1 --parameters @/Users/vishrut/repos/azure-cli/src/command_modules/azure-cli-monitor/azure/cli/command_modules/monitor/tests/autoscale-settings.json -g scalesetrg3
{
  "autoscaleSettingResourceName": "settings1",
  "enabled": false,
  "id": "/subscriptions/xxxx-xxxx-xxxx/resourceGroups/scalesetrg3/providers/microsoft.insights/autoscalesettings/settings1",
  "location": "West US",
  "name": "settings1",
  "notifications": null,
  "profiles": [
    {
      "capacity": {
        "default": "4",
        "maximum": "5",
        "minimum": "1"
      },
      "fixedDate": null,
      "name": "Day",
      "recurrence": null,
      "rules": [
        {
          "metricTrigger": {
            "metricName": "Percentage CPU",
            "metricResourceUri": "/subscriptions/xxxx-xxxx-xxxx/resourceGroups/scalesetrg3/providers/Microsoft.Compute/virtualMachineScaleSets/vmscaleset3",
            "operator": "GreaterThanOrEqual",
            "statistic": "Average",
            "threshold": 60.0,
            "timeAggregation": "Average",
            "timeGrain": "0:05:00",
            "timeWindow": "0:45:00"
          },
          "scaleAction": {
            "cooldown": "0:20:00",
            "direction": "Increase",
            "type": "ChangeCount",
            "value": "2"
          }
        }
      ]
    }
  ],
  "resourceGroup": "scalesetrg3",
  "tags": {
    "$type": "Microsoft.WindowsAzure.Management.Common.Storage.CasePreservedDictionary, Microsoft.WindowsAzure.Management.Common.Storage"
  },
  "targetResourceUri": "/subscriptions/xxxx-xxxx-xxxx/resourceGroups/scalesetrg3/providers/Microsoft.Compute/virtualMachineScaleSets/vmscaleset3",
  "type": "Microsoft.Insights/autoscaleSettings"
}

@troydai @tjprescott @derekbekoe Please review the PR when you get a chance. Thanks!

Also please advise if the location of the scaffold command or template file doesn't seem appropriate.

tjprescott
tjprescott suggested changes Mar 3, 2017
View reviewed changes
Copy link

@tjprescott tjprescott left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Also, we'd definitely want to have (eventually) commands that would get a good default autoscale profile for the different things that can take one.

...dules/azure-cli-monitor/azure/cli/command_modules/monitor/autoscale-parameters-template.json Outdated
@@ -0,0 +1,38 @@
{
"autoscale_setting_resource_name": "autoscale setting resource name",
Copy link

@tjprescott tjprescott Mar 3, 2017

Choose a reason for hiding this comment

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

I'd recommend a name like "{MyAutoscaleSettings}"

...dules/azure-cli-monitor/azure/cli/command_modules/monitor/autoscale-parameters-template.json Outdated
"tags": {},
"profiles": [
{
"name": "name of the profile",
Copy link

@tjprescott tjprescott Mar 3, 2017

Choose a reason for hiding this comment

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

Rec: "{AutoscaleProfile}"

...dules/azure-cli-monitor/azure/cli/command_modules/monitor/autoscale-parameters-template.json Outdated
{
"metric_trigger": {
"metric_name": "name of the metric",
"metric_resource_uri": "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{providerName}/{resourceName}/{subResourceName}",
Copy link

@tjprescott tjprescott Mar 3, 2017

Choose a reason for hiding this comment

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

Rec: "{FullyQualifiedAzureResourceID}"

...dules/azure-cli-monitor/azure/cli/command_modules/monitor/autoscale-parameters-template.json Outdated
"rules": [
{
"metric_trigger": {
"metric_name": "name of the metric",
Copy link

@tjprescott tjprescott Mar 3, 2017

Choose a reason for hiding this comment

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

Rec: "{name}"

...dules/azure-cli-monitor/azure/cli/command_modules/monitor/autoscale-parameters-template.json Outdated
"metric_name": "name of the metric",
"metric_resource_uri": "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{providerName}/{resourceName}/{subResourceName}",
"time_grain": "(duration in ISO8601 format)PT5M",
"statistic": "Average or Min or Max or Sum",
Copy link

@tjprescott tjprescott Mar 3, 2017

Choose a reason for hiding this comment

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

Rec: "{Average|Mix|Max|Sum}"

...dules/azure-cli-monitor/azure/cli/command_modules/monitor/autoscale-parameters-template.json
"scale_action": {
"direction": "None or Increase or Decrease",
"type": "ChangeCount or PercentChangeCount or ExactCount",
"value": "2 (number of instances that are involved in the scaling)",
Copy link

@tjprescott tjprescott Mar 3, 2017

Choose a reason for hiding this comment

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

Is value actually a string or do you just have a string because of the comment?

Copy link
Owner Author

@vishrutshah vishrutshah Mar 3, 2017

Choose a reason for hiding this comment

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

this is defined as string in sdk so i got chance to add comment as well inside it :)

...dules/azure-cli-monitor/azure/cli/command_modules/monitor/autoscale-parameters-template.json
}
],
"notifications": [],
"enabled": false,
Copy link

@tjprescott tjprescott Mar 3, 2017

Choose a reason for hiding this comment

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

Same question: false or "false"?

Copy link
Owner Author

@vishrutshah vishrutshah Mar 3, 2017

Choose a reason for hiding this comment

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

verified: this is a type bool

src/command_modules/azure-cli-monitor/azure/cli/command_modules/monitor/custom.py Outdated
def scaffold_autoscale_settings_parameters(client): # pylint: disable=unused-argument
'''Scaffold fully formed autoscale-settings' parameters as json template
'''
return _load_autoscale_settings_prameres(AUTOSCALE_SETTINGS_PARAMETER_FILE_PATH)
Copy link

@tjprescott tjprescott Mar 3, 2017

Choose a reason for hiding this comment

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

Typo: ..._parameters

Copy link
Owner Author

@vishrutshah vishrutshah Mar 3, 2017

Choose a reason for hiding this comment

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

silly typo... third time.. same word :)

src/command_modules/azure-cli-monitor/azure/cli/command_modules/monitor/custom.py Outdated
# Autoscale settings parameter scaffold file path
CURR_DIR = os.path.dirname(os.path.realpath(__file__))
AUTOSCALE_SETTINGS_PARAMETER_FILE_PATH = os.path.join(CURR_DIR,
'autoscale-parameters-template.json')
Copy link

@tjprescott tjprescott Mar 3, 2017

Choose a reason for hiding this comment

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

Since this is only used by one command, I would move this logic into that command.

Copy link
Owner Author

@vishrutshah vishrutshah Mar 3, 2017

Choose a reason for hiding this comment

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

@tjprescott newbie question for python: If we move it inside method wouldn't it be initialized every time we call that command?

Copy link

@tjprescott tjprescott Mar 3, 2017

Choose a reason for hiding this comment

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

Yes, but the way the CLI is designed you can only call the method once per process. Moving it would ensure it ONLY gets run when you use that specific command. That's why I recommended that.

Copy link
Owner Author

@vishrutshah vishrutshah Mar 3, 2017

Choose a reason for hiding this comment

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

Perfect. Moved it inside the method :)

src/command_modules/azure-cli-monitor/azure/cli/command_modules/monitor/commands.py Outdated
with ServiceGroup(__name__, get_monitor_autoscale_settings_operation,
autoscale_settings_scaffold) as s:
with s.group('monitor autoscale-settings') as c:
c.command('scaffold', 'scaffold_autoscale_settings_parameters')
Copy link

@tjprescott tjprescott Mar 3, 2017

Choose a reason for hiding this comment

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

Command names need a verb, so perhaps something like "get-scaffold" or "get-parameters-template".

@vishrutshah
Copy link
Owner Author

vishrutshah commented Mar 3, 2017

Updated based on the review feedback. Please have a look again when you get a chance. Thanks!

tjprescott
tjprescott approved these changes Mar 3, 2017
View reviewed changes
Copy link

@tjprescott tjprescott left a comment

Choose a reason for hiding this comment

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

LGTM

@vishrutshah
Copy link
Owner Author

vishrutshah commented Mar 3, 2017

Thanks for the review travis.

@vishrutshah vishrutshah merged commit 875e53f into monitor-cli Mar 3, 2017
@vishrutshah vishrutshah deleted the simplify-autoscale-create branch March 3, 2017 23:16
vishrutshah added a commit that referenced this pull request Mar 16, 2017
@vishrutshah
Adding basic structure for monitor service
276358f 
Adding list command for all azure monitor data plane
Adding help docs for az monitor data plane commands
Adding commands for management place of Azure monitor service
Updating azure-monitor-cli to azure-mgmt-monitor and azure-monitor sdk
Using nargs to input list of metric-names
Implement filter for metrics list command
Implement filter and select in activity-logs command
Implement filter and select for tenant-activity-logs command
Adding scaffold command in autoscale-settings (#4)
Adding unittests for custom monitor commands (#5)
Rename expanded params and resolve bugs

* Fix expand parameters bug in util to support any parameters
* Adding --filters into activity-logs & tenant-activity-logs
* Adding --ids support into alert-rules & autoscale-settings' show and delete commands
* ignoring filter for alert-rules and autoscale-settings list
vishrutshah added a commit that referenced this pull request Mar 22, 2017
@vishrutshah
[Azure Monitor] Adding Azure monitor commands for management and data…
258f822 
… plane (Azure#2534)

* Adding basic structure for monitor service
* Adding list command for all azure monitor data plane
* Adding help docs for az monitor data plane commands
* Adding commands for management place of Azure monitor service
* Updating azure-monitor-cli to azure-mgmt-monitor and azure-monitor sdk
* Using nargs to input list of metric-names
* Implement filter for metrics list command
* Implement filter and select in activity-logs command
* Implement filter and select for tenant-activity-logs command
* Adding scaffold command in autoscale-settings (#4)
* Adding unittests for custom monitor commands (#5)
* Rename expanded params and resolve bugs

* Fix expand parameters bug in util to support any parameters
* Adding --filters into activity-logs & tenant-activity-logs
* Adding --ids support into alert-rules & autoscale-settings' show and delete commands
* ignoring filter for alert-rules and autoscale-settings list

* Help docs for all monitor commands to review

* rename activity-logs to activity-log

* Removing tenant-activity-logs command

* Removing event-categories commands as less useful

* Update version to 0.0.1b1+dev

* Renamed service-dignostic-settings to diagnostic-settings

* update help for review

* Update unittests for filters

* Renaming parameter_abc into parameter

* marking metric-names as required parameter for metrics list

* Removing help.json from the review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@tjprescott tjprescott tjprescott approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vishrutshah @tjprescott