Skip to content

Avoid predefined error log#2561

Merged
cyli merged 1 commit into
moby:masterfrom
fcrisciani:fix_misc
Apr 4, 2018
Merged

Avoid predefined error log#2561
cyli merged 1 commit into
moby:masterfrom
fcrisciani:fix_misc

Conversation

@fcrisciani
Copy link
Copy Markdown

  • fix the logic that was checking for predefined networks. Before the
    GetNetwork was passing the network name, while instead the network ID is
    the key that is supposed to be used. The CreateNetwork anyway is doing
    the very same check and create the network if not existing.

Signed-off-by: Flavio Crisciani flavio.crisciani@docker.com

- fix the logic that was checking for predefined networks. Before the
GetNetwork was passing the network name, while instead the network ID is
the key that is supposed to be used. The CreateNetwork anyway is doing
the very same check and create the network if not existing.

Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
@fcrisciani
Copy link
Copy Markdown
Author

cc @abhi @aluzzardi

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 16, 2018

Codecov Report

Merging #2561 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2561      +/-   ##
==========================================
+ Coverage   61.35%   61.42%   +0.06%     
==========================================
  Files         133      133              
  Lines       21765    21764       -1     
==========================================
+ Hits        13354    13368      +14     
+ Misses       6974     6972       -2     
+ Partials     1437     1424      -13

Copy link
Copy Markdown
Contributor

@cyli cyli left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@abhi abhi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@anshulpundir anshulpundir left a comment

Choose a reason for hiding this comment

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

Can you please add a unit-test ? @fcrisciani

@fcrisciani
Copy link
Copy Markdown
Author

@anshulpundir which unit test do you want to put? there is no behavior change the previous call was passing the wrong parameter

@cyli
Copy link
Copy Markdown
Contributor

cyli commented Mar 16, 2018

@fcrisciani I don't think there was previously a test for this behavior. Perhaps a test in manager_test.go to assert that the correct networks are created on a fresh manager, and that if those networks are deleted, then on restart of the manager they are created again?

@fcrisciani
Copy link
Copy Markdown
Author

@cyli the network were already created correctly because the get was returning nil. The problem here was that after the second election the get will return nil but the create will fail because the predefined network is already there. As I was saying there is not change in behavior, just removing a warning happening on any subsequent leader election

@cyli
Copy link
Copy Markdown
Contributor

cyli commented Mar 16, 2018

@fcrisciani I agree. I think @anshulpundir was just suggesting, and I was agreeing, that it'd be nice to have a unit test for the network create at all (there is not currently any test for the networks being created), not that we think your change caused any change in behavior. It'd be nice to have a test to verify that the behavior was the same before and after this change is all. :)

@fcrisciani
Copy link
Copy Markdown
Author

@anshulpundir @cyli this PR #2571 added the network restore tests

@cyli
Copy link
Copy Markdown
Contributor

cyli commented Apr 4, 2018

Thanks @fcrisciani!

@cyli cyli merged commit c0d88ac into moby:master Apr 4, 2018
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Apr 17, 2018
Relevant changes:

- moby/swarmkit#2551 RoleManager will remove deleted nodes from the cluster membership
- moby/swarmkit#2574 Scheduler/TaskReaper: handle unassigned tasks marked for shutdown
- moby/swarmkit#2561 Avoid predefined error log
- moby/swarmkit#2557 Task reaper should delete tasks with removed slots that were not yet assigned
- moby/swarmkit#2587 [fips] Agent reports FIPS status
- moby/swarmkit#2603 Fix manager/state/store.timedMutex

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Apr 18, 2018
Relevant changes:

- moby/swarmkit#2551 RoleManager will remove deleted nodes from the cluster membership
- moby/swarmkit#2574 Scheduler/TaskReaper: handle unassigned tasks marked for shutdown
- moby/swarmkit#2561 Avoid predefined error log
- moby/swarmkit#2557 Task reaper should delete tasks with removed slots that were not yet assigned
- moby/swarmkit#2587 [fips] Agent reports FIPS status
- moby/swarmkit#2603 Fix manager/state/store.timedMutex

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 333b2f28fef4ba857905e7263e7b9bbbf7c522fc
Component: engine
thaJeztah added a commit to thaJeztah/docker-ce that referenced this pull request Apr 19, 2018
Relevant changes:

- moby/swarmkit#2551 RoleManager will remove deleted nodes from the cluster membership
- moby/swarmkit#2574 Scheduler/TaskReaper: handle unassigned tasks marked for shutdown
- moby/swarmkit#2561 Avoid predefined error log
- moby/swarmkit#2557 Task reaper should delete tasks with removed slots that were not yet assigned
- moby/swarmkit#2587 [fips] Agent reports FIPS status
- moby/swarmkit#2603 Fix manager/state/store.timedMutex

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 333b2f28fef4ba857905e7263e7b9bbbf7c522fc)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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.

4 participants