implement _safe_* calls for applier_task in redash, scim, generic by tnguyen-db · Pull Request #507 · databrickslabs/ucx

tnguyen-db · 2023-10-26T07:11:21Z

Resolves Add retry to redash permissions #489
Removes for loop in applier_task in redash, scim, generic
Sets the right rate for each
Implements safe* calls

… and scim

codecov · 2023-10-26T08:01:13Z

Codecov Report

Merging #507 (06d311d) into main (70aa0f5) will increase coverage by 0.51%.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #507      +/-   ##
==========================================
+ Coverage   80.30%   80.81%   +0.51%     
==========================================
  Files          31       31              
  Lines        3219     3284      +65     
  Branches      617      621       +4     
==========================================
+ Hits         2585     2654      +69     
+ Misses        489      488       -1     
+ Partials      145      142       -3

Files	Coverage Δ
...rc/databricks/labs/ucx/workspace_access/generic.py	`92.41% <100.00%> (+1.58%)`	⬆️
src/databricks/labs/ucx/workspace_access/redash.py	`87.82% <100.00%> (+2.56%)`	⬆️
src/databricks/labs/ucx/workspace_access/scim.py	`100.00% <100.00%> (+3.57%)`	⬆️

... and 1 file with indirect coverage changes

larsgeorge-db

Please explain the change to me. I do not understand how the loop and the "retries on errors" achieve the same. More inline...

src/databricks/labs/ucx/workspace_access/generic.py

larsgeorge-db · 2023-10-26T08:06:55Z

src/databricks/labs/ucx/workspace_access/generic.py

                raise RetryableError() from e

+    @retried(on=[RetryableError])
+    def _safe_update_permissions(


This handles errors now, but how does this ensure all permissions have been applied successfully?

If no error is returned, can we assume the permissions have been applied?

Apparently not? Please consult with @renardeinside, as there are seemingly cases where the function returns without error, but the permission is not available? I am just making the point that before it looped over presence of permissions, not that it runs without exceptions.

Yes, indeed, there is now _inflight_check to check if the permissions have been applied as expected in _applier_task for each type

In general, when the API returns 200 and no error the permission should be applied. If it's not then it should be reported as a platform bug. What are the scenarios we try to cover here?

Apparently, the API can be inconsistent, I don't know if the problem has been raised already to the platform

Got it, we should fix that but this should also be reported to the platform.

larsgeorge-db · 2023-10-26T08:08:16Z

Also, this adds a bunch of new code paths that the test now covers less. Please add more coverage in the tests.

nfx

This PR introduces regressions, please address all the comments

src/databricks/labs/ucx/workspace_access/generic.py

nfx · 2023-10-26T08:31:09Z

src/databricks/labs/ucx/workspace_access/generic.py

            else:
                raise RetryableError() from e

+    @retried(on=[RetryableError])


No timeout configured. See how it's configurable for unit testing in other places

Which timeout value do you suggest?
I haven't found examples on how to configure it in the unit tests.
I was rather thinking of mocking @retried in the unit tests, wdyt?

@tnguyen-db did you look at this one?

ucx/src/databricks/labs/ucx/workspace_access/secrets.py

Lines 92 to 97 in bc7f29e

@rate_limited(max_requests=30)

def _rate_limited_put_acl(self, object_id: str, principal: str, permission: workspace.AclPermission):

self._ws.secrets.put_acl(object_id, principal, permission)

retry_on_value_error = retried(on=[ValueError], timeout=self._verify_timeout)

retried_check = retry_on_value_error(self._inflight_check)

retried_check(object_id, principal, permission)

tnguyen-db · 2023-10-26T09:42:57Z

@larsgeorge-db
The main problem this PR was trying to solve was to introduce retries for the dash permissions.
self._ws.dbsql_permissions.set only had the 3 attempts in the loop which did not take into consideration the types of errors.
The solution attempts to have a better control on the retry mechanism by lowering them at the level of the API call.

I've then generalized the solution for generic and scim

tnguyen-db · 2023-10-26T13:36:11Z

src/databricks/labs/ucx/workspace_access/redash.py

+        set_retry_on_value_error = retried(on=[RetryableError], timeout=self._verify_timeout)
+        set_retried_check = set_retry_on_value_error(self._safe_set_permissions)
+        set_retried_check(object_type, object_id, acl)
+
+        retry_on_value_error = retried(on=[ValueError], timeout=self._verify_timeout)
+        retried_check = retry_on_value_error(self._inflight_check)
+        return retried_check(object_id, object_id, acl)


@nfx is it the kind of logic you expect?

tnguyen-db · 2023-10-26T13:37:01Z

tests/unit/workspace_access/test_redash.py

+    sup = RedashPermissionsSupport(ws=ws, listings=[], verify_timeout=timedelta(seconds=1))
+    with pytest.raises(TimeoutError) as e:
+        result = sup._applier_task(
+            sql.ObjectTypePlural.QUERIES,
+            "test",
+            [sql.AccessControl(group_name="group_1", permission_level=sql.PermissionLevel.CAN_RUN)],
+        )
+    assert "Timed out after" in str(e.value)
+


@nfx same question as above but for the test

src/databricks/labs/ucx/workspace_access/generic.py

nfx

lgtm

…the workspace (#519) Duplicated from: #507

nfx · 2023-10-30T19:58:08Z

Closing this as merged

tnguyen-db added 4 commits October 25, 2023 12:05

create _safe_update_permission to move down the retry in _applier_task

5e182e9

fix unit tests for _applier_task

22d171f

implement _safe_* functions for api calls in _applier_task for redash…

0832cdb

… and scim

remove INTERNAL_SERVER_ERROR from the retry exclusion

1f8896a

tnguyen-db requested a review from a team October 26, 2023 07:11

redefine unit tests

c2acce5

tnguyen-db force-pushed the feature/retry-redash-permissions branch from 5c88480 to c2acce5 Compare October 26, 2023 07:14

make fmt, make lint

c1c507a

larsgeorge-db reviewed Oct 26, 2023

View reviewed changes

nfx requested changes Oct 26, 2023

View reviewed changes

tnguyen-db commented Oct 26, 2023

View reviewed changes

tnguyen-db force-pushed the feature/retry-redash-permissions branch from af4e3f1 to 80824c0 Compare October 26, 2023 13:48

modify retried logic

7a5711e

tnguyen-db force-pushed the feature/retry-redash-permissions branch from 80824c0 to 7a5711e Compare October 26, 2023 13:50

tnguyen-db added 4 commits October 26, 2023 16:30

add retry for _ws.dbsql_permissions.get

4822b24

refactor retry logic in generic.py + unit tests

81c9138

refactor retry logic in scim.py + unit tests

71a044c

typos

674eeaf

nfx requested changes Oct 26, 2023

View reviewed changes

tnguyen-db added 3 commits October 27, 2023 09:21

change default timeout in generic, redash, scim

c4e8a5b

make RetryableError raise more informative

c4ba6c3

remove retry inside _inflight_check + unit tests

06d311d

nfx approved these changes Oct 27, 2023

View reviewed changes

tnguyen-db had a problem deploying to account-admin October 27, 2023 18:28 — with GitHub Actions Failure

tnguyen-db mentioned this pull request Oct 27, 2023

Retry set/update permissions when possible and assess the changes in the workspace #519

Merged

tnguyen-db had a problem deploying to account-admin October 27, 2023 19:12 — with GitHub Actions Failure

tnguyen-db had a problem deploying to account-admin October 28, 2023 12:02 — with GitHub Actions Failure

tnguyen-db had a problem deploying to account-admin October 28, 2023 12:58 — with GitHub Actions Failure

tnguyen-db had a problem deploying to account-admin October 30, 2023 07:55 — with GitHub Actions Error

fix typo in parameter in _inflight_check call

79f7134

tnguyen-db force-pushed the feature/retry-redash-permissions branch from 06d311d to 4924012 Compare October 30, 2023 09:54

tnguyen-db had a problem deploying to account-admin October 30, 2023 09:54 — with GitHub Actions Failure

tnguyen-db had a problem deploying to account-admin October 30, 2023 10:21 — with GitHub Actions Failure

tnguyen-db had a problem deploying to account-admin October 30, 2023 10:58 — with GitHub Actions Failure

tnguyen-db force-pushed the feature/retry-redash-permissions branch from 4924012 to 79f7134 Compare October 30, 2023 13:21

tnguyen-db temporarily deployed to account-admin October 30, 2023 13:22 — with GitHub Actions Inactive

nfx pushed a commit that referenced this pull request Oct 30, 2023

Retry set/update permissions when possible and assess the changes in …

142cf75

…the workspace (#519) Duplicated from: #507

nfx closed this Oct 30, 2023

tnguyen-db had a problem deploying to account-admin November 29, 2023 13:21 — with GitHub Actions Failure

	@rate_limited(max_requests=30)
	def _rate_limited_put_acl(self, object_id: str, principal: str, permission: workspace.AclPermission):
	self._ws.secrets.put_acl(object_id, principal, permission)
	retry_on_value_error = retried(on=[ValueError], timeout=self._verify_timeout)
	retried_check = retry_on_value_error(self._inflight_check)
	retried_check(object_id, principal, permission)

Conversation

tnguyen-db commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

larsgeorge-db left a comment

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!

Choose a reason for hiding this comment

Uh oh!

mwojtyczka Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

larsgeorge-db commented Oct 26, 2023

Uh oh!

nfx left a comment

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!

tnguyen-db commented Oct 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nfx left a comment

Choose a reason for hiding this comment

Uh oh!

nfx commented Oct 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tnguyen-db commented Oct 26, 2023 •

edited

Loading

codecov bot commented Oct 26, 2023 •

edited

Loading

mwojtyczka Oct 27, 2023 •

edited

Loading