From d429700c7b29cf4e6525443f95859afacfb8901b Mon Sep 17 00:00:00 2001 From: Allen Sun Date: Fri, 15 Jun 2018 17:05:59 +0800 Subject: [PATCH] refactor: make code more encapsulate and logic simple Signed-off-by: Allen Sun --- daemon/mgr/container.go | 251 ++++++++++++++++++++-------------- daemon/mgr/container_state.go | 6 +- daemon/mgr/container_types.go | 11 ++ 3 files changed, 163 insertions(+), 105 deletions(-) diff --git a/daemon/mgr/container.go b/daemon/mgr/container.go index f264696165..194374ec66 100644 --- a/daemon/mgr/container.go +++ b/daemon/mgr/container.go @@ -450,20 +450,24 @@ func (mgr *ContainerManager) Start(ctx context.Context, id, detachKeys string) ( } func (mgr *ContainerManager) start(ctx context.Context, c *Container, detachKeys string) error { - c.Lock() - if c.Config == nil || c.State == nil { - c.Unlock() - return errors.Wrapf(errtypes.ErrNotfound, "container %s", c.ID) - } c.DetachKeys = detachKeys - // initialise container network mode + if err := mgr.prepareContainerNetwork(ctx, c); err != nil { + return err + } + + return mgr.createContainerdContainer(ctx, c) +} + +func (mgr *ContainerManager) prepareContainerNetwork(ctx context.Context, c *Container) error { + c.Lock() + defer c.Unlock() + networkMode := c.HostConfig.NetworkMode if IsContainer(networkMode) { origContainer, err := mgr.Get(ctx, strings.SplitN(networkMode, ":", 2)[1]) if err != nil { - c.Unlock() return err } @@ -472,40 +476,40 @@ func (mgr *ContainerManager) start(ctx context.Context, c *Container, detachKeys c.ResolvConfPath = origContainer.ResolvConfPath c.Config.Hostname = origContainer.Config.Hostname c.Config.Domainname = origContainer.Config.Domainname - } else { - // initialise host network mode - if IsHost(networkMode) { - hostname, err := os.Hostname() - if err != nil { - c.Unlock() - return err - } - c.Config.Hostname = strfmt.Hostname(hostname) - } - // build the network related path. - if err := mgr.buildNetworkRelatedPath(c); err != nil { - c.Unlock() + return nil + } + + // initialise host network mode + if IsHost(networkMode) { + hostname, err := os.Hostname() + if err != nil { return err } + c.Config.Hostname = strfmt.Hostname(hostname) + } - // initialise network endpoint - if c.NetworkSettings != nil { - for name, endpointSetting := range c.NetworkSettings.Networks { - endpoint := mgr.buildContainerEndpoint(c) - endpoint.Name = name - endpoint.EndpointConfig = endpointSetting - if _, err := mgr.NetworkMgr.EndpointCreate(ctx, endpoint); err != nil { - logrus.Errorf("failed to create endpoint: %v", err) - c.Unlock() - return err - } - } + // build the network related path. + if err := mgr.buildNetworkRelatedPath(c); err != nil { + return err + } + + // initialise network endpoint + if c.NetworkSettings == nil { + return nil + } + + for name, endpointSetting := range c.NetworkSettings.Networks { + endpoint := mgr.buildContainerEndpoint(c) + endpoint.Name = name + endpoint.EndpointConfig = endpointSetting + if _, err := mgr.NetworkMgr.EndpointCreate(ctx, endpoint); err != nil { + logrus.Errorf("failed to create endpoint: %v", err) + return err } } - c.Unlock() - return mgr.createContainerdContainer(ctx, c) + return nil } // buildNetworkRelatedPath builds the network related path. @@ -902,12 +906,14 @@ func (mgr *ContainerManager) Remove(ctx context.Context, name string, options *t // if the container is running, force to stop it. if c.IsRunning() && options.Force { - msg, err := mgr.Client.DestroyContainer(ctx, c.ID, c.StopTimeout()) + _, err := mgr.Client.DestroyContainer(ctx, c.ID, c.StopTimeout()) if err != nil && !errtypes.IsNotfound(err) { - return errors.Wrapf(err, "failed to destroy container %s", c.ID) + return errors.Wrapf(err, "failed to destroy container %s when removing", c.ID) } - if err := mgr.markStoppedAndRelease(c, msg); err != nil { - return errors.Wrapf(err, "failed to mark container %s stop status", c.ID) + // After stopping a running container, we should release container resource + c.UnsetMergedDir() + if err := mgr.releaseContainerResources(c); err != nil { + logrus.Errorf("failed to release container %s resources when removing: %v", c.ID, err) } } @@ -915,22 +921,23 @@ func (mgr *ContainerManager) Remove(ctx context.Context, name string, options *t logrus.Errorf("failed to detach volume: %v", err) } - // remove name - c.Lock() - mgr.NameToID.Remove(c.Name) - c.Unlock() - - // remove meta data - if err := mgr.Store.Remove(c.Key()); err != nil { - logrus.Errorf("failed to remove container %s from meta store: %v", c.ID, err) + // remove snapshot + if err := mgr.Client.RemoveSnapshot(ctx, c.ID); err != nil { + logrus.Errorf("failed to remove snapshot of container %s: %v", c.ID, err) } + // When removing a container, we have set up such rule for object removing sequences: + // 1. container object in pouchd's memory; + // 2. meta.json for container in local disk. + + // remove name + mgr.NameToID.Remove(c.Name) // remove container cache mgr.cache.Remove(c.ID) - // remove snapshot - if err := mgr.Client.RemoveSnapshot(ctx, c.ID); err != nil { - logrus.Errorf("failed to remove snapshot of container %s: %v", c.ID, err) + // remove meta.json for container in local disk + if err := mgr.Store.Remove(c.Key()); err != nil { + logrus.Errorf("failed to remove container %s from meta store: %v", c.ID, err) } return nil @@ -1687,14 +1694,6 @@ func (mgr *ContainerManager) markStoppedAndRelease(c *Container, m *ctrd.Message c.SetStatusStopped(code, errMsg) - // unset Snapshot MergedDir. Stop a container will - // delete the containerd container, the merged dir - // will also be deleted, so we should unset the - // container's MergedDir. - if c.Snapshotter != nil && c.Snapshotter.Data != nil { - c.Snapshotter.Data["MergedDir"] = "" - } - // Action Container Remove and function markStoppedAndRelease are conflict. // If a container has been removed and the corresponding meta.json will be removed as well. // However, when this function markStoppedAndRelease still keeps the container instance, @@ -1707,47 +1706,52 @@ func (mgr *ContainerManager) markStoppedAndRelease(c *Container, m *ctrd.Message // Remove io and network config may occur error, so we should update // container's status on disk as soon as possible. - if err := c.Write(mgr.Store); err != nil { - logrus.Errorf("failed to update meta: %v", err) - return err - } + defer func() { + if err := c.Write(mgr.Store); err != nil { + logrus.Errorf("failed to update meta: %v", err) + } + }() - // release resource - if io := mgr.IOs.Get(c.ID); io != nil { - io.Close() - mgr.IOs.Remove(c.ID) + c.UnsetMergedDir() + + return mgr.releaseContainerResources(c) +} + +func (mgr *ContainerManager) markExitedAndRelease(c *Container, m *ctrd.Message) error { + var ( + exitCode int64 // container exit code used for container state setting + errMsg string // container exit error message used for container state setting + ) + if m != nil { + exitCode = int64(m.ExitCode()) + if err := m.RawError(); err != nil { + errMsg = err.Error() + } } - // No network binded, just return - c.Lock() - if c.NetworkSettings == nil { - c.Unlock() + c.SetStatusExited(exitCode, errMsg) + + // Action Container Remove and function markStoppedAndRelease are conflict. + // If a container has been removed and the corresponding meta.json will be removed as well. + // However, when this function markStoppedAndRelease still keeps the container instance, + // there will be possibility that in markStoppedAndRelease, code calls c.Write(mgr.Store) to + // write the removed meta.json again. If that, incompatibilty happens. + // As a result, we check whether this container is still in the meta store. + if container, err := mgr.container(c.Name); err != nil || container == nil { return nil } - for name, epConfig := range c.NetworkSettings.Networks { - endpoint := mgr.buildContainerEndpoint(c) - endpoint.Name = name - endpoint.EndpointConfig = epConfig - if err := mgr.NetworkMgr.EndpointRemove(context.Background(), endpoint); err != nil { - // TODO(ziren): it is a trick, we should wrapper "sanbox - // not found"" as an error type - if !strings.Contains(err.Error(), "not found") { - logrus.Errorf("failed to remove endpoint: %v", err) - c.Unlock() - return err - } + // Remove io and network config may occur error, so we should update + // container's status on disk as soon as possible. + defer func() { + if err := c.Write(mgr.Store); err != nil { + logrus.Errorf("failed to update meta: %v", err) } - } - c.Unlock() + }() - // update meta - if err := c.Write(mgr.Store); err != nil { - logrus.Errorf("failed to update meta of container %s: %v", c.ID, err) - return err - } + c.UnsetMergedDir() - return nil + return mgr.releaseContainerResources(c) } // exitedAndRelease be register into ctrd as a callback function, when the running container suddenly @@ -1758,12 +1762,10 @@ func (mgr *ContainerManager) exitedAndRelease(id string, m *ctrd.Message) error return err } - if err := mgr.markStoppedAndRelease(c, m); err != nil { + if err := mgr.markExitedAndRelease(c, m); err != nil { return err } - c.SetStatusExited() - // Action Container Remove and function exitedAndRelease are conflict. // If a container has been removed and the corresponding meta.json will be removed as well. // However, when this function exitedAndRelease still keeps the container instance, @@ -1774,11 +1776,6 @@ func (mgr *ContainerManager) exitedAndRelease(id string, m *ctrd.Message) error return nil } - if err := c.Write(mgr.Store); err != nil { - logrus.Errorf("failed to update meta: %v", err) - return err - } - // send exit event to monitor mgr.monitor.PostEvent(ContainerExitEvent(c).WithHandle(func(c *Container) error { // check status and restart policy @@ -1816,19 +1813,65 @@ func (mgr *ContainerManager) execExitedAndRelease(id string, m *ctrd.Message) er execConfig.Running = false execConfig.Error = m.RawError() - if io := mgr.IOs.Get(id); io != nil { - if err := m.RawError(); err != nil { - fmt.Fprintf(io.Stdout, "%v\n", err) - } + io := mgr.IOs.Get(id) + if io == nil { + return nil + } - // close io - io.Close() - mgr.IOs.Remove(id) + if err := m.RawError(); err != nil { + fmt.Fprintf(io.Stdout, "%v\n", err) } + // close io + io.Close() + mgr.IOs.Remove(id) + return nil } +func (mgr *ContainerManager) releaseContainerResources(c *Container) error { + mgr.releaseContainerIOs(c.ID) + return mgr.releaseContainerNetwork(c) +} + +// releaseContainerNetwork release container network when container exits or is stopped. +func (mgr *ContainerManager) releaseContainerNetwork(c *Container) error { + c.Lock() + defer c.Unlock() + if c.NetworkSettings == nil { + return nil + } + + for name, epConfig := range c.NetworkSettings.Networks { + endpoint := mgr.buildContainerEndpoint(c) + endpoint.Name = name + endpoint.EndpointConfig = epConfig + if err := mgr.NetworkMgr.EndpointRemove(context.Background(), endpoint); err != nil { + // TODO(ziren): it is a trick, we should wrapper "sanbox + // not found"" as an error type + if !strings.Contains(err.Error(), "not found") { + logrus.Errorf("failed to remove endpoint: %v", err) + return err + } + } + } + + return nil +} + +// releaseContainerIOs releases container IO resources. +func (mgr *ContainerManager) releaseContainerIOs(containerID string) { + // release resource + io := mgr.IOs.Get(containerID) + if io == nil { + return + } + + io.Close() + mgr.IOs.Remove(containerID) + return +} + func (mgr *ContainerManager) attachVolume(ctx context.Context, name string, c *Container) (string, string, error) { driver := volumetypes.DefaultBackend v, err := mgr.VolumeMgr.Get(ctx, name) diff --git a/daemon/mgr/container_state.go b/daemon/mgr/container_state.go index 811ec33c12..749352c994 100644 --- a/daemon/mgr/container_state.go +++ b/daemon/mgr/container_state.go @@ -110,10 +110,14 @@ func (c *Container) SetStatusStopped(exitCode int64, errMsg string) { } // SetStatusExited sets a container to be status exited. -func (c *Container) SetStatusExited() { +func (c *Container) SetStatusExited(exitCode int64, errMsg string) { c.Lock() defer c.Unlock() c.State.Status = types.StatusExited + c.State.FinishedAt = time.Now().UTC().Format(utils.TimeLayout) + c.State.Pid = -1 + c.State.ExitCode = exitCode + c.State.Error = errMsg } // SetStatusPaused sets a container to be status paused. diff --git a/daemon/mgr/container_types.go b/daemon/mgr/container_types.go index fa6a892be9..51ee9c02cb 100644 --- a/daemon/mgr/container_types.go +++ b/daemon/mgr/container_types.go @@ -283,6 +283,17 @@ func (c *Container) FormatStatus() (string, error) { return status, nil } +// UnsetMergedDir unsets Snapshot MergedDir. Stop a container will +// delete the containerd container, the merged dir +// will also be deleted, so we should unset the +// container's MergedDir. +func (c *Container) UnsetMergedDir() { + if c.Snapshotter == nil || c.Snapshotter.Data == nil { + return + } + c.Snapshotter.Data["MergedDir"] = "" +} + // ContainerRestartPolicy represents the policy is used to manage container. type ContainerRestartPolicy types.RestartPolicy