Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 214
Normalize clusters in jobs according to Terraform provider logic#3198
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
1ec84ccf738d76cf30853c02364b691b9d5118f0b806712d64554189File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Uh oh!
There was an error while loading. Please reload this page.
This file was deleted.
Uh oh!
There was an error while loading. Please reload this page.
This file was deleted.
Uh oh!
There was an error while loading. Please reload this page.
This file was deleted.
Uh oh!
There was an error while loading. Please reload this page.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,2 @@ | ||
| # Temporarily disabling due to DBR release breakage. | ||
| CloudEnvs.gcp = false | ||
| EnvMatrix.DATABRICKS_CLI_DEPLOYMENT = ["terraform"] # Error: deploying jobs.some_other_job: creating: Method=Jobs.Create *retries.Err *apierr.APIError StatusCode=400 ErrorCode="INVALID_PARAMETER_VALUE" Message="The field 'node_type_id' cannot be supplied when an instance pool ID is provided." |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,2 @@ | ||
| # Temporarily disabling due to DBR release breakage. | ||
| CloudEnvs.gcp = false | ||
| EnvMatrix.DATABRICKS_CLI_DEPLOYMENT = ["terraform"] # Error: deploying jobs.some_other_job: creating: Method=Jobs.Create *retries.Err *apierr.APIError StatusCode=400 ErrorCode="INVALID_PARAMETER_VALUE" Message="The field 'node_type_id' cannot be supplied when an instance pool ID is provided." |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| package resourcemutator | ||
| import ( | ||
| "context" | ||
| "github.com/databricks/cli/bundle" | ||
| "github.com/databricks/cli/libs/diag" | ||
| "github.com/databricks/databricks-sdk-go/service/compute" | ||
| "github.com/databricks/databricks-sdk-go/service/jobs" | ||
| ) | ||
| type jobClustersFixups struct{} | ||
| func JobClustersFixups() bundle.Mutator { | ||
| return &jobClustersFixups{} | ||
| } | ||
| func (m *jobClustersFixups) Name() string { | ||
| return "JobClustersFixups" | ||
| } | ||
| func (m *jobClustersFixups) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { | ||
| for _, job := range b.Config.Resources.Jobs { | ||
| if job == nil { | ||
| continue | ||
| } | ||
| // TODO: we should raise a warning when user specify both InstancePoolId and NodeTypeId since it's illegal. | ||
| // Once we remove TF backend and had warning for some time, we can remove this transformation, the backend | ||
| // will reject such configs. | ||
| prepareJobSettingsForUpdate(&job.JobSettings) | ||
| } | ||
| return nil | ||
| } | ||
| // Copied from | ||
| // https://github.com/databricks/terraform-provider-databricks/blob/a8c92bb/clusters/resource_cluster.go | ||
| // https://github.com/databricks/terraform-provider-databricks/blob/a8c92bb/clusters/clusters_api.go#L440 | ||
| func ModifyRequestOnInstancePool(c *compute.ClusterSpec) { | ||
| // Instance profile id does not exist or not set | ||
| if c.InstancePoolId == "" { | ||
| // Worker must use an instance pool if driver uses an instance pool, | ||
| // therefore empty the computed value for driver instance pool. | ||
| c.DriverInstancePoolId = "" | ||
| return | ||
| } | ||
| if c.AwsAttributes != nil { | ||
| // Reset AwsAttributes | ||
| awsAttributes := compute.AwsAttributes{ | ||
| InstanceProfileArn: c.AwsAttributes.InstanceProfileArn, | ||
| } | ||
| c.AwsAttributes = &awsAttributes | ||
| } | ||
| if c.AzureAttributes != nil { | ||
| c.AzureAttributes = &compute.AzureAttributes{} | ||
| } | ||
| if c.GcpAttributes != nil { | ||
| gcpAttributes := compute.GcpAttributes{ | ||
| GoogleServiceAccount: c.GcpAttributes.GoogleServiceAccount, | ||
| } | ||
| c.GcpAttributes = &gcpAttributes | ||
| } | ||
| c.EnableElasticDisk = false | ||
| c.NodeTypeId = "" | ||
| c.DriverNodeTypeId = "" | ||
| } | ||
| // Copied https://github.com/databricks/terraform-provider-databricks/blob/a8c92bb130def431b3fadd9fd533c463e8d4813b/jobs/resource_job.go#L1016 | ||
| func prepareJobSettingsForUpdate(js *jobs.JobSettings) { | ||
| for _, task := range js.Tasks { | ||
| if task.NewCluster != nil { | ||
| ModifyRequestOnInstancePool(task.NewCluster) | ||
| } | ||
| } | ||
| for ind := range js.JobClusters { | ||
| ModifyRequestOnInstancePool(&js.JobClusters[ind].NewCluster) | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
This clears computed fields on the TF side. We don't have that problem here because we exclusively use the configuration as input to our requests. What doesn't work if we don't apply these fixups?
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.
See the tests that were enabled in this PR.
Other fields are copied from TF for completeness -- If they are output-only it does not hurt to clear them.
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.
They are output only IFF the instance pool is set.
If a user configures both an instance pool and a node type ID, I think we should raise 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.
Well, that would be a breaking change, so we probably should start with a warning. However, this is out of scope in this PR, here I'm just surfacing what TF already does so that we have parity between deployment backends.
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 makes sense. Can you throw in a TODO for good measure so we look at this later?
I prefer removing these modifications if we don't need them once direct is the only deployment mode.