new resource `azurerm_machine_learning_inference_cluster` #11550

gro1m · 2021-05-01T21:21:37Z

Test results:
PASS: TestAccInferenceCluster_requiresImport (858.19s)
--- PASS: TestAccInferenceCluster_custom_ssl_complete (899.78s)
--- PASS: TestAccInferenceCluster_basic (905.25s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/machinelearning 910.925s

Notes for the reviewer:

Update did not work -> Tests commented out, but functionality left in. I do not know where the mistake lies, but I guess I cannot let the update in, when the tests fail. Hoping the reviewer can suggest a fix for the tests.
Microsoft SSL Certificates did not work, even after passing cert and key as was requested by the error message -> not included.
As the Inference Cluster attaches to a Kubernetes Service of Type machinelearningservices.AKS unfortunately not all properties could be retrieved (via containerservice.AKS), namely "description", "cluster purpose" and the (custom) SSL Configuration.

katbyte

Thanks @gro1m - i've given this a quick review and it's off the a great start!

azurerm/internal/services/machinelearning/machine_learning_inference_cluster_resource.go

azurerm/internal/services/machinelearning/machine_learning_inference_cluster_resource_test.go

azurerm/internal/services/machinelearning/ssl_config/key.pem

katbyte · 2021-05-03T21:04:48Z

azurerm/internal/services/machinelearning/machine_learning_inference_cluster_resource.go

+				Default:  false,
+			},
+
+			"ssl_certificate_custom": {


i think we can remove custom as its implied?

Suggested change

"ssl_certificate_custom": {

"ssl_certificate": {

I was thinking about doing a ssl_certificate_custom and ssl_certificate_microsoft block, but the Microsoft SSL configuration does not seem to work (just specifying leaf_domain_label still requests cert and key files). In the second commit I just go for ssl here and I removed the ssl_enabled parameter as I think this can be implied if you specify non-empty certificates and keys.

ssl_certificate/ssl_certificate_microsoft would work

Does that mean you are not happy with:

ssl { cert = file("testdata/cert.pem") key = file("testdata/key.pem") cname = "www.contoso.com" }

In principle the schema could be - correct me if I am wrong - improved in future (if the Microsoft bug does not occur - I think it is a bug) as follows:

"ssl": { Type: schema.TypeSet, Optional: true, ForceNew: true, MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "cert": { Type: schema.TypeString, Optional: true, Default: "", ConflictsWith: {"leaf_domain_label", "overwrite_existing_domain"}, }, "key": { Type: schema.TypeString, Optional: true, Default: "", ConflictsWith: {"leaf_domain_label", "overwrite_existing_domain"}, }, "cname": { Type: schema.TypeString, Optional: true, Default: "", ConflictsWith: {"leaf_domain_label", "overwrite_existing_domain"}, }, "leaf_domain_label": { Type: schema.TypeString, Optional: true, Default: "", ConflictsWith: {"cert", "key", "cname"}, }, "overwrite_existing_domain": { Type: schema.TypeString, Optional: true, Default: "", ConflictsWith: {"cert", "key", "cname"}, }, }, }, },

What do you think is best for possible future enhancements: 2 separate blocks with now only an "ssl_certificate" block or everything in the "ssl" block or if you want "ssl_certificate" block?

azurerm/internal/services/machinelearning/machine_learning_inference_cluster_resource.go

gro1m · 2021-05-05T10:07:12Z

I included the update now, but I expect it to fail:

TF_ACC=1 go test -v ./azurerm/internal/services/machinelearning -run=TestAccInferenceCluster -timeout 60m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/05/05 08:24:20 [DEBUG] not using binary driver name, it's no longer needed
2021/05/05 08:24:20 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccInferenceCluster_basic
=== PAUSE TestAccInferenceCluster_basic
=== RUN   TestAccInferenceCluster_requiresImport
=== PAUSE TestAccInferenceCluster_requiresImport
=== RUN   TestAccInferenceCluster_complete
=== PAUSE TestAccInferenceCluster_complete
=== RUN   TestAccInferenceCluster_basicUpdate
=== PAUSE TestAccInferenceCluster_basicUpdate
=== RUN   TestAccInferenceCluster_completeUpdate
=== PAUSE TestAccInferenceCluster_completeUpdate
=== CONT  TestAccInferenceCluster_basicUpdate
=== CONT  TestAccInferenceCluster_completeUpdate
=== CONT  TestAccInferenceCluster_complete
=== CONT  TestAccInferenceCluster_requiresImport
=== CONT  TestAccInferenceCluster_basic
Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420799178" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420797524" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420791885" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420797028" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420799508" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420799178" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420797524" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420791885" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420797028" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420799508" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420799178" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420799508" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420797028" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420791885" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420797524" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420799178" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420799178" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420797028" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420797028" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420791885" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420791885" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420797524" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420797524" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420799508" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420799178" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420797524" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420791885" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420797028" "Microsoft.MachineLearningServices" map[]}=== CONT  TestAccInferenceCluster_basicUpdate
    testing.go:620: Step 3/4 error: Error running apply: 
        Error: A resource with the ID "/subscriptions/b609167b-ec1b-406c-accc-dc6cb5df9f43/resourceGroups/acctestRG-ml-210505082420799178/providers/Microsoft.MachineLearningServices/workspaces/acctest-MLW2105050824207991/computes/AIC-21050578" already exists - to be managed via Terraform this resource needs to be imported into the State. Please see the resource documentation for "azurerm_machine_learning_inference_cluster" for more information.
        
          on ../../../../../../../../var/folders/t2/y0zhp0x97q5bk_s_sjq4b3zh0000gn/T/tftest210209280/work723527519/config879850977/terraform_plugin_test.tf line 111, in resource "azurerm_machine_learning_inference_cluster" "test":
         111: resource "azurerm_machine_learning_inference_cluster" "test" {
        
        
Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420799508" "Microsoft.MachineLearningServices" map[]}=== CONT  TestAccInferenceCluster_completeUpdate
    testing.go:620: Step 3/4 error: Error running apply: 
        Error: A resource with the ID "/subscriptions/b609167b-ec1b-406c-accc-dc6cb5df9f43/resourceGroups/acctestRG-ml-210505082420797028/providers/Microsoft.MachineLearningServices/workspaces/acctest-MLW2105050824207970/computes/AIC-21050528" already exists - to be managed via Terraform this resource needs to be imported into the State. Please see the resource documentation for "azurerm_machine_learning_inference_cluster" for more information.
        
          on ../../../../../../../../var/folders/t2/y0zhp0x97q5bk_s_sjq4b3zh0000gn/T/tftest210209280/work965243913/config501865756/terraform_plugin_test.tf line 111, in resource "azurerm_machine_learning_inference_cluster" "test":
         111: resource "azurerm_machine_learning_inference_cluster" "test" {
        
        
Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420799178" "Microsoft.MachineLearningServices" map[]}Debug: id = &{"b609167b-ec1b-406c-accc-dc6cb5df9f43" "acctestRG-ml-210505082420797028" "Microsoft.MachineLearningServices" map[]}--- PASS: TestAccInferenceCluster_requiresImport (947.45s)
--- PASS: TestAccInferenceCluster_basic (1009.88s)
--- PASS: TestAccInferenceCluster_complete (1015.03s)
--- FAIL: TestAccInferenceCluster_basicUpdate (1034.19s)
--- FAIL: TestAccInferenceCluster_completeUpdate (1038.56s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/machinelearning     1043.327s
FAIL
make: *** [acctests] Error 1

I have no clue what I am doing wrong, so if someone can help me with this I would be very happy

gro1m · 2021-05-05T14:50:57Z

@katbyte I thought the git diff was incorrect (i.e. all lines changed), but I was looking at commit 1 instead of commit 2. So then I pushed 3 commits to fix that just to realize that there were unneccessary. In order not to pollute this PR I reset hard these three commits with:

git reset --hard 94639c6f1
git push --force-with-lease

The only problem we have now is that it states "16 workflows awaiting approval" instead of "8 workflows awaiting approval" - maybe has to do something with my "hard reset" - sorry about that.
Apart from that, I should have addressed all your comments and I would be happy if you could further review:)

azurerm/internal/services/machinelearning/parse/inference_cluster.go

katbyte · 2021-05-07T21:30:46Z

THanks @gro1m - have 5 failing tests now:

aristosvo

A few comments to give you an idea how I'd start here to rework it. This makes it easier to cut to functionality later together

azurerm/internal/services/machinelearning/machine_learning_inference_cluster_resource.go

aristosvo

Round 2 😄

Your changes make it a lot easier for me to go through your code, just some small remarks on minor details left.

I went a bit further and inspected your implementation against the API from a TF point of view. What I see is that you implemented and filled almost all possible fields of the objects. I have a suspicion it might be too much for a first implementation (and probably not necessary), especially as it requires information as input which is not exposed from the resource itself.

Nice job anyway, I think it's almost there if we can tweak the input parameters a bit!

azurerm/internal/services/machinelearning/machine_learning_inference_cluster_resource.go

…-> no ignores in ImportStep anymore

aristosvo · 2021-05-14T21:20:38Z

@katbyte @gro1m I took the liberty to update the docs and tests based on the last 2 comments to make it easier to comment on/review it.

Hope you don't mind 🥺

azurerm/internal/services/machinelearning/machine_learning_inference_cluster_resource_test.go

gro1m · 2021-05-19T20:02:35Z

Did also test here the latest stand and every test passes.

TF_ACC=1 go test -v ./azurerm/internal/services/machinelearning -run=TestAccInferenceCluster -timeout 60m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/05/19 21:43:59 [DEBUG] not using binary driver name, it's no longer needed
2021/05/19 21:43:59 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccInferenceCluster_basic
=== PAUSE TestAccInferenceCluster_basic
=== RUN   TestAccInferenceCluster_requiresImport
=== PAUSE TestAccInferenceCluster_requiresImport
=== RUN   TestAccInferenceCluster_complete
=== PAUSE TestAccInferenceCluster_complete
=== RUN   TestAccInferenceCluster_completeProduction
=== PAUSE TestAccInferenceCluster_completeProduction
=== CONT  TestAccInferenceCluster_basic
=== CONT  TestAccInferenceCluster_completeProduction
=== CONT  TestAccInferenceCluster_complete
=== CONT  TestAccInferenceCluster_requiresImport
--- PASS: TestAccInferenceCluster_complete (898.54s)
--- PASS: TestAccInferenceCluster_requiresImport (917.88s)
--- PASS: TestAccInferenceCluster_completeProduction (951.42s)
--- PASS: TestAccInferenceCluster_basic (952.04s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/machinelearning     957.246s

gro1m · 2021-05-19T20:07:24Z

@ArcturusZhang Am I right that there is no sense to update anything here?
I checked with @aristosvo, but we did not find anything useful to update nor did it work.
The only parameters that we found that may be updatable were:

ssl -> we think that changes in SSL certificate should force the creation of a new inference cluster.
tags -> also do not seem to be updatable nor setable via the Portal UI.

katbyte

Thanks @gro1m - LGTM 👍

ghost · 2021-05-21T01:12:37Z

This has been released in version 2.60.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.60.0"
}
# ... other configuration ...

…11550) Fixes hashicorp#11252

github-actions · 2021-06-20T02:13:57Z

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.

Add support for Azure Machine Learning Inference Cluster Resource

97e4b03

ghost added size/XXL documentation labels May 1, 2021

gro1m changed the title ~~Add support for Azure Machine Learning Inference Cluster Resource~~ new resource azurerm_machine_learning_inference_cluster May 1, 2021

katbyte requested changes May 3, 2021

View reviewed changes

katbyte added service/machine-learning waiting-response labels May 3, 2021

Changes due to review#1

94639c6

ghost removed the waiting-response label May 5, 2021

gro1m force-pushed the r/azurerm_machine_learning_inference_cluster branch from e376dc1 to 94639c6 Compare May 5, 2021 14:41

gro1m added 3 commits May 5, 2021 20:08

Update website docs

9c92e3b

tflint (terrafmt -f .)

8206781

Fix golint errors (unused argument and comment formatting)

58dc726

gro1m commented May 5, 2021

View reviewed changes

azurerm/internal/services/machinelearning/parse/inference_cluster.go Show resolved Hide resolved

gro1m added 4 commits May 6, 2021 22:33

Fix tflint issues (S018, S020, R002, gofmt -s, whitespace)

c7685b9

make generate (use resourceids.go)

35117bb

Use node pool id and auto-generated node pool validators and parsers

09d96fe

update docs (website lint)

8c3f213

aristosvo suggested changes May 8, 2021

View reviewed changes

gro1m added 2 commits May 9, 2021 11:11

use camel case for all variables

5293cf2

Fix missing camel cases

48c75a7

aristosvo suggested changes May 9, 2021

View reviewed changes

Use ComputeResource to get AKS Cluster ID and other properties

c1d6dbe

katbyte added the waiting-response label May 9, 2021

Infer cluster_purpose, description and ssl config parameters as well …

c737bec

…-> no ignores in ImportStep anymore

Update docs

e35934e

gro1m requested a review from katbyte May 15, 2021 15:03

Remove superfluous node pool parse and validate files

7fc3646

gro1m commented May 15, 2021

View reviewed changes

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

Aris van Ommeren added 3 commits May 15, 2021 19:34

Fix FastProd test

2dd75e9

Remove go generate for agentpools

a715f51

Optimize cluster_purpose values and validation

650409e

aristosvo force-pushed the r/azurerm_machine_learning_inference_cluster branch from a5aed24 to 650409e Compare May 15, 2021 17:42

gro1m added 6 commits May 16, 2021 22:53

resolve merge conflict

23ae0d3

git pull

90761b5

update read of ml workspace id

423cf23

make fmt

5666a72

remove compute type

c704fe3

Fix cluster purpose validation: DenseProd -> DevTest

79ab9b7

upgrade aks api version to 2021-03-01

5c22aed

katbyte approved these changes May 19, 2021

View reviewed changes

katbyte added this to the v2.60.0 milestone May 19, 2021

katbyte merged commit 57b1158 into hashicorp:master May 19, 2021

katbyte added a commit that referenced this pull request May 19, 2021

CHANGELOG.md for #11550

b72ca4e

gro1m deleted the r/azurerm_machine_learning_inference_cluster branch May 20, 2021 06:10

favoretti pushed a commit to gro1m/terraform-provider-azurerm that referenced this pull request May 26, 2021

new resource azurerm_machine_learning_inference_cluster (hashicorp#…

edcd14a

…11550) Fixes hashicorp#11252

favoretti pushed a commit to gro1m/terraform-provider-azurerm that referenced this pull request May 26, 2021

CHANGELOG.md for hashicorp#11550

c062a7b

favoretti pushed a commit to gro1m/terraform-provider-azurerm that referenced this pull request May 26, 2021

new resource azurerm_machine_learning_inference_cluster (hashicorp#…

1972cbe

…11550) Fixes hashicorp#11252

favoretti pushed a commit to gro1m/terraform-provider-azurerm that referenced this pull request May 26, 2021

new resource azurerm_machine_learning_inference_cluster (hashicorp#…

9f0e1b8

…11550) Fixes hashicorp#11252

github-actions bot locked as resolved and limited conversation to collaborators Jun 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new resource `azurerm_machine_learning_inference_cluster` #11550

new resource `azurerm_machine_learning_inference_cluster` #11550

gro1m commented May 1, 2021 •

edited

Loading

katbyte left a comment

katbyte May 3, 2021

gro1m May 5, 2021

katbyte May 5, 2021

gro1m May 5, 2021 •

edited

Loading

gro1m commented May 5, 2021 •

edited

Loading

gro1m commented May 5, 2021

katbyte commented May 7, 2021

aristosvo left a comment

aristosvo left a comment

aristosvo commented May 14, 2021 •

edited

Loading

gro1m commented May 19, 2021

gro1m commented May 19, 2021 •

edited

Loading

katbyte left a comment

ghost commented May 21, 2021

github-actions bot commented Jun 20, 2021

new resource azurerm_machine_learning_inference_cluster #11550

new resource azurerm_machine_learning_inference_cluster #11550

Conversation

gro1m commented May 1, 2021 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

katbyte May 3, 2021

Choose a reason for hiding this comment

gro1m May 5, 2021

Choose a reason for hiding this comment

katbyte May 5, 2021

Choose a reason for hiding this comment

gro1m May 5, 2021 • edited Loading

Choose a reason for hiding this comment

gro1m commented May 5, 2021 • edited Loading

gro1m commented May 5, 2021

katbyte commented May 7, 2021

aristosvo left a comment

Choose a reason for hiding this comment

aristosvo left a comment

Choose a reason for hiding this comment

aristosvo commented May 14, 2021 • edited Loading

gro1m commented May 19, 2021

gro1m commented May 19, 2021 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

ghost commented May 21, 2021

github-actions bot commented Jun 20, 2021

new resource `azurerm_machine_learning_inference_cluster` #11550

new resource `azurerm_machine_learning_inference_cluster` #11550

gro1m commented May 1, 2021 •

edited

Loading

gro1m May 5, 2021 •

edited

Loading

gro1m commented May 5, 2021 •

edited

Loading

aristosvo commented May 14, 2021 •

edited

Loading

gro1m commented May 19, 2021 •

edited

Loading