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

bugfix: skip teardown network, if the sandbox has been stopped #1539

Merged
merged 1 commit into from
Jun 19, 2018

Conversation

YaoZengzeng
Copy link
Contributor

Signed-off-by: YaoZengzeng yaozengzeng@zju.edu.cn

Ⅰ. Describe what this PR did

Please refer: #1536

When stop the sandbox, if the network namespace file of the sandbox does not exist, skip tearing down the pod network.

Ⅱ. Does this pull request fix one issue?

fixes #1536

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@pouchrobot pouchrobot added areas/network kind/bug This is bug report for project size/M labels Jun 15, 2018
@codecov-io
Copy link

codecov-io commented Jun 15, 2018

Codecov Report

Merging #1539 into master will decrease coverage by 2.88%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1539      +/-   ##
==========================================
- Coverage   41.25%   38.36%   -2.89%     
==========================================
  Files         255      257       +2     
  Lines       16837    17691     +854     
==========================================
- Hits         6946     6788     -158     
- Misses       9024    10038    +1014     
+ Partials      867      865       -2
Impacted Files Coverage Δ
cri/v1alpha2/cri.go 0% <0%> (ø) ⬆️
cri/v1alpha1/cri.go 0% <0%> (ø) ⬆️
pkg/net/interface.go 0% <0%> (-89.07%) ⬇️
apis/server/container_bridge.go 56.61% <0%> (-26.27%) ⬇️
apis/server/router.go 74.55% <0%> (-16.75%) ⬇️
daemon/mgr/container.go 33.84% <0%> (-15.21%) ⬇️
ctrd/container.go 43.43% <0%> (-6.75%) ⬇️
ctrd/utils.go 56.84% <0%> (-3.16%) ⬇️
cli/update.go 0% <0%> (ø) ⬆️
cri/v1alpha1/cri_stream.go 0% <0%> (ø) ⬆️
... and 6 more

@YaoZengzeng
Copy link
Contributor Author

@allencloud PTAL

@fuweid fuweid self-requested a review June 19, 2018 01:59
if err != nil {
return nil, err
}
} else if !os.IsNotExist(err) {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to add more information in the error, could we use pkgerrors.Wrap to wrap the error? WDYT?

return nil, err
}
// If the sandbox has been closed, the corresponding network namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we put the comment on the top?

return nil, err
}
// If the sandbox has been closed, the corresponding network namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we put the comment on the top of if? if not, it seems that we delete the code but not comment. 😄

if err != nil {
return nil, err
}
} else if !os.IsNotExist(err) {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

use pkgerrors.Wrap to show the error which means that the file has been removed. WDYT?

Signed-off-by: YaoZengzeng <yaozengzeng@zju.edu.cn>
@YaoZengzeng
Copy link
Contributor Author

@fuweid Updated.

@fuweid
Copy link
Contributor

fuweid commented Jun 19, 2018

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/network kind/bug This is bug report for project size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] stop sandbox failed for not finding network namespace path
4 participants