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

Fix race conditions in empty cluster K8s tests. #1517

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

SanjayVas
Copy link
Member

@SanjayVas SanjayVas commented Mar 7, 2024

  • List Pods using current ReplicaSet rather than Deployment match labels.
  • Wait for all Deployments to be complete after apply operations.

@SanjayVas SanjayVas requested a review from Marco-Premier March 7, 2024 21:09
@wfa-reviewable
Copy link

This change is Reviewable

@SanjayVas SanjayVas force-pushed the sanjayvas-panelmatch-wait branch from 22fa87d to 33ff31e Compare March 7, 2024 23:25
@SanjayVas SanjayVas marked this pull request as draft March 7, 2024 23:52
Copy link
Member Author

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @Marco-Premier and @SanjayVas)

a discussion (no related file):
Still hitting a race condition. Will update this comment when I figure it out


Copy link
Contributor

@Marco-Premier Marco-Premier left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)

Copy link
Contributor

@Marco-Premier Marco-Premier left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)

a discussion (no related file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Still hitting a race condition. Will update this comment when I figure it out

I noticed that waitUntilDeploymentsReady returns if the old deployment was deployed already, without waiting for the new apply to take effect.
Maybe we can check if the pod name changes? Or simply set a time out even though it would be "flaky" and not deterministic


Copy link
Member Author

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)

a discussion (no related file):

I noticed that waitUntilDeploymentsReady returns if the old deployment was deployed already

IIUC, the Deployment doesn't get replaced. It should be the same deployment, just with different state.


@SanjayVas SanjayVas force-pushed the sanjayvas-panelmatch-wait branch 2 times, most recently from f8918a7 to a813002 Compare March 8, 2024 23:26
@SanjayVas SanjayVas marked this pull request as ready for review March 8, 2024 23:26
@SanjayVas SanjayVas requested a review from Marco-Premier March 8, 2024 23:27
Copy link
Member Author

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Depends on world-federation-of-advertisers/common-jvm#237

Reviewable status: 3 of 11 files reviewed, all discussions resolved (waiting on @Marco-Premier)

@SanjayVas SanjayVas force-pushed the sanjayvas-panelmatch-wait branch 2 times, most recently from 69baa4c to b426714 Compare March 9, 2024 01:58
@SanjayVas SanjayVas changed the title Wait for Kingdom restart in EmptyClusterPanelMatchCorrectnessTest. Fix race conditions in empty cluster K8s tests. Mar 9, 2024
Copy link
Contributor

@Marco-Premier Marco-Premier left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

This fixes a race condition where the port forwarding may be pointing to the Kingdom pod used for resource setup.
@SanjayVas SanjayVas force-pushed the sanjayvas-panelmatch-wait branch from b426714 to cf321bf Compare March 11, 2024 16:52
Copy link
Member Author

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

@SanjayVas SanjayVas enabled auto-merge (squash) March 11, 2024 16:52
@SanjayVas SanjayVas merged commit 392833d into main Mar 11, 2024
4 checks passed
@SanjayVas SanjayVas deleted the sanjayvas-panelmatch-wait branch March 11, 2024 17:34
ple13 pushed a commit that referenced this pull request Aug 16, 2024
* List Pods using current ReplicaSet rather than Deployment match labels.
* Wait for all Deployments to be complete after apply operations.
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