Fix for Flaky test TestServiceWithPredefinedNetwork by selansen · Pull Request #36551 · moby/moby

selansen · 2018-03-09T22:14:05Z

TestServiceWithPredefinedNetwork test case was failing
at times. To fix the issue, added new API to check
for services after we clean up all services. Tested
multiple times and this sould fix flaky issue.

Signed-off-by: selansen elango.siva@docker.com

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

dnephin

There seems to be a couple problems with this test.

It's not actually checking anything about the service, just that it exists. Shouldn't it be comparing the inspect result to the expected?
It shouldn't be doing cleanup. That is handled by setupTest(t)().

What is the purpose of this test? What is the host network a special case? That is what needs to be checked.

dnephin · 2018-03-09T22:24:48Z

integration/network/service_test.go

Removing this is not the correct fix. It should be used as a filter in ServiceList.

The actual regression issue was we couldnt create service with host network. Thats what I tried to test it to make sure there is no issue in creating service with host network.

For your clean up comment : I try to remove service and make sure service is getting cleared. Thats what I meant by cleanup. I am not doing anything extra. Do you suggest I dont need to do verify the service that I created whether I am able to remove or not?. Pls let me know your feedback on this.
3)Regarding not adding filter: I am trying to get all the services running. so looked into existing examples of functions calling serviceList API. none of them using filter. Your previous comment on original comment said , I have defined filter but not using it. So i wanted to clean up the code as part of your previous comment. Pls correct me if my understand is wrong.

I try to remove service and make sure service is getting cleared. Thats what I meant by cleanup. I am not doing anything extra. Do you suggest I dont need to do verify the service that I created whether I am able to remove or not?. Pls let me know your feedback on this.

That's right. The daemon which runs this service is going to be shutdown when the test exits (defer d.Stop(t)) so you don't need to spend time doing cleanup.

Regarding not adding filter: I am trying to get all the services running.

Ok, I guess this is ok since you're targeting a daemon that was launched for this test.

thaJeztah · 2018-03-14T19:07:44Z

@selansen this needs a minor rebase

selansen · 2018-03-14T19:53:28Z

should I pull latest moby and apply my change on top of it and submit again ?

…

On Wed, Mar 14, 2018 at 3:09 PM, Sebastiaan van Stijn < ***@***.***> wrote: @selansen <https://github.com/selansen> this needs a minor rebase — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36551 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKEKn0xiI-xgk123GyuyL1U_e-zfgZ2Fks5teWrZgaJpZM4Sk8aV> .

codecov · 2018-03-14T22:35:08Z

Codecov Report

❗ No coverage uploaded for pull request base (master@4460472). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #36551   +/-   ##
=========================================
  Coverage          ?   34.76%           
=========================================
  Files             ?      612           
  Lines             ?    45577           
  Branches          ?        0           
=========================================
  Hits              ?    15844           
  Misses            ?    27663           
  Partials          ?     2070

arm64b · 2018-03-15T03:03:36Z

should I pull latest moby and apply my change on top of it and submit again?

You should fetch the latest code then make your topic branch rebase to the master.

selansen · 2018-03-15T03:19:16Z

Thats what I did and committed my changes. Looks like all sanity passed.
@thaJeztah , pls let me know if I will have to do anything else.

arm64b · 2018-03-15T03:28:34Z

Seems the symptom of TestServiceWithIngressNetwork is very similar as this one...

selansen · 2018-03-15T03:39:15Z

Yes. taken care of that too

selansen · 2018-03-15T17:19:35Z

@thaJeztah , Hope this helps flaky issue that we are seeing on selective target.

thaJeztah · 2018-03-16T12:05:42Z

~~I think for now this looks ok~~ (not entirely, see below), but we need to implement #36581, so that we don't wait for all services to be cleaned up (in case we want the option of running tests in parallel)

thaJeztah

noticed some things; @selansen can you check?

thaJeztah · 2018-03-16T12:08:21Z

integration/network/service_test.go

Erm, this function doesn't seem to be called?

I kept the new API so that TestServiceWithIngressNetwork will call it. Let me update it.

thaJeztah · 2018-03-16T12:10:11Z

integration/network/service_test.go

If we don't check if the service (plus tasks?) are all removed here, this test may not be testing what it was created for (i.e.: check that the ingress network is not removed when removing services)

Idea was to check service is removed instead task as we are testing service create/remove. Thats why I added new API. Let me update TestServiceWithIngressNetwork. Thanks for pointing it out.

BTW actual test that was written by Chris Telfer was to verify Ingress network absence after service remove. So test will fail if it is not met. Daniel mentioned service clean up will happen automatically when we tear down the swarm . so we dont need to check this explicitly. Anyways I update the with new API that TestServiceWithIngressNetwork with my newly added noServices check to make sure we cover that .

vdemeester

LGTM 🐯

selansen · 2018-03-16T19:39:10Z

Failed test cases are not related to my test cases.

arm64b · 2018-03-19T05:35:43Z

Seems DockerSuite.TestPostContainersAttach test case failed is introduced recently, don't know if @thaJeztah concern has been addressed or not, if it's true, we can try to rebuild the 2 failure nodes

thaJeztah

LGTM, but needs a rebase (looks like because of the gotestyourself changes)

selansen · 2018-03-21T15:13:05Z

@thaJeztah , I just did a rebase. should be good to go.

TestServiceWithPredefinedNetwork test case was failing at times. To fix the issue, added new API to check for services after we clean up all services. Tested multiple times and this sould fix flaky issue. Signed-off-by: selansen <elango.siva@docker.com>

thaJeztah · 2018-03-21T16:47:07Z

Thanks! moved it to "status/merge" - fingers crossed if it all goes green 👍

thaJeztah · 2018-03-21T18:07:26Z

Hm, more flakiness?

16:53:08 ----------------------------------------------------------------------
16:53:08 FAIL: docker_api_swarm_service_test.go:61: DockerSwarmSuite.TestAPISwarmServicesCreate
16:53:08 
16:53:08 [dee4f3a4df82c] waiting for daemon to start
16:53:08 [dee4f3a4df82c] daemon started
16:53:08 
16:53:08 docker_api_swarm_service_test.go:92:
16:53:08     waitAndAssert(c, defaultReconciliationTimeout, d.CheckActiveContainerCount, checker.Equals, 0)
16:53:08 docker_utils_test.go:452:
16:53:08     c.Assert(v, checker, args...)
16:53:08 ... obtained int = 3
16:53:08 ... expected int = 0
16:53:08 ... output: "dec8242edf45\na6b9f3e67481\nb05bf8c0000a\n"
16:53:08 
16:53:08 [dee4f3a4df82c] exiting daemon

thaJeztah · 2018-03-21T18:08:46Z

These tests pass now though;

16:17:58 === RUN   TestServiceWithPredefinedNetwork
16:18:00 --- PASS: TestServiceWithPredefinedNetwork (1.91s)
16:18:00 	daemon.go:283: [d081ae40fa771] waiting for daemon to start
16:18:00 	daemon.go:315: [d081ae40fa771] daemon started
16:18:00 	daemon.go:273: [d081ae40fa771] exiting daemon
16:18:00 === RUN   TestServiceWithIngressNetwork
16:18:11 --- PASS: TestServiceWithIngressNetwork (11.51s)
16:18:11 	daemon.go:283: [de1f27941839a] waiting for daemon to start
16:18:11 	daemon.go:315: [de1f27941839a] daemon started
16:18:11 	daemon.go:273: [de1f27941839a] exiting daemon
16:18:11 PASS

selansen · 2018-03-21T21:33:46Z

Not sure about docker_api_swarm_service_test. They are not related to the fix I did. May be we need to take it as separate issue.

arm64b · 2018-03-22T03:22:18Z

docker_api_swarm_service_test.go failure can be tracked by #36501 and #36386.

selansen requested review from dnephin and vdemeester as code owners March 9, 2018 22:14

GordonTheTurtle added the status/0-triage label Mar 9, 2018

dnephin reviewed Mar 9, 2018

View reviewed changes

vdemeester added status/2-code-review and removed status/0-triage labels Mar 12, 2018

thaJeztah added the rebuild/* label Mar 14, 2018

GordonTheTurtle removed the rebuild/* label Mar 14, 2018

thaJeztah added the rebuild/* label Mar 14, 2018

GordonTheTurtle removed the rebuild/* label Mar 14, 2018

selansen force-pushed the 36247 branch from d54eee9 to 422ea0a Compare March 14, 2018 22:35

selansen force-pushed the 36247 branch from 422ea0a to 55bc027 Compare March 14, 2018 22:50

thaJeztah mentioned this pull request Mar 14, 2018

ExportContainer: do not panic #36586

Merged

thaJeztah added the rebuild/janky label Mar 15, 2018

GordonTheTurtle removed the rebuild/janky label Mar 15, 2018

thaJeztah added area/networking Networking area/testing labels Mar 15, 2018

thaJeztah mentioned this pull request Mar 15, 2018

Update libnetwork to fix stale HNS endpoints on Windows #36603

Merged

kolyshkin mentioned this pull request Mar 16, 2018

Flaky test TestServiceWithPredefinedNetwork #36547

Closed

thaJeztah mentioned this pull request Mar 16, 2018

add support for compressibility of log file #29932

Merged

thaJeztah requested changes Mar 16, 2018

View reviewed changes

vdemeester approved these changes Mar 16, 2018

View reviewed changes

selansen force-pushed the 36247 branch from 55bc027 to 5b3af39 Compare March 16, 2018 14:10

thaJeztah added the rebuild/* label Mar 21, 2018

GordonTheTurtle removed the rebuild/* label Mar 21, 2018

thaJeztah approved these changes Mar 21, 2018

View reviewed changes

selansen force-pushed the 36247 branch from 5b3af39 to 99bdd4a Compare March 21, 2018 15:10

selansen force-pushed the 36247 branch from 99bdd4a to dabffd8 Compare March 21, 2018 15:49

thaJeztah added status/4-merge and removed status/2-code-review labels Mar 21, 2018

thaJeztah added the rebuild/experimental label Mar 21, 2018

GordonTheTurtle removed the rebuild/experimental label Mar 21, 2018

thaJeztah added the rebuild/experimental label Mar 21, 2018

GordonTheTurtle removed the rebuild/experimental label Mar 21, 2018

vdemeester merged commit cd01c14 into moby:master Mar 22, 2018

thaJeztah mentioned this pull request Apr 18, 2018

[18.03] Updates to integration tests, migrate to "gotestyourself" docker-archive/docker-ce#513

Merged

anusha-ragunathan mentioned this pull request May 17, 2018

Some slight tweaks for the integration test #37086

Merged

thaJeztah mentioned this pull request Aug 31, 2018

[epic] flaky tests #37306

Open

thaJeztah mentioned this pull request Jul 2, 2019

Flaky test: TestServiceRemoveKeepsIngressNetwork #39426

Open

Conversation

selansen commented Mar 9, 2018 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin left a comment

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!

thaJeztah commented Mar 14, 2018

Uh oh!

selansen commented Mar 14, 2018 via email

Uh oh!

codecov bot commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arm64b commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

selansen commented Mar 15, 2018

Uh oh!

arm64b commented Mar 15, 2018

Uh oh!

selansen commented Mar 15, 2018

Uh oh!

selansen commented Mar 15, 2018

Uh oh!

thaJeztah commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

selansen commented Mar 16, 2018

Uh oh!

arm64b commented Mar 19, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

selansen commented Mar 21, 2018

Uh oh!

thaJeztah commented Mar 21, 2018

Uh oh!

thaJeztah commented Mar 21, 2018

Uh oh!

thaJeztah commented Mar 21, 2018

Uh oh!

selansen commented Mar 21, 2018

Uh oh!

arm64b commented Mar 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

selansen commented Mar 9, 2018 •

edited by thaJeztah

Loading

codecov bot commented Mar 14, 2018 •

edited

Loading

arm64b commented Mar 15, 2018 •

edited

Loading

thaJeztah commented Mar 16, 2018 •

edited

Loading