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

Make integration.WaitForPodsReady use pod Ready condition #5308

Merged
merged 5 commits into from
Feb 3, 2021

Conversation

briandealwis
Copy link
Member

@briandealwis briandealwis commented Jan 27, 2021

Towards #5249
Merge after: #5307 because debug tests will fail due to dlv/go1.12 breakage #5300

Description
WaitForPodsReady uses WaitForPodsInPhase to check that the pods' phases are Running. But this does not account for readiness probes. It also turns out that our waitForDebugEvents() calls WaitForPodsReady() with no pod names, expecting it to wait for all pods to be ready.

This PR splits out WaitForPodsReady() into a utility waitForPods() that takes a predicate and waits for pods to satisfy that predicate. WaitForPodsReady() calls into waitForPods() with a predicate that waits for the pod's Ready condition to become true. WaitForPodsInPhase() calls into waitForPods() with predicate that checks the pod's phase. waitForPods() takes a list of pod names to monitor, and if empty then it waits for all pods (in that namespace).

@briandealwis briandealwis requested a review from a team as a code owner January 27, 2021 17:51
@google-cla google-cla bot added the cla: yes label Jan 27, 2021
@codecov
Copy link

codecov bot commented Jan 27, 2021

Codecov Report

Merging #5308 (a31b0a9) into master (1da1bed) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5308      +/-   ##
==========================================
- Coverage   71.82%   71.81%   -0.01%     
==========================================
  Files         392      392              
  Lines       14215    14219       +4     
==========================================
+ Hits        10210    10212       +2     
- Misses       3255     3257       +2     
  Partials      750      750              
Impacted Files Coverage Δ
pkg/skaffold/instrumentation/meter.go 50.00% <0.00%> (-4.17%) ⬇️
pkg/skaffold/instrumentation/types.go 0.00% <0.00%> (ø)
pkg/skaffold/instrumentation/export.go 74.57% <0.00%> (+0.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1da1bed...a31b0a9. Read the comment docs.

@briandealwis
Copy link
Member Author

Tests are failing as expected since #5307 isn't committed \o/

Comment on lines +242 to 243
var waiting []string
for _, podName := range podNames {
Copy link
Contributor

Choose a reason for hiding this comment

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

would this cause any issues when waitForAllPods is true? I'm just thinking that maybe a pod goes through all the necessary phases before any other and so it ends up being the only one in podNames resulting in waiting being empty despite there potentially being more pods who haven't reached a phase yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is true: there is a race condition where pod creation events are pending in the watcher. I guess I could check len(w.ResultChan()) to see if there are other events pending.

Copy link
Member Author

Choose a reason for hiding this comment

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

I hope these pods will eventually reach a fixpoint :-/ I suppose we do have a timeout. I print out how many pending messages there are.

@briandealwis
Copy link
Member Author

(note: tests should fail as still haven't merged with HEAD to pick up #5307)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants