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

Sort tasks by task_key before generating the Terraform configuration #1776

Merged
merged 7 commits into from
Sep 26, 2024

Conversation

shreyas-goenka
Copy link
Contributor

@shreyas-goenka shreyas-goenka commented Sep 17, 2024

Changes

Sort the tasks in the resultant bundle.tf.json. This is important because configuration from one task can leak into another if the tasks are not sorted.

For more details see:

  1. [ISSUE] Issue with databricks_jobs resource: default for source ignored terraform-provider-databricks#3951
  2. [ISSUE] Issue with databricks_job resource: Job with two tasks keeps flipping the tasks and never reached a stable state terraform-provider-databricks#4011

Tests

Unit test and manually.

For manual testing I used the following configuration:

resources:
  jobs:
    foo:
      tasks: 
        - task_key: task-Z
          notebook_task: 
            notebook_path: nb.py
            source: GIT
          existing_cluster_id: 0715-133738-ju0ma84z

        - task_key: task-1
          notebook_task: 
            notebook_path: ${workspace.file_path}/local.py
            source: WORKSPACE
          existing_cluster_id: 0715-133738-ju0ma84z
          depends_on: 
            - task_key: task-Z


      git_source: 
        git_provider: gitHub
        git_url: https://github.com/shreyas-goenka/job-source-tmp.git
        git_branch: main

Steps (1):

  1. Deploy this bundle.
  2. Comment out "source: GIT"
  3. Deploy again

Before:
Deploying this bundle twice would fail. This is because the "source: GIT" would carry over to the next deployment.

After:
There was no error on the subsequent deployment.

Steps (2):

  1. Deploy once
  2. Deploy again

Before:
Works correctly but leads to a update API call every time.

After:
No diff is detected by terraform.

@shreyas-goenka shreyas-goenka marked this pull request as ready for review September 17, 2024 16:42
@shreyas-goenka
Copy link
Contributor Author

Note: This stabilises the plan because tasks are an ordered list in Terraform, and Terraform computes the diff for an ordered list based on the index.

Sorting works because the Databricks Terraform provider sorts the task keys when returned by the GET job API, resulting in a stable plan.

This is not a complete solution yet because a new task can cause displacement in the ordered task list, which might possibly cause configuration intermixing across tasks. However, solving this in Terraform requires a migration to the Terraform plugin for databricks_job.

@pietern
Copy link
Contributor

pietern commented Sep 23, 2024

@shreyas-goenka Do you know where the tasks are reordered?

They are an array in the configuration and, therefore, should have stable order all the way until they hit the backend.

@shreyas-goenka
Copy link
Contributor Author

shreyas-goenka commented Sep 23, 2024

@pietern The tasks are reordered in the terraform provider itself. They do not seem to be altered by the API based on my personal testing.

The code reference for sorting it is three years old, so it might have been reordered by the API at some point and then fixed. We could very well fix this upstream as well by not sorting by task key anymore. I'll need to run this by @mgyucht and the Jobs team to confirm that an upstream patch could work.

ref:
https://github.com/databricks/terraform-provider-databricks/blame/af46555600f4895d84419675ae445bfe61c4143c/jobs/resource_job.go#L341.

@shreyas-goenka
Copy link
Contributor Author

Note: The order of the tasks is faithfully retained by both the API get body and the YAML file for the job in the UI.

@pietern pietern changed the title Sort tasks by task_key before generating the terraform configuration Sort tasks by task_key before generating the Terraform configuration Sep 26, 2024
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Thanks!

bundle/deploy/terraform/tfdyn/convert_job.go Show resolved Hide resolved
bundle/deploy/terraform/tfdyn/convert_job.go Show resolved Hide resolved
@shreyas-goenka shreyas-goenka added this pull request to the merge queue Sep 26, 2024
Merged via the queue into main with commit 4e8e027 Sep 26, 2024
5 checks passed
@shreyas-goenka shreyas-goenka deleted the sort-job-tasks branch September 26, 2024 13:30
andrewnester added a commit that referenced this pull request Oct 1, 2024
Bundles:
 * Added support for creating all-purpose clusters ([#1698](#1698)).
 * Reduce time until the prompt is shown for bundle run ([#1727](#1727)).
 * Use Unity Catalog for pipelines in the default-python template ([#1766](#1766)).
 * Add verbose flag to the "bundle deploy" command ([#1774](#1774)).
 * Fixed full variable override detection ([#1787](#1787)).
 * Add sub-extension to resource files in built-in templates ([#1777](#1777)).
 * Fix panic in `apply_presets.go` ([#1796](#1796)).

Internal:
 * Assert tokens are redacted in origin URL when username is not specified ([#1785](#1785)).
 * Refactor jobs path translation ([#1782](#1782)).
 * Add JobTaskClusterSpec validate mutator ([#1784](#1784)).
 * Pin Go toolchain to 1.22.7 ([#1790](#1790)).
 * Modify SetLocation test utility to take full locations as argument ([#1788](#1788)).
 * Simplified isFullVariableOverrideDef implementation ([#1791](#1791)).
 * Sort tasks by `task_key` before generating the Terraform configuration ([#1776](#1776)).
 * Trim trailing whitespace ([#1794](#1794)).
 * Move trampoline code into trampoline package ([#1793](#1793)).
 * Rename `RootPath` -> `BundleRootPath` ([#1792](#1792)).

API Changes:
 * Changed `databricks apps delete` command to return .
 * Changed `databricks apps deploy` command with new required argument order.
 * Changed `databricks apps start` command to return .
 * Changed `databricks apps stop` command to return .
 * Added `databricks temporary-table-credentials` command group.
 * Added `databricks serving-endpoints put-ai-gateway` command.
 * Added `databricks disable-legacy-access` command group.
 * Added `databricks account disable-legacy-features` command group.

OpenAPI commit 6f6b1371e640f2dfeba72d365ac566368656f6b6 (2024-09-19)
Dependency updates:
 * Upgrade to Go SDK 0.47.0 ([#1799](#1799)).
 * Upgrade to TF provider 1.52 ([#1781](#1781)).
 * Bump golang.org/x/mod from 0.20.0 to 0.21.0 ([#1758](#1758)).
 * Bump github.com/hashicorp/hc-install from 0.7.0 to 0.9.0 ([#1772](#1772)).
github-merge-queue bot pushed a commit that referenced this pull request Oct 1, 2024
Bundles:
* Added support for creating all-purpose clusters
([#1698](#1698)).
* Reduce time until the prompt is shown for bundle run
([#1727](#1727)).
* Use Unity Catalog for pipelines in the default-python template
([#1766](#1766)).
* Add verbose flag to the "bundle deploy" command
([#1774](#1774)).
* Fixed full variable override detection
([#1787](#1787)).
* Add sub-extension to resource files in built-in templates
([#1777](#1777)).
* Fix panic in `apply_presets.go`
([#1796](#1796)).

Internal:
* Assert tokens are redacted in origin URL when username is not
specified ([#1785](#1785)).
* Refactor jobs path translation
([#1782](#1782)).
* Add JobTaskClusterSpec validate mutator
([#1784](#1784)).
* Pin Go toolchain to 1.22.7
([#1790](#1790)).
* Modify SetLocation test utility to take full locations as argument
([#1788](#1788)).
* Simplified isFullVariableOverrideDef implementation
([#1791](#1791)).
* Sort tasks by `task_key` before generating the Terraform configuration
([#1776](#1776)).
* Trim trailing whitespace
([#1794](#1794)).
* Move trampoline code into trampoline package
([#1793](#1793)).
* Rename `RootPath` -> `BundleRootPath`
([#1792](#1792)).

API Changes:
 * Changed `databricks apps delete` command to return .
* Changed `databricks apps deploy` command with new required argument
order.
 * Changed `databricks apps start` command to return .
 * Changed `databricks apps stop` command to return .
 * Added `databricks temporary-table-credentials` command group.
 * Added `databricks serving-endpoints put-ai-gateway` command.
 * Added `databricks disable-legacy-access` command group.
 * Added `databricks account disable-legacy-features` command group.

OpenAPI commit 6f6b1371e640f2dfeba72d365ac566368656f6b6 (2024-09-19)
Dependency updates:
* Upgrade to Go SDK 0.47.0
([#1799](#1799)).
* Upgrade to TF provider 1.52
([#1781](#1781)).
* Bump golang.org/x/mod from 0.20.0 to 0.21.0
([#1758](#1758)).
* Bump github.com/hashicorp/hc-install from 0.7.0 to 0.9.0
([#1772](#1772)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants