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

while applying policies, wait for all expected policy revisions. #1706

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

tommyp1ckles
Copy link
Contributor

Previously, we always waited for the r+1 revision, which in the case of multiple policies being applied may result in a race condition where some, but not all, of the policies are in place prior to proceeding with tests.

@tommyp1ckles tommyp1ckles temporarily deployed to ci June 7, 2023 20:27 — with GitHub Actions Inactive
@tommyp1ckles tommyp1ckles requested review from a team and nathanjsweet and removed request for a team June 7, 2023 20:30
@tommyp1ckles
Copy link
Contributor Author

@cilium/sig-policy @nathanjsweet Can you confirm that every new/deleted policy will result in a separate revision (i.e. will not ever be batched up into a single revision)?

@joestringer
Copy link
Member

Today, it will bump by at least one revision per resource update. I wouldn't consider that as API but it's unlikely to change any time particularly soon.

I'd prefer a more formal way to associate the target revision with the target resource update like we used to have on the direct agent API, but that's probably quite a bit more work. Unclear whether that's worthwhile at this stage.

@tommyp1ckles tommyp1ckles temporarily deployed to ci June 9, 2023 04:44 — with GitHub Actions Inactive
@tommyp1ckles tommyp1ckles temporarily deployed to ci June 9, 2023 17:00 — with GitHub Actions Inactive
Previously, we always waited for the r+1 revision, which in the case of
multiple policies being applied may result in a race condition where
some, but not all, of the policies are in place prior to proceeding with
tests.

As well, policy revision deltas are now counted per cluster, thus we can
wait for the correct revision for each cluster.

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
@tommyp1ckles tommyp1ckles temporarily deployed to ci June 9, 2023 17:40 — with GitHub Actions Inactive
@tommyp1ckles tommyp1ckles marked this pull request as ready for review June 9, 2023 17:41
@tommyp1ckles tommyp1ckles requested a review from a team as a code owner June 9, 2023 17:41
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 20, 2023
@ti-mo ti-mo merged commit 0d5a34d into main Jun 21, 2023
19 checks passed
@ti-mo ti-mo deleted the pr/tp/wait-for-all-revisions branch June 21, 2023 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants