-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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 flaky TestServiceWithDefaultAddressPoolInit #39671
Conversation
7146630
to
c186d46
Compare
608d40e
to
5d50ffa
Compare
34b90f4
to
fafea21
Compare
@thaJeztah any idea why the test case is failing ? |
fafea21
to
53b8069
Compare
and changing the order of the operation breaks the next test |
is there a manager that failed to stop perhaps?
|
53b8069
to
c763936
Compare
Means someone is already listening on this port. Not sure what it means in this exact context. |
177a832
to
c76e7bb
Compare
2549161
to
952d643
Compare
Derek add label: rebuild/windowsRS1 |
Derek add label: rebuild/windowsRS1 |
1.This commit replaces serviceRunningCount with swarm.RunningTasksCount to accurately check if the service is running with the accurate number of instances or not. serviceRunningCount was only checking the ServiceList and was not checking if the tasks were running or not This adds a safe barrier to execute docker network inspect commands for overlay networks which get created asynchronously via Swarm 2. Make sure client connections are closed 3. Make sure every service and network name is unique 4. Make sure services and networks are cleaned up Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
952d643
to
f3a3ea0
Compare
Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
PTAL @kolyshkin |
@tiborvass PTAL |
ping @tiborvass @kolyshkin PTAL 🤗 |
poll.WaitOn(t, swarm.NoTasks(ctx, c), swarm.ServicePoll) | ||
err = c.NetworkRemove(ctx, overlayID) | ||
assert.NilError(t, err) | ||
c.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see one potential problem with all the assert
statements here. Once any assert
fails, the text execution is cancelled, meaning the above cleanup code (remove/close/etc) won't be run.
Perhaps we need to change those assert.
calls to check.
ones. The difference is check.
won't abort test execution immediately (but still mark the test as failed).
If it's not possible to use check.
everywhere, maybe we need to do cleanup in a defer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but AFAIK the RootDir
will be cleaned up so it won't affect another PR, as for the current PR, this test failed so the reason for the assert failure should be addressed before moving to the next test
fixes #38514 Flaky test: TestServiceWithDefaultAddressPoolInit
This commit replaces serviceRunningCount with
swarm.RunningTasksCount to accurately check if the
service is running with the accurate number of instances
or not. serviceRunningCount was only checking the ServiceList
and was not checking if the tasks were running or not
This adds a safe barrier to execute docker network inspect
commands for overlay networks which get created
asynchronously via swarmkit
Signed-off-by: Arko Dasgupta arko.dasgupta@docker.com