Skip to content

Pass in network IDs during restart#38580

Merged
thaJeztah merged 1 commit intomoby:masterfrom
andrey-ko:fix-restart
Feb 7, 2019
Merged

Pass in network IDs during restart#38580
thaJeztah merged 1 commit intomoby:masterfrom
andrey-ko:fix-restart

Conversation

@andrey-ko
Copy link
Copy Markdown
Contributor

@andrey-ko andrey-ko commented Jan 16, 2019 •
edited
Loading

for windows all networks are re-populated in the store during network controller initialization. In current version it also regenerate network Ids which may be referenced by other components and it may cause broken references to networks. This commit avoids regeneration of network ids.

@andrey-ko andrey-ko changed the title fix windows restart (https://github.com/docker/escalation/issues/1037) Pass in network IDs during restart Jan 16, 2019
@ddebroy
Copy link
Copy Markdown
Contributor

ddebroy commented Jan 16, 2019

@fcrisciani PTAL

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jan 16, 2019

Could you provide a description of the change (also in the commit message)?

fcrisciani
fcrisciani reviewed Jan 16, 2019
View reviewed changes
Comment thread daemon/daemon.go Outdated
fcrisciani
fcrisciani approved these changes Jan 16, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@fcrisciani fcrisciani left a comment

Choose a reason for hiding this comment

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

Add commit message with some details on the change, but LGTM

selansen
selansen approved these changes Jan 16, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@selansen selansen left a comment

Choose a reason for hiding this comment

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

LGTM.
Like @thaJeztah mentioned , pls add description.

@selansen
Copy link
Copy Markdown
Contributor

selansen commented Jan 16, 2019

Just looked into failed test cases. They don't seem related to this. But both Windows CIs are failing. Pls confirm the same.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 16, 2019 •
edited
Loading

Codecov Report

Merging #38580 into master will increase coverage by 0.4%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #38580     +/-   ##
=========================================
+ Coverage    36.6%   37.01%   +0.4%     
=========================================
  Files         608      610      +2     
  Lines       45224    45751    +527     
=========================================
+ Hits        16555    16933    +378     
- Misses      26383    26490    +107     
- Partials     2286     2328     +42

yongtang
yongtang approved these changes Jan 16, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@yongtang yongtang left a comment

Choose a reason for hiding this comment

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

LGTM

@andrey-ko
Copy link
Copy Markdown
Contributor Author

andrey-ko commented Jan 17, 2019

investigating why test "TestStartReturnCorrectExitCode" fails on RS1...

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jan 17, 2019

TestStartReturnCorrectExitCode is flaky; I've seen it fail on other pull requests; let me restart CI

thaJeztah
thaJeztah approved these changes Jan 17, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@andrey-ko
Copy link
Copy Markdown
Contributor Author

andrey-ko commented Jan 18, 2019 •
edited
Loading

so after some investigation of failed tests on RS1 I find out that problem is that in new version hcsshim GetContainers changed returned error this happened after this commit:
microsoft/hcsshim@e70197a#diff-ece1473ec5e6e5832bc471a16557a457L157
so our fix with retry loop doesn't work correctly any more:

moby/daemon/graphdriver/windows/windows.go

Line 276 in 5ec3138

for {

more specifically condition err == hcsshim.ErrVmcomputeOperationAccessIsDenied is always false now because err is of hcs.HcsError type now, not syscall.Errno as it was before

@ddebroy
Copy link
Copy Markdown
Contributor

ddebroy commented Jan 18, 2019

@andrey-ko will it be possible to capture the findings around GetContainers above in a fresh issue?

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jan 18, 2019

ping @jstarks @jhowardmsft PTAL ^^ wondering if we can get utility functions in hcsshim to prevent such breaks (thinking of a hcsshim.IsSomeTypeOfError(err)), or a similar approach as the errdefs package uses (define interfaces for error-classes);

moby/errdefs/defs.go

Lines 3 to 11 in d48392a

// ErrNotFound signals that the requested object doesn't exist
type ErrNotFound interface {
NotFound()
}
// ErrInvalidParameter signals that the user input is invalid
type ErrInvalidParameter interface {
InvalidParameter()
}

moby/errdefs/is.go

Lines 32 to 42 in d48392a

// IsNotFound returns if the passed in error is an ErrNotFound
func IsNotFound(err error) bool {
_, ok := getImplementer(err).(ErrNotFound)
return ok
}
// IsInvalidParameter returns if the passed in error is an ErrInvalidParameter
func IsInvalidParameter(err error) bool {
_, ok := getImplementer(err).(ErrInvalidParameter)
return ok
}

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jan 18, 2019 •
edited
Loading

(removed comment I didn't read the diff correctly)

But, perhaps a causer interface would still make sense

@andrey-ko andrey-ko mentioned this pull request Jan 19, 2019
Open
@andrey-ko
keep old network ids
e017717 
for windows all networks are re-populated in the store during network controller initialization. In current version it also regenerate network Ids which may be referenced by other components and it may cause broken references to a networks. This commit avoids regeneration of network ids.

Signed-off-by: Andrey Kolomentsev <[email protected]>
@thaJeztah thaJeztah mentioned this pull request Jan 30, 2019
Closed
@cpuguy83
Copy link
Copy Markdown
Member

cpuguy83 commented Feb 2, 2019

What's the status here? Seems like it's all good, tests are green.

@andrey-ko
Copy link
Copy Markdown
Contributor Author

andrey-ko commented Feb 7, 2019

What's the status here? Seems like it's all good, tests are green.

Other issue discussed above is actually unrelated to this PR, so we can merge it

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Feb 7, 2019

ok, let's get this merged then 👍 thanks!

@thaJeztah thaJeztah merged commit 611b23c into moby:master Feb 7, 2019
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Feb 7, 2019

@ddebroy @andrey-ko there's some existing issues about restart-policies and windows containers; this one looks like it could be resolved by this fix; could you check? #29832

also (but they seem different, so likely unrelated); #33542, #35430

@andrey-ko andrey-ko deleted the fix-restart branch February 8, 2019 22:59
@thaJeztah thaJeztah mentioned this pull request Feb 11, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

+3 more reviewers

@fcrisciani fcrisciani fcrisciani approved these changes

@yongtang yongtang yongtang approved these changes

@selansen selansen selansen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@AkihiroSuda AkihiroSuda

Labels

area/networking Networking platform/windows status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@andrey-ko @ddebroy @thaJeztah @selansen @cpuguy83 @fcrisciani @yongtang @AkihiroSuda @GordonTheTurtle