-
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
azurerm_machine_learning_compute_cluster
- Add support for update identity
#26404
Changes from 3 commits
35c8972
5b7a531
bcb4900
f2089e2
3629a51
1bf2541
3512d78
515c029
2a85ab0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -27,6 +27,7 @@ func resourceComputeCluster() *pluginsdk.Resource { | |||||||||||||||||||||
return &pluginsdk.Resource{ | ||||||||||||||||||||||
Create: resourceComputeClusterCreate, | ||||||||||||||||||||||
Read: resourceComputeClusterRead, | ||||||||||||||||||||||
Update: resourceComputeClusterUpdate, | ||||||||||||||||||||||
Delete: resourceComputeClusterDelete, | ||||||||||||||||||||||
|
||||||||||||||||||||||
Importer: pluginsdk.ImporterValidatingResourceId(func(id string) error { | ||||||||||||||||||||||
|
@@ -37,6 +38,7 @@ func resourceComputeCluster() *pluginsdk.Resource { | |||||||||||||||||||||
Timeouts: &pluginsdk.ResourceTimeout{ | ||||||||||||||||||||||
Create: pluginsdk.DefaultTimeout(30 * time.Minute), | ||||||||||||||||||||||
Read: pluginsdk.DefaultTimeout(5 * time.Minute), | ||||||||||||||||||||||
Update: pluginsdk.DefaultTimeout(30 * time.Minute), | ||||||||||||||||||||||
Delete: pluginsdk.DefaultTimeout(30 * time.Minute), | ||||||||||||||||||||||
}, | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
@@ -69,7 +71,7 @@ func resourceComputeCluster() *pluginsdk.Resource { | |||||||||||||||||||||
ValidateFunc: validation.StringInSlice([]string{string(machinelearningcomputes.VMPriorityDedicated), string(machinelearningcomputes.VMPriorityLowPriority)}, false), | ||||||||||||||||||||||
}, | ||||||||||||||||||||||
|
||||||||||||||||||||||
"identity": commonschema.SystemAssignedUserAssignedIdentityOptionalForceNew(), | ||||||||||||||||||||||
"identity": commonschema.SystemAssignedUserAssignedIdentityOptional(), | ||||||||||||||||||||||
|
||||||||||||||||||||||
"scale_settings": { | ||||||||||||||||||||||
Type: pluginsdk.TypeList, | ||||||||||||||||||||||
|
@@ -355,6 +357,88 @@ func resourceComputeClusterRead(d *pluginsdk.ResourceData, meta interface{}) err | |||||||||||||||||||||
return tags.FlattenAndSet(d, computeResource.Model.Tags) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
func resourceComputeClusterUpdate(d *pluginsdk.ResourceData, meta interface{}) error { | ||||||||||||||||||||||
mlWorkspacesClient := meta.(*clients.Client).MachineLearning.Workspaces | ||||||||||||||||||||||
client := meta.(*clients.Client).MachineLearning.MachineLearningComputes | ||||||||||||||||||||||
ctx, cancel := timeouts.ForCreate(meta.(*clients.Client).StopContext, d) | ||||||||||||||||||||||
defer cancel() | ||||||||||||||||||||||
|
||||||||||||||||||||||
workspaceID, err := workspaces.ParseWorkspaceID(d.Get("machine_learning_workspace_id").(string)) | ||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
return err | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
id, err := machinelearningcomputes.ParseComputeID(d.Id()) | ||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
return err | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
workspace, err := mlWorkspacesClient.Get(ctx, *workspaceID) | ||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
return fmt.Errorf("retrieving %s: %+v", workspaceID, err) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
workspaceModel := workspace.Model | ||||||||||||||||||||||
if workspaceModel == nil { | ||||||||||||||||||||||
return fmt.Errorf("retrieving %s: `model` was nil", workspaceID) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
identity, err := expandIdentity(d.Get("identity").([]interface{})) | ||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
return fmt.Errorf("expanding `identity`: %+v", err) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
vmPriority := machinelearningcomputes.VMPriority(d.Get("vm_priority").(string)) | ||||||||||||||||||||||
computeClusterAmlComputeProperties := machinelearningcomputes.AmlComputeProperties{ | ||||||||||||||||||||||
VMSize: utils.String(d.Get("vm_size").(string)), | ||||||||||||||||||||||
VMPriority: &vmPriority, | ||||||||||||||||||||||
ScaleSettings: expandScaleSettings(d.Get("scale_settings").([]interface{})), | ||||||||||||||||||||||
UserAccountCredentials: expandUserAccountCredentials(d.Get("ssh").([]interface{})), | ||||||||||||||||||||||
EnableNodePublicIP: pointer.To(d.Get("node_public_ip_enabled").(bool)), | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
computeClusterAmlComputeProperties.RemoteLoginPortPublicAccess = pointer.To(machinelearningcomputes.RemoteLoginPortPublicAccessDisabled) | ||||||||||||||||||||||
if d.Get("ssh_public_access_enabled").(bool) { | ||||||||||||||||||||||
computeClusterAmlComputeProperties.RemoteLoginPortPublicAccess = pointer.To(machinelearningcomputes.RemoteLoginPortPublicAccessEnabled) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
if subnetId, ok := d.GetOk("subnet_resource_id"); ok && subnetId.(string) != "" { | ||||||||||||||||||||||
computeClusterAmlComputeProperties.Subnet = &machinelearningcomputes.ResourceId{Id: subnetId.(string)} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// NOTE: The 'AmlCompute' 'ComputeLocation' field should always point | ||||||||||||||||||||||
// to configuration files 'location' field... | ||||||||||||||||||||||
computeClusterProperties := machinelearningcomputes.AmlCompute{ | ||||||||||||||||||||||
Properties: &computeClusterAmlComputeProperties, | ||||||||||||||||||||||
ComputeLocation: utils.String(d.Get("location").(string)), | ||||||||||||||||||||||
Description: utils.String(d.Get("description").(string)), | ||||||||||||||||||||||
DisableLocalAuth: utils.Bool(!d.Get("local_auth_enabled").(bool)), | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// NOTE: The 'ComputeResource' 'Location' field should always point | ||||||||||||||||||||||
// to the workspace's 'location'... | ||||||||||||||||||||||
computeClusterParameters := machinelearningcomputes.ComputeResource{ | ||||||||||||||||||||||
Properties: computeClusterProperties, | ||||||||||||||||||||||
Identity: identity, | ||||||||||||||||||||||
Location: workspaceModel.Location, | ||||||||||||||||||||||
Tags: tags.Expand(d.Get("tags").(map[string]interface{})), | ||||||||||||||||||||||
Sku: &machinelearningcomputes.Sku{ | ||||||||||||||||||||||
Name: workspaceModel.Sku.Name, | ||||||||||||||||||||||
Tier: pointer.To(machinelearningcomputes.SkuTier(*workspaceModel.Sku.Tier)), | ||||||||||||||||||||||
}, | ||||||||||||||||||||||
} | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should still be able to retrieve the existing Compute Cluster and patch the SKU from the workspace into the model instead of having to set everything from the config like in the create? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or we can try update without SKU? if this works we don't need to get the workspace in update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, try it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tried, can update without SKU. Also the API doc the SKU is the SKU for the workspace(https://learn.microsoft.com/en-us/rest/api/azureml/compute/create-or-update?view=rest-azureml-2024-04-01&tabs=HTTP#request-body). So we can ignore it or get it from the workspace, do we need to remove it in update? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you can update successfully without sending anything for the SKU (and it doesn't change the SKU) then it's fine to omit getting it from the workspace. |
||||||||||||||||||||||
|
||||||||||||||||||||||
future, err := client.ComputeCreateOrUpdate(ctx, *id, computeClusterParameters) | ||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
return fmt.Errorf("creating %s: %+v", id, err) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
if err := future.Poller.PollUntilDone(ctx); err != nil { | ||||||||||||||||||||||
return fmt.Errorf("waiting for creation of %s: %+v", id, err) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
|
||||||||||||||||||||||
return resourceComputeClusterRead(d, meta) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
func resourceComputeClusterDelete(d *pluginsdk.ResourceData, meta interface{}) error { | ||||||||||||||||||||||
client := meta.(*clients.Client).MachineLearning.MachineLearningComputes | ||||||||||||||||||||||
ctx, cancel := timeouts.ForDelete(meta.(*clients.Client).StopContext, d) | ||||||||||||||||||||||
|
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 do we need to get the workspace here? Can't we retrieve all the info on the Compute Cluster by calling
client.ComputeGet
?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 SKU used by the compute cluster resources is the
SKU
of the workspace. And compute GET API will not return the SKU, but nil.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.
That sounds like a bug we should raise on the Rest API Spec, can you please open one?