bugfix: fix container can't ping outside and resolve domain names #2025
Conversation
test/cli_run_network_test.go
Outdated
|
|
||
| "github.com/alibaba/pouch/test/command" | ||
| "github.com/alibaba/pouch/test/environment" | ||
| "github.com/go-check/check" |
test/cli_run_network_test.go
Outdated
| } | ||
|
|
||
| // TearDownTest does cleanup work in the end of each test. | ||
| func (suite *PouchRunNetworkSuite) TearDownTest(c *check.C) { |
test/cli_run_network_test.go
Outdated
|
|
||
| // TestRunWithPing is to verify run container with network ping publish website. | ||
| func (suite *PouchRunNetworkSuite) TestRunWithPing(c *check.C) { | ||
| pc, _, _, _ := runtime.Caller(0) |
There was a problem hiding this comment.
please use the name instead of complicated hacking function
daemon/mgr/network.go
Outdated
| @@ -182,7 +182,7 @@ func (nm *NetworkManager) Remove(ctx context.Context, name string) error { | |||
| nw, err := nm.controller.NetworkByName(name) | |||
| if err != nil { | |||
| if err == libnetwork.ErrNoSuchNetwork(name) { | |||
| return errors.Wrap(errtypes.ErrNotfound, err.Error()) | |||
There was a problem hiding this comment.
I think we can keep it the wrap here 😄
There was a problem hiding this comment.
Never ever directly return the ErrNotFound, it will be so ambiguous for user to understand.
No one understands the not found if no context is there. @rudyfly
daemon/mgr/network.go
Outdated
| @@ -182,7 +182,7 @@ func (nm *NetworkManager) Remove(ctx context.Context, name string) error { | |||
| nw, err := nm.controller.NetworkByName(name) | |||
| if err != nil { | |||
| if err == libnetwork.ErrNoSuchNetwork(name) { | |||
| return errors.Wrap(errtypes.ErrNotfound, err.Error()) | |||
There was a problem hiding this comment.
Never ever directly return the ErrNotFound, it will be so ambiguous for user to understand.
No one understands the not found if no context is there. @rudyfly
test/cli_run_network_test.go
Outdated
| func (suite *PouchRunNetworkSuite) TearDownTest(c *check.C) { | ||
| } | ||
|
|
||
| // TestRunWithPing is to verify run container with network ping publish website. |
|
Could you also add an integration test to cover the domain name resolving? @rudyfly |
|
@fuweid PTAL 😄 @allencloud |
|
ping @rudyfly |
Codecov Report
@@ Coverage Diff @@
## master #2025 +/- ##
=========================================
- Coverage 63.32% 63.3% -0.03%
=========================================
Files 200 200
Lines 15525 15527 +2
=========================================
- Hits 9831 9829 -2
- Misses 4454 4456 +2
- Partials 1240 1242 +2
|
fix container can't ping outside. Signed-off-by: Rudy Zhang <[email protected]>
allencloud
changed the title
|
LGTM, tested very well on my local machine. |
pouchrobot
added
the
LGTM
Ⅰ. Describe what this PR did
fix container can't ping outside with nat network.
Ⅱ. Does this pull request fix one issue?
fixes #1920
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
run testcase
TestRunWithPingⅤ. Special notes for reviews
Signed-off-by: Rudy Zhang [email protected]