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 endpoints disappear when pouchd restart #1312

Merged
merged 1 commit into from
May 15, 2018

Conversation

rudyfly
Copy link
Collaborator

@rudyfly rudyfly commented May 14, 2018

Ⅰ. Describe what this PR did

Fix endpoints disappear when pouchd restart, restore network controller
need keep endpoints that is using by the running containers, add the
active sandboxes option to initialize network controller.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

The container's endpoint is exist when pouchd has restarted.

  1. create container
# pouch run -d --name foo registry.hub.docker.com/library/busybox:latest top
  1. restart pouchd
  2. check the endpoint is exist or not
# pouch exec -ti foo ifconfig
eth0      Link encap:Ethernet  HWaddr 02:42:AC:11:00:02
          inet addr:172.17.0.2  Bcast:0.0.0.0  Mask:255.255.255.0
          inet6 addr: fe80::42:acff:fe11:2/64 Scope:Link
          UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
          RX packets:22 errors:0 dropped:0 overruns:0 frame:0
          TX packets:7 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:0
          RX bytes:1828 (1.7 KiB)  TX bytes:586 (586.0 B)

lo        Link encap:Local Loopback
          inet addr:127.0.0.1  Mask:255.0.0.0
          inet6 addr: ::1/128 Scope:Host
          UP LOOPBACK RUNNING  MTU:65536  Metric:1
          RX packets:0 errors:0 dropped:0 overruns:0 frame:0
          TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:1000
          RX bytes:0 (0.0 B)  TX bytes:0 (0.0 B)

Ⅴ. Special notes for reviews

Signed-off-by: Rudy Zhang rudyflyzhang@gmail.com

@pouchrobot pouchrobot added kind/bug This is bug report for project size/L labels May 14, 2018
@rudyfly rudyfly force-pushed the network-bugfix branch 2 times, most recently from c83b458 to d1ae621 Compare May 14, 2018 14:06
Fix endpoints disappear when pouchd restart, restore network controller
need keep endpoints that is using by the running containers, add the
active sandboxes option to initialize network controller.

Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>
@codecov-io
Copy link

Codecov Report

Merging #1312 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1312      +/-   ##
==========================================
- Coverage   17.38%   17.37%   -0.01%     
==========================================
  Files         189      189              
  Lines       11772    11789      +17     
==========================================
+ Hits         2046     2048       +2     
- Misses       9578     9594      +16     
+ Partials      148      147       -1
Impacted Files Coverage Δ
daemon/mgr/container_utils.go 49.61% <0%> (-8.42%) ⬇️
daemon/mgr/network.go 3.19% <0%> (-0.14%) ⬇️
daemon/mgr/container.go 0% <0%> (ø) ⬆️
daemon/logger/jsonfile/utils.go 70% <0%> (+1.66%) ⬆️

@@ -65,13 +65,32 @@ type NetworkManager struct {
}

// NewNetworkManager creates a brand new network manager.
func NewNetworkManager(cfg *config.Config, store *meta.Store) (*NetworkManager, error) {
func NewNetworkManager(cfg *config.Config, store *meta.Store, ctrMgr ContainerMgr) (*NetworkManager, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we replace ctrMgr to containerMgr, because ctrMgr may be a little confused.

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 think both is ok, short name in function paramters

// Create a new controller instance
cfg.NetworkConfg.MetaPath = path.Dir(store.BaseDir)
cfg.NetworkConfg.ExecRoot = network.DefaultExecRoot

initNetworkLog(cfg)

// get active sandboxes
ctrs, err := ctrMgr.List(context.Background(),
Copy link
Contributor

Choose a reason for hiding this comment

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

replace ctrs to containers ?

logrus.Errorf("failed to new network manager, can not get container list")
return nil, errors.Wrap(err, "failed to get container list")
}
cfg.NetworkConfg.ActiveSandboxes = make(map[string]interface{})
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to store ActiveSandboxes to disk ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

don't need to store to disk

@@ -562,7 +585,7 @@ func endpointOptions(n libnetwork.Network, endpoint *types.Endpoint) ([]libnetwo
return createOptions, nil
}

func (nm *NetworkManager) sandboxOptions(endpoint *types.Endpoint) ([]libnetwork.SandboxOption, error) {
func buildSandboxOptions(config network.Config, endpoint *types.Endpoint) ([]libnetwork.SandboxOption, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think sandbox options should not depend on endpoint config. If we have many endpoints, it means we will lose some information?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

endpoint in pouchd is different from endpoint in libnetwork, next version I will change it to 'NetDevice'

@HusterWan
Copy link
Contributor

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label May 15, 2018
@HusterWan HusterWan merged commit 46f76ba into AliyunContainerService:master May 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants