Added `databricks labs ucx repair-run --step ...` CLI command for repair run of any failed workflows, like `assessment`, `migrate-groups` etc. by prajin-29 · Pull Request #724 · databrickslabs/ucx

prajin-29 · 2023-12-22T02:28:04Z

This may close #658

codecov · 2023-12-22T02:29:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (24d4acb) 79.45% compared to head (5bc0dc8) 79.58%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #724      +/-   ##
==========================================
+ Coverage   79.45%   79.58%   +0.13%     
==========================================
  Files          42       42              
  Lines        4517     4546      +29     
  Branches      845      849       +4     
==========================================
+ Hits         3589     3618      +29     
  Misses        710      710              
  Partials      218      218

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nfx

change the title of the pull request to be changelog-ready. See other commit messages on main branch for consistency examples

nfx · 2023-12-22T08:37:13Z


+    def repair_run(self, workflow):
+        try:
+            job_id_list = [job_id for step, job_id in self._state.jobs.items() if workflow == step]


There's only one job id per step, use job_id = state.jobs.get(step), not the list comprehension

changed accordingly

nfx · 2023-12-22T08:38:29Z

+            if job_id_list:
+                job_id = job_id_list[0]
+                job_runs = list(self._ws.jobs.list_runs(job_id=job_id, limit=1))
+                state = job_runs[0].state


Bug: will crash when there are no runs

Changed the code. If there is no runs it will throw a warning message

nfx · 2023-12-22T08:39:51Z

+                job_runs = list(self._ws.jobs.list_runs(job_id=job_id, limit=1))
+                state = job_runs[0].state
+                if job_runs and state.result_state.value != "SUCCESS":
+                    run_id = job_runs[0].run_id


Extract first run into a variable

Changed the code now.

nfx · 2023-12-22T08:40:22Z

+                if job_runs and state.result_state.value != "SUCCESS":
+                    run_id = job_runs[0].run_id
+                    self._ws.jobs.repair_run(run_id=run_id, rerun_all_failed_tasks=True)
+                    logger.info("Exception")


What is this?

Added for testing.Removed now from the code

nfx · 2023-12-22T08:42:42Z

+    def repair_run(self, workflow):
+        try:
+            job_id_list = [job_id for step, job_id in self._state.jobs.items() if workflow == step]
+            if job_id_list:


Can you invert the logic a bit: has to be "if job id is not found, show a warning and return immediately". Don't delay that to an else statement. See contributing.md - I think i added explanation why

inverted the logic.

nfx · 2023-12-22T08:47:23Z

Can you also add an integration test?

nfx · 2023-12-27T09:31:39Z

+                job_runs = list(self._ws.jobs.list_runs(job_id=job_id, limit=1))
+                if not job_runs:
+                    logger.warning(f"{workflow} job is not initialized yet. Can't trigger repair run now")
+                else:


You don't need "else" here, just de-dent the next code block

Changed according

nfx · 2023-12-27T09:35:18Z

+@retried(on=[NotFound, InvalidParameterValue, OperationFailed], timeout=timedelta(minutes=10))
+def test_repair_run_assessment_job(ws, new_installation, caplog):
+    install = new_installation()
+    install.run_workflow("assessment")


It's easier to "break" the destroy database workflow, where you can inject failure by removing the database before starting the job

used destroy database workflow to test the repair run in Integration test

nfx · 2023-12-27T09:36:54Z

Make pull request title changelog-friendly

nfx · 2023-12-28T09:05:30Z

+            if state.result_state.value != "SUCCESS":
+                run_id = latest_job_run.run_id
+                job_run_waiter = self._ws.jobs.repair_run(run_id=run_id, rerun_all_failed_tasks=True)
+                job_run_waiter.result()


This would timeout in 20 minutes. What you should do instead is construct a url of this new run and open it in browser and return from cli

removed calling the result.Instead opening the run url in browser.

# Conflicts: # labs.yml # src/databricks/labs/ucx/cli.py

…n browser.

prajin-29 · 2023-12-28T15:58:44Z

@nfx All the comments has been implemented.Kindly review the same.

nfx

Lgtm

* Added `databricks labs ucx repair-run --step ...` CLI command for repair run of any failed workflows, like `assessment`, `migrate-groups` etc. ([#724](#724)). * Added `databricks labs ucx revert-migrated-table` command ([#729](#729)). * Allow specifying a group list when group match options are used ([#725](#725)). * Fixed installation issue when upgrading from an older version of the tool and improve logs ([#740](#740)). * Renamed summary panel from Failure Summary to Assessment Summary ([#733](#733)). * Retry internal error when getting permissions and update legacy table ACL documentation ([#728](#728)). * Speedup installer execution ([#727](#727)).

…air run of any failed workflows, like `assessment`, `migrate-groups` etc. (#724)

* Added `databricks labs ucx repair-run --step ...` CLI command for repair run of any failed workflows, like `assessment`, `migrate-groups` etc. ([#724](#724)). * Added `databricks labs ucx revert-migrated-table` command ([#729](#729)). * Allow specifying a group list when group match options are used ([#725](#725)). * Fixed installation issue when upgrading from an older version of the tool and improve logs ([#740](#740)). * Renamed summary panel from Failure Summary to Assessment Summary ([#733](#733)). * Retry internal error when getting permissions and update legacy table ACL documentation ([#728](#728)). * Speedup installer execution ([#727](#727)).

…air run of any failed workflows, like `assessment`, `migrate-groups` etc. (#724)

* Added `databricks labs ucx repair-run --step ...` CLI command for repair run of any failed workflows, like `assessment`, `migrate-groups` etc. ([#724](#724)). * Added `databricks labs ucx revert-migrated-table` command ([#729](#729)). * Allow specifying a group list when group match options are used ([#725](#725)). * Fixed installation issue when upgrading from an older version of the tool and improve logs ([#740](#740)). * Renamed summary panel from Failure Summary to Assessment Summary ([#733](#733)). * Retry internal error when getting permissions and update legacy table ACL documentation ([#728](#728)). * Speedup installer execution ([#727](#727)).

Adding cli command for repair run

7940ace

prajin-29 requested review from a team and priyal-c December 22, 2023 02:28

prajin-29 had a problem deploying to account-admin December 22, 2023 02:28 — with GitHub Actions Failure

Adding cli command for repair run

e33e28f

prajin-29 had a problem deploying to account-admin December 22, 2023 06:46 — with GitHub Actions Failure

Adding cli command for repair run

18d0e79

prajin-29 had a problem deploying to account-admin December 22, 2023 07:56 — with GitHub Actions Failure

nfx requested changes Dec 22, 2023

View reviewed changes

prajin-29 added 2 commits December 27, 2023 14:03

Adding cli command for repair run

70d075d

Merge branch 'main' into feature/retry_run_logic_cli

482397d

prajin-29 temporarily deployed to account-admin December 27, 2023 08:34 — with GitHub Actions Inactive

Updated the repair_run code as per the comments.

b13d0ba

prajin-29 had a problem deploying to account-admin December 27, 2023 08:41 — with GitHub Actions Failure

Updated the timeout interval

26fb60a

prajin-29 had a problem deploying to account-admin December 27, 2023 09:08 — with GitHub Actions Failure

nfx requested changes Dec 27, 2023

View reviewed changes

Updated the timeout interval

1a7e1dc

prajin-29 had a problem deploying to account-admin December 28, 2023 08:44 — with GitHub Actions Failure

prajin-29 changed the title ~~Adding cli command for repair run~~ Added New CLI command to repair run a workflow Dec 28, 2023

Updated the integration test

f699fa9

nfx reviewed Dec 28, 2023

View reviewed changes

Merge branch 'main' into feature/retry_run_logic_cli

8c78752

# Conflicts: # labs.yml # src/databricks/labs/ucx/cli.py

prajin-29 had a problem deploying to account-admin December 28, 2023 09:55 — with GitHub Actions Failure

prajin-29 changed the title ~~Added New CLI command to repair run a workflow~~ Added New CLI command to repair run any Failed workflows(assessment,migrate-groups etc.) Dec 28, 2023

prajin-29 changed the title ~~Added New CLI command to repair run any Failed workflows(assessment,migrate-groups etc.)~~ Added databricks labs ucx repair-run --step ... to CLI commands for repair running any failed workflows(assessment,migrate-groups etc) Dec 28, 2023

Updated the calling API part. After calling the API opening the url i…

c996857

…n browser.

prajin-29 had a problem deploying to account-admin December 28, 2023 11:43 — with GitHub Actions Failure

Updated the URL.

ff1e5bc

prajin-29 had a problem deploying to account-admin December 28, 2023 12:12 — with GitHub Actions Failure

Updated the if else logic.

b48b46c

prajin-29 temporarily deployed to account-admin December 28, 2023 14:49 — with GitHub Actions Inactive

Updated the if else logic.

5bc0dc8

prajin-29 had a problem deploying to account-admin December 28, 2023 14:51 — with GitHub Actions Failure

prajin-29 temporarily deployed to account-admin December 28, 2023 15:35 — with GitHub Actions Inactive

nfx approved these changes Dec 28, 2023

View reviewed changes

nfx merged commit 5ff8bc1 into main Dec 28, 2023

nfx deleted the feature/retry_run_logic_cli branch December 28, 2023 16:02

nfx mentioned this pull request Dec 28, 2023

Release v0.8.0 #744

Merged

HariGS-DB pushed a commit that referenced this pull request Jan 2, 2024

Added databricks labs ucx repair-run --step ... CLI command for rep…

97e94bd

…air run of any failed workflows, like `assessment`, `migrate-groups` etc. (#724)

FastLee pushed a commit that referenced this pull request Jan 19, 2024

Added databricks labs ucx repair-run --step ... CLI command for rep…

371e20b

…air run of any failed workflows, like `assessment`, `migrate-groups` etc. (#724)

Conversation

prajin-29 commented Dec 22, 2023

Uh oh!

codecov bot commented Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nfx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfx commented Dec 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfx commented Dec 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prajin-29 commented Dec 28, 2023

Uh oh!

nfx left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Dec 22, 2023 •

edited

Loading