new resource - `azurerm_machine_learning_compute_cluster` #11675

gro1m · 2021-05-12T10:09:57Z

Basic implementation for a Machine Learning Compute Cluster (without possibility to ssh into compute cluster). Unfortunately there are still some problems:

--- TEST SUMMARY ---

TF_ACC=1 go test -v ./azurerm/internal/services/machinelearning -run=TestAccComputeCluster -timeout 60m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/05/12 10:29:26 [DEBUG] not using binary driver name, it's no longer needed
2021/05/12 10:29:26 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccComputeCluster_basic
=== PAUSE TestAccComputeCluster_basic
=== RUN   TestAccComputeCluster_requiresImport
=== PAUSE TestAccComputeCluster_requiresImport
=== RUN   TestAccComputeCluster_basicUpdate
=== PAUSE TestAccComputeCluster_basicUpdate
=== CONT  TestAccComputeCluster_basic
=== CONT  TestAccComputeCluster_basicUpdate
=== CONT  TestAccComputeCluster_requiresImport

<…>

    testing.go:620: Step 2/2, expected an error with pattern, no match on: Error running apply: exit status 1
        
        Error: ID contained more segments than required: "/subscriptions/b609167b-ec1b-406c-accc-dc6cb5df9f43/resourceGroups/acctestRG-ml-210512102926964607/providers/Microsoft.MachineLearningServices/workspaces/acctest-MLW2105121029269646/computes/CC-21051207", map[computes:CC-21051207]
        
          on terraform_plugin_test.tf line 113, in resource "azurerm_machine_learning_compute_cluster" "import":
         113: resource "azurerm_machine_learning_compute_cluster" "import" {
        
        
=== CONT  TestAccComputeCluster_basicUpdate
    testing.go:620: Step 3/4 error: After applying this test step, the plan was not empty.
        stdout:
        
        
        An execution plan has been generated and is shown below.
        Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # azurerm_machine_learning_compute_cluster.test will be updated in-place
          ~ resource "azurerm_machine_learning_compute_cluster" "test" {
                id                            = "/subscriptions/b609167b-ec1b-406c-accc-dc6cb5df9f43/resourceGroups/acctestRG-ml-210512102926969345/providers/Microsoft.MachineLearningServices/workspaces/acctest-MLW2105121029269693/computes/CC-21051245"
                name                          = "CC-21051245"
                # (5 unchanged attributes hidden)
        
        
              ~ scale_settings {
                  ~ max_node_count                   = 1 -> 2
                  ~ min_node_count                   = 0 -> 1
                  ~ node_idle_time_before_scale_down = "PT30S" -> "PT60S"
                }
                # (1 unchanged block hidden)
            }
        
        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccComputeCluster_requiresImport (865.39s)
--- FAIL: TestAccComputeCluster_basicUpdate (876.13s)
--- PASS: TestAccComputeCluster_basic (976.30s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/machinelearning     981.063s
FAIL
make: *** [acctests] Error 1

azurerm/internal/services/machinelearning/machine_learning_compute_cluster_resource.go

gro1m · 2021-05-15T18:53:07Z

@aristosvo If you have time would be happy if you could review this as well. I tried to fix the requiresImport error, but I did not find what the issue is. I removed the update for now, as this is impossible to achieve with the current API. For the requiresImport I still get this error:

Error: ID contained more segments than required: "/subscriptions/b609167b-ec1b-406c-accc-dc6cb5df9f43/resourceGroups/acctestRG-ml-210512102926964607/providers/Microsoft.MachineLearningServices/workspaces/acctest-MLW2105121029269646/computes/CC-21051207", map[computes:CC-21051207]

It seems like the computes/CC-21051207 part is unexpected, but why. I thought that I am using the correct parsers and I did not find any erroneous ones. But maybe you can find something?!

aristosvo

To solve your initial problem we can fix the test, haven't looked at the whole resource in detail

azurerm/internal/services/machinelearning/machine_learning_compute_cluster_resource_test.go

gro1m · 2021-05-16T10:59:44Z

TEST SUMMARY:

TF_ACC=1 go test -v ./azurerm/internal/services/machinelearning -run=TestAccComputeCluster -timeout 60m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/05/16 10:18:46 [DEBUG] not using binary driver name, it's no longer needed
2021/05/16 10:18:46 [DEBUG] not using binary driver name, it's no longer needed
=== RUN TestAccComputeCluster_basic
=== PAUSE TestAccComputeCluster_basic
=== RUN TestAccComputeCluster_requiresImport
=== PAUSE TestAccComputeCluster_requiresImport
=== CONT TestAccComputeCluster_basic
=== CONT TestAccComputeCluster_requiresImport
--- PASS: TestAccComputeCluster_basic (560.54s)
--- PASS: TestAccComputeCluster_requiresImport (568.04s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/machinelearning 572.932s

azurerm/internal/services/machinelearning/machine_learning_compute_cluster_resource.go

gro1m · 2021-05-18T09:00:47Z

As there have been some small changes, I tested again and everything still looks good:

TF_ACC=1 go test -v ./azurerm/internal/services/machinelearning -run=TestAccComputeCluster -timeout 60m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/05/18 10:18:42 [DEBUG] not using binary driver name, it's no longer needed
2021/05/18 10:18:42 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccComputeCluster_basic
=== PAUSE TestAccComputeCluster_basic
=== RUN   TestAccComputeCluster_requiresImport
=== PAUSE TestAccComputeCluster_requiresImport
=== CONT  TestAccComputeCluster_basic
=== CONT  TestAccComputeCluster_requiresImport
--- PASS: TestAccComputeCluster_basic (461.23s)
--- PASS: TestAccComputeCluster_requiresImport (472.31s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/machinelearning     476.998s

favoretti · 2021-05-18T09:12:03Z

My 2ct on the matter - since the cluster is ephemeral by design, we could potentially release these without ability to update the scale settings and basically recreate it every time, it's not a super time-consuming operation. While we're working with Service Team on fixing the swagger - people (we) could use this already and implement scaling updates once SDK has a fix?

gro1m · 2021-05-20T10:00:54Z

@katbyte Should look good as well now:)

TF_ACC=1 go test -v ./azurerm/internal/services/machinelearning -run=TestAccComputeCluster -timeout 60m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/05/20 11:10:17 [DEBUG] not using binary driver name, it's no longer needed
2021/05/20 11:10:17 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccComputeCluster_basic
=== PAUSE TestAccComputeCluster_basic
=== RUN   TestAccComputeCluster_requiresImport
=== PAUSE TestAccComputeCluster_requiresImport
=== CONT  TestAccComputeCluster_basic
=== CONT  TestAccComputeCluster_requiresImport
--- PASS: TestAccComputeCluster_basic (764.75s)
--- PASS: TestAccComputeCluster_requiresImport (785.94s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/machinelearning     790.986s

…rm_machine_learning_workspace` (hashicorp#11739)

favoretti · 2021-05-26T12:13:03Z

gro1m · 2021-06-10T09:07:26Z

@katbyte @tombuildsstuff @aristosvo @favoretti @ArcturusZhang Microsoft informed us that they expect we could start implementing the update operation in August (fix in July, communication in August). The workaround for the time being to get unblocked would be to manually update the auto generated GO SDK.

katbyte

THanks @caius - just have a couple comments on schema bit otherwise this is looking good

website/docs/r/machine_learning_compute_cluster.html.markdown

katbyte · 2021-05-16T20:55:43Z

website/docs/r/machine_learning_compute_cluster.html.markdown

+
+* `scale_settings` - (Required) A `scale_settings` block as defined below. Changing this forces a new Machine Learning Compute Cluster to be created.
+
+* `vm_priority` - (Required) The priority of the VM. Changing this forces a new Machine Learning Compute Cluster to be created.


what values are possible here? and can we validate against them?

Options are "LowPriority" or "Dedicated", so possible we could validate that :)

addressed in new commit

katbyte · 2021-05-16T20:56:46Z

website/docs/r/machine_learning_compute_cluster.html.markdown

+## Arguments Reference
+
+The following arguments are supported:
+


could we put name. location, worspace id first?

addressed in new commit.

katbyte · 2021-06-10T18:36:21Z

website/docs/r/machine_learning_compute_cluster.html.markdown

+
+* `min_node_count` - (Required) Minimal node count. Changing this forces a new Machine Learning Compute Cluster to be created.
+
+* `node_idle_time_before_scale_down` - (Required) Node Idle Time Before Scale Down: defines the time until the compute is shutdown when it has gone into Idle state. Is defined according to W3C XML schema standard for duration. Changing this forces a new Machine Learning Compute Cluster to be created.


would this make more sense as

Suggested change

* `node_idle_time_before_scale_down` - (Required) Node Idle Time Before Scale Down: defines the time until the compute is shutdown when it has gone into Idle state. Is defined according to W3C XML schema standard for duration. Changing this forces a new Machine Learning Compute Cluster to be created.

* `scale_down_nodes_after_idle_duration` - (Required) Node Idle Time Before Scale Down: defines the time until the compute is shutdown when it has gone into Idle state. Is defined according to W3C XML schema standard for duration. Changing this forces a new Machine Learning Compute Cluster to be created.

?

That is the azure cli term used here: https://docs.azure.cn/zh-cn/cli/ext/azure-cli-ml/ml/computetarget/create?view=azure-cli-latest#ext_azure_cli_ml_az_ml_computetarget_create_amlcompute. But I agree that your proposal is more descriptive, so up to you.

lets use this more description one, we tend to change names to better match the terraform schema/provider the best UX

addressed in newest commit

gro1m · 2021-06-10T21:07:19Z

@katbyte Everything should be addressed now, could you test please, because I won't push for some hours now? I did not change the naming of vm_size and node_idle_time_after_shutdown (see reasons above). If you still think it is better to change it would be nice if you could that.

katbyte · 2021-06-10T21:59:42Z

address your comments inline above, just node_idle_time_after_shutdown to address now

…_nodes_after_idle_duration

gro1m · 2021-06-11T07:04:41Z

@katbyte I addressed your change and the tests should pass:

TEST SUMMARY:

=== RUN   TestAccComputeCluster_basic
=== PAUSE TestAccComputeCluster_basic
=== RUN   TestAccComputeCluster_requiresImport
=== PAUSE TestAccComputeCluster_requiresImport
=== CONT  TestAccComputeCluster_basic
=== CONT  TestAccComputeCluster_requiresImport
--- PASS: TestAccComputeCluster_basic (515.41s)
--- PASS: TestAccComputeCluster_requiresImport (529.17s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/machinelearning     540.560s

katbyte

Thanks @gro1m - LGTM 👍

github-actions · 2021-06-18T06:43:41Z

This functionality has been released in v2.64.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

github-actions · 2021-07-19T02:07:47Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

ghost added size/XXL documentation labels May 12, 2021

favoretti reviewed May 12, 2021

View reviewed changes

This was referenced May 13, 2021

Upgrade Machinelearningservices to 2021-04-01 Azure/azure-sdk-for-go#14666

Closed

Compute Cluster operation does not work in Machinelearningservices version 2020-04-01 Azure/azure-sdk-for-go#14669

Closed

ghost added size/XL and removed size/XXL labels May 15, 2021

aristosvo suggested changes May 15, 2021

View reviewed changes

azurerm/internal/services/machinelearning/machine_learning_compute_cluster_resource_test.go Outdated Show resolved Hide resolved

ghost added size/XXL and removed size/XL labels May 16, 2021

gro1m changed the title ~~[WIP] new resource - azurerm_machine_learning_compute_cluster~~ new resource - azurerm_machine_learning_compute_cluster May 16, 2021

ArcturusZhang reviewed May 18, 2021

View reviewed changes

azurerm/internal/services/machinelearning/machine_learning_compute_cluster_resource.go Outdated Show resolved Hide resolved

gro1m force-pushed the r/azurerm_machine_learning_compute_cluster branch from 44236cd to 178d4ca Compare May 18, 2021 07:07

favoretti force-pushed the r/azurerm_machine_learning_compute_cluster branch 2 times, most recently from 245103e to 4a80be0 Compare May 26, 2021 11:30

gro1m added 10 commits May 26, 2021 13:53

Add basic compute cluster resource

abc8a12

Address review#1

871128d

Remove Update

48cd1c7

Fix requiresImport test

b793366

update docs

26db4c4

update read of ml workspace id

8ee8682

chore - use auto-generated parsers and validators for resource `azure…

9786f12

…rm_machine_learning_workspace` (hashicorp#11739)

fix tflint errors

162178f

fix golint: unnecessary conversion for node_idle_time_before_scale_down

9d69494

Address review#2: remove compute type

1f02fa0

favoretti force-pushed the r/azurerm_machine_learning_compute_cluster branch from 4a80be0 to 51d5ca1 Compare May 26, 2021 11:53

Remove extra generators

ce605a4

favoretti added new-resource service/machine-learning labels May 26, 2021

favoretti added this to the v2.61.0 milestone May 26, 2021

tombuildsstuff modified the milestones: v2.61.0, v2.62.0 May 27, 2021

Merge branch 'master' into r/azurerm_machine_learning_compute_cluster

6582fb3

katbyte modified the milestones: v2.62.0, v2.63.0 Jun 4, 2021

katbyte requested changes Jun 10, 2021

View reviewed changes

katbyte added the waiting-response label Jun 10, 2021

gro1m added 2 commits June 10, 2021 22:38

address review

3c3a287

make fmt

2c823ce

katbyte modified the milestones: v2.63.0, v2.64.0 Jun 11, 2021

address review: rename node_idle_time_before_scale_down to scale_down…

bf83889

…_nodes_after_idle_duration

terrafmt fmt -f on test file

bcc528b

katbyte approved these changes Jun 11, 2021

View reviewed changes

katbyte merged commit 353fedd into hashicorp:master Jun 11, 2021

katbyte added a commit that referenced this pull request Jun 11, 2021

CHANGELOG.md for #11675

0d30999

gro1m deleted the r/azurerm_machine_learning_compute_cluster branch June 11, 2021 20:51

github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new resource - `azurerm_machine_learning_compute_cluster` #11675

new resource - `azurerm_machine_learning_compute_cluster` #11675

gro1m commented May 12, 2021 •

edited

Loading

gro1m commented May 15, 2021

aristosvo left a comment

gro1m commented May 16, 2021

gro1m commented May 18, 2021

favoretti commented May 18, 2021

gro1m commented May 20, 2021

favoretti commented May 26, 2021

gro1m commented Jun 10, 2021

katbyte left a comment

katbyte May 16, 2021

gro1m Jun 10, 2021

gro1m Jun 10, 2021

katbyte May 16, 2021

gro1m Jun 10, 2021

katbyte Jun 10, 2021

gro1m Jun 10, 2021

katbyte Jun 10, 2021

gro1m Jun 11, 2021

gro1m commented Jun 10, 2021 •

edited

Loading

katbyte commented Jun 10, 2021

gro1m commented Jun 11, 2021 •

edited

Loading

katbyte left a comment

github-actions bot commented Jun 18, 2021

github-actions bot commented Jul 19, 2021


		* `scale_settings` - (Required) A `scale_settings` block as defined below. Changing this forces a new Machine Learning Compute Cluster to be created.

		* `vm_priority` - (Required) The priority of the VM. Changing this forces a new Machine Learning Compute Cluster to be created.

		## Arguments Reference

		The following arguments are supported:


		* `min_node_count` - (Required) Minimal node count. Changing this forces a new Machine Learning Compute Cluster to be created.

		* `node_idle_time_before_scale_down` - (Required) Node Idle Time Before Scale Down: defines the time until the compute is shutdown when it has gone into Idle state. Is defined according to W3C XML schema standard for duration. Changing this forces a new Machine Learning Compute Cluster to be created.

	* `node_idle_time_before_scale_down` - (Required) Node Idle Time Before Scale Down: defines the time until the compute is shutdown when it has gone into Idle state. Is defined according to W3C XML schema standard for duration. Changing this forces a new Machine Learning Compute Cluster to be created.
	* `scale_down_nodes_after_idle_duration` - (Required) Node Idle Time Before Scale Down: defines the time until the compute is shutdown when it has gone into Idle state. Is defined according to W3C XML schema standard for duration. Changing this forces a new Machine Learning Compute Cluster to be created.

new resource - azurerm_machine_learning_compute_cluster #11675

new resource - azurerm_machine_learning_compute_cluster #11675

Conversation

gro1m commented May 12, 2021 • edited Loading

gro1m commented May 15, 2021

aristosvo left a comment

Choose a reason for hiding this comment

gro1m commented May 16, 2021

gro1m commented May 18, 2021

favoretti commented May 18, 2021

gro1m commented May 20, 2021

favoretti commented May 26, 2021

gro1m commented Jun 10, 2021

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gro1m commented Jun 10, 2021 • edited Loading

katbyte commented Jun 10, 2021

gro1m commented Jun 11, 2021 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 18, 2021

github-actions bot commented Jul 19, 2021

new resource - `azurerm_machine_learning_compute_cluster` #11675

new resource - `azurerm_machine_learning_compute_cluster` #11675

gro1m commented May 12, 2021 •

edited

Loading

gro1m commented Jun 10, 2021 •

edited

Loading

gro1m commented Jun 11, 2021 •

edited

Loading