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: fix remove all endpoints when network disconnect #1284

Merged
merged 1 commit into from
May 11, 2018

Conversation

rudyfly
Copy link
Collaborator

@rudyfly rudyfly commented May 9, 2018

Ⅰ. Describe what this PR did

Fix remove all endpoints when network disconnect.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Signed-off-by: Rudy Zhang [email protected]

@pouchrobot pouchrobot added areas/network kind/bug This is bug report for project size/M labels May 9, 2018
@allencloud
Copy link
Collaborator

Please change error message more readable.

return errors.Wrapf(err, "failed to leave network: %s", endpoint.Name) will return message failed to leave network: abc: error.Error(). Two : make code unreadable.

logrus.Errorf("failed to delete sandbox id: %s, err: %v", sid, err)
return err
if err != nil || sb == nil {
return errors.Errorf("failed to get sandbox by id(%s)", sid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we communicate before about how to deal with the err and nil.

First I think we should make sure the API of function SandboxByID to check if there is possibility of returning both a nil sandbox and nil-err; if it does not and returns an instance of sandbox and nil-err, we could simply judge if err != nil.

In the code

if err != nil || sb == nil {
    return errors.Errorf("failed to get sandbox by id(%s)", sid)
}

It will miss the err message.

return errors.Errorf("not connected to the network(%s)", endpoint.Name)
}

if err := ep.Leave(sb); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this pr, you said remove all endpoints when network disconnect,

However, I found that it only deletes one endpoint. Is it correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean the bug is remove all endpoints when network disconnect, the correct result is just remove the endpoint in this network.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So I think the pr title is some kind of misleading. 😃

Fix remove all endpoints when network disconnect.

Signed-off-by: Rudy Zhang <[email protected]>
@codecov-io
Copy link

Codecov Report

Merging #1284 into master will decrease coverage by 0.94%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1284      +/-   ##
==========================================
- Coverage   16.45%   15.51%   -0.95%     
==========================================
  Files         182      173       -9     
  Lines       11285    10876     -409     
==========================================
- Hits         1857     1687     -170     
+ Misses       9292     9070     -222     
+ Partials      136      119      -17
Impacted Files Coverage Δ
daemon/mgr/network.go 3.32% <0%> (-0.17%) ⬇️
client/client.go 58.94% <0%> (-7.06%) ⬇️
pkg/meta/boltdb.go 60.56% <0%> (-5.71%) ⬇️
daemon/mgr/container.go 0% <0%> (ø) ⬆️
apis/server/container_bridge.go 0% <0%> (ø) ⬆️
daemon/mgr/image.go 0% <0%> (ø) ⬆️
apis/server/image_bridge.go 15% <0%> (ø) ⬆️
daemon/mgr/volume.go 0% <0%> (ø) ⬆️
daemon/config/config.go 7.89% <0%> (ø) ⬆️
apis/server/network_bridge.go 0% <0%> (ø) ⬆️
... and 14 more

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label May 11, 2018
@allencloud allencloud merged commit bd54a92 into AliyunContainerService:master May 11, 2018
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 LGTM one maintainer or community participant agrees to merge the pull reuqest. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants