-
Notifications
You must be signed in to change notification settings - Fork 461
[Draft] Add expected_workspace_status in databricks_mws_workspaces to support LPW #5019
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
base: main
Are you sure you want to change the base?
Conversation
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.
a few comments
please run |
…ete ws in provisioning state
jenkins trigger all |
mws/mws_workspaces_test.go
Outdated
}) | ||
} | ||
|
||
func TestMwsAccGcpWorkspacesWithExpectedProvisioning(t *testing.T) { |
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.
Integration test run (link)
On its last run, this integration test failed during the post-test destroy due to not being able to delete a still PROVISIONING workspace. Error message:
Error: -26T17:50:17.498Z [ERROR] sdk.helper_resource: Error running post-test destroy, there may be dangling resources: test_terraform_path=/tmp/plugintest-terraform1770350163/terraform test_step_number=1
| Error: cannot delete mws workspaces: INVALID_STATE: workspace 49063535405170 cannot be deleted because it is in status PROVISIONING but not must be in one of: PROVISIONING
I pushed a new commit to catch INVALID_STATE_TRANSITION
errors in DELETE
, but this is probably not the best approach - not sure if there is a way to have the integration test instead run through both CREATE and UPDATE so that the workspace is in a RUNNING
status? cc @panchenxue-databricks
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.
You probably need to support this case of deleting workspaces in PROVISIONING, since users could attempt to delete a partially created workspace, or they could modify an attribute that requires recreation (like deployment name).
For this test, you could also just add an additional step to put the workspace in a running state by updating expected_workspace_status
.
@tinglin-db Now it fails with:
that's why I think that deletion in the provisioning state should be handled on backend, otherwise there will be dangling resources. |
WorkspaceURL string `json:"workspace_url,omitempty" tf:"computed"` | ||
WorkspaceStatus string `json:"workspace_status,omitempty" tf:"computed"` | ||
WorkspaceStatusMessage string `json:"workspace_status_message,omitempty" tf:"computed"` | ||
ExpectedWorkspaceStatus string `json:"expected_workspace_status,omitempty"` |
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.
We should also document this in docs/resources/mws_workspaces.md.
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.
Is this true even if the feature is still in private preview?
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.
Yes. Private preview services, methods and fields (and their documentation) are still part of the SDKs. Currently, private preview methods are not shown in the REST API documentation. For example: https://registry.terraform.io/providers/databricks/databricks/latest/docs/resources/feature_engineering_feature
I would like us to add badges on individual fields if their stability level lags behind the resource level, but we haven't gotten to that yet. In the meantime, we can add a note in the documentation that this feature is in private preview.
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.
Added documentation, let me know if the wording works
mws/resource_mws_workspaces.go
Outdated
if expectedStatus == WorkspaceStatusProvisioning { | ||
log.Printf("[INFO] Expected status is PROVISIONING, skipping wait in read function") | ||
err = nil | ||
} else { | ||
err = workspacesAPI.WaitForExpectedStatus(workspace, d.Timeout(schema.TimeoutRead)) | ||
} |
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.
We should have one place for logic around waiting for workspace status. I believe this is unnecessary because the implementation of WaitForExpectedStatus already handles this properly.
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.
I modified WaitForExpectedStatus to take in the expected_status. We need to pass it in from d.Get("expected_workspace_status")
because the field is not returned by the GET during polling
… logic, removed deletion skip, removed read skip
3af696c
to
08466fc
Compare
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.
Aligned on the pathway forward, let's wrap this up.
mws/resource_mws_workspaces.go
Outdated
return err | ||
} | ||
return a.WaitForRunning(ws, timeout) | ||
return a.WaitForExpectedStatus(ws, WorkspaceStatusRunning, timeout) |
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.
Let's set the expected workspace status that is provided by the user in this request.
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.
Updated
mws/mws_workspaces_test.go
Outdated
}) | ||
} | ||
|
||
func TestMwsAccGcpWorkspacesWithExpectedProvisioning(t *testing.T) { |
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.
Separately, two more test cases:
- New workspace in provisioning state -> set expected state to RUNNING
- New workspace in provisioning state -> unset expected state, workspace should progress to RUNNING.
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.
Sounds good, added these test cases
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.
Very close, some nits but this is basically looking good to me.
WorkspaceURL string `json:"workspace_url,omitempty" tf:"computed"` | ||
WorkspaceStatus string `json:"workspace_status,omitempty" tf:"computed"` | ||
WorkspaceStatusMessage string `json:"workspace_status_message,omitempty" tf:"computed"` | ||
ExpectedWorkspaceStatus string `json:"expected_workspace_status,omitempty"` |
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.
Yes. Private preview services, methods and fields (and their documentation) are still part of the SDKs. Currently, private preview methods are not shown in the REST API documentation. For example: https://registry.terraform.io/providers/databricks/databricks/latest/docs/resources/feature_engineering_feature
I would like us to add badges on individual fields if their stability level lags behind the resource level, but we haven't gotten to that yet. In the meantime, we can add a note in the documentation that this feature is in private preview.
mws/resource_mws_workspaces.go
Outdated
} | ||
|
||
var workspaceRunningUpdatesAllowed = []string{"credentials_id", "network_id", "storage_customer_managed_key_id", "private_access_settings_id", "managed_services_customer_managed_key_id", "custom_tags"} | ||
var workspaceRunningUpdatesAllowed = []string{"credentials_id", "network_id", "storage_customer_managed_key_id", "private_access_settings_id", "managed_services_customer_managed_key_id", "custom_tags", "expected_workspace_status"} |
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.
I know this isn't your fault, but would you mind formatting this list with one entry per line? Changes to this list are easier to review this way.
mws/resource_mws_workspaces.go
Outdated
func (a WorkspacesAPI) WaitForRunning(ws Workspace, timeout time.Duration) error { | ||
// If expected_workspace_status is specified, WaitForExpectedStatus will wait until workspace is in the expected status. | ||
// If not, it will wait until workspace is running, and otherwise will try to explain why it failed. | ||
func (a WorkspacesAPI) WaitForExpectedStatus(ws Workspace, expected_status string, timeout time.Duration) 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.
func (a WorkspacesAPI) WaitForExpectedStatus(ws Workspace, expected_status string, timeout time.Duration) error { | |
func (a WorkspacesAPI) WaitForExpectedStatus(ws Workspace, expectedStatus string, timeout time.Duration) error { |
err = workspacesAPI.WaitForRunning(workspace, d.Timeout(schema.TimeoutRead)) | ||
// The expected_workspace_status field is input only. | ||
// Therefore, we need to read it from the original Terraform configuration. | ||
expectedStatus := d.Get("expected_workspace_status").(string) |
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.
expectedStatus := d.Get("expected_workspace_status").(string) | |
// PROVISIONING workspace import may fail because the "expected_workspace_status" is not included in the state during import, nor is it returned by the API. | |
// As a result, the provider will (likely) wait for RUNNING state, which will never happen, and timeout. | |
// TODO: fix thisl. | |
expectedStatus := d.Get("expected_workspace_status").(string) |
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.
Added comment
a5a7519
to
c2104e4
Compare
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
Changes
To support least privileged workspaces on GCP, adding a new field
expected_workspace_status
, which will be translated toworkspace_state
in the API request.Tests
Added a unit test
TestResourceWorkspaceCreateGcpWithExpectedProvisioning
and an integration testTestMwsAccGcpWorkspacesWithExpectedProvisioning
make test
run locallyNEXT_CHANGELOG.md
fileTested E2E Locally