Skip to content
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

[Internal] Restart Cluster before Library Installation #4384

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

parthban-db
Copy link
Contributor

@parthban-db parthban-db commented Jan 8, 2025

Changes

Library resource with SDK_V2 implementation waits for the cluster to get into the running state before starting installation. This PR mimics this behavior in the plugin framework implementation of the library resource.

Fixes #4353

Tests

Added a integration test.

  • make test run locally
  • relevant change in docs/ folder
  • covered with integration tests in internal/acceptance
  • using Go SDK
  • using TF Plugin Framework

@parthban-db parthban-db requested review from a team as code owners January 8, 2025 21:45
@parthban-db parthban-db requested review from rauchy and removed request for a team January 8, 2025 21:45
Copy link

github-actions bot commented Jan 8, 2025

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/terraform

Inputs:

  • PR number: 4384
  • Commit SHA: ee11c559de855e4d26047e0eed963e026989ea57

Checks will be approved automatically on success.

@alexott
Copy link
Contributor

alexott commented Jan 9, 2025

@parthban-db we have #4036 that mentions that we can install libraries in UI without starting the cluster - the question is if it works via API the same way or not...

Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general is good, see my comment about existing issue about installing on terminated clusters via UI

@parthban-db
Copy link
Contributor Author

@parthban-db we have #4036 that mentions that we can install libraries in UI without starting the cluster - the question is if it works via API the same way or not...

I will ask the team about this.

@parthban-db
Copy link
Contributor Author

@parthban-db we have #4036 that mentions that we can install libraries in UI without starting the cluster - the question is if it works via API the same way or not...

@alexott I talked with the team, and according to them, /api/2.0/libraries/install should not need to start the cluster, but the actual installation on the dataplane will require the cluster to be live.
I'm not sure how we want Terraform to behave in this case. We can either start the cluster and wait for it to go live or just create the library, and the library will be installed once the cluster goes live.

@alexott
Copy link
Contributor

alexott commented Jan 10, 2025

@parthban-db, We can make it controllable by the user - we start the cluster and install the library just to check if there are no errors. But if we can allow users to specify that they don't want to wait for it, then it could be a solution.

@mgyucht wdyt?

@mgyucht
Copy link
Contributor

mgyucht commented Jan 10, 2025

LGTM: we revert this back to the original & documented behavior of starting the cluster if it isn't already running.

Longer term, we can revisit how Terraform users will think about cluster and library lifecycles. I can see this as very useful in some use-cases (e.g. DABs), but there may also be users who don't want a cluster to be started for the sake of creating/updating a library.

PreConfig: func() {
// Delete the created cluster
w := databricks.Must(databricks.NewWorkspaceClient())
getter, err := w.Clusters.Delete(context.Background(), compute.DeleteCluster{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a confusing name for this method... @renaudhartert-db maybe we want to revisit this in the next version of the SDK. It should really be "terminate cluster" to align with basically all other terminology.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants