-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
kubelet: add a generic pod lifecycle event generator #13571
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ package dockertools | |
|
||
import ( | ||
"fmt" | ||
"strings" | ||
|
||
docker "github.com/fsouza/go-dockerclient" | ||
kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" | ||
|
@@ -27,6 +28,19 @@ import ( | |
// This file contains helper functions to convert docker API types to runtime | ||
// (kubecontainer) types. | ||
|
||
func mapStatus(status string) kubecontainer.ContainerStatus { | ||
// Parse the status string in docker.APIContainers. This could break when | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you think this would break? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might break because we are parsing a human readable string. On the other hand, it should be easy to verify when upgrading docker. |
||
// we upgrade docker. | ||
switch { | ||
case strings.HasPrefix(status, "Up"): | ||
return kubecontainer.ContainerStatusRunning | ||
case strings.HasPrefix(status, "Exited"): | ||
return kubecontainer.ContainerStatusExited | ||
default: | ||
return kubecontainer.ContainerStatusUnknown | ||
} | ||
} | ||
|
||
// Converts docker.APIContainers to kubecontainer.Container. | ||
func toRuntimeContainer(c *docker.APIContainers) (*kubecontainer.Container, error) { | ||
if c == nil { | ||
|
@@ -37,12 +51,14 @@ func toRuntimeContainer(c *docker.APIContainers) (*kubecontainer.Container, erro | |
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return &kubecontainer.Container{ | ||
ID: kubetypes.DockerID(c.ID).ContainerID(), | ||
Name: dockerName.ContainerName, | ||
Image: c.Image, | ||
Hash: hash, | ||
Created: c.Created, | ||
Status: mapStatus(c.Status), | ||
}, nil | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a pretty big change that is not strictly related to the point of this PR... am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primary focus of this PR is to reduce redundant syncs (10 second periodic syncs) when there is no change. The added PLEG already checks the container runtime frequently for any container changes. We keep the periodic sync as a fallback, in case anything is missed, but it should be less frequent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yujuhong cool, that makes sense to me -- this seems like the kind of thing that could affect some of the E2Es that are outside the merge path -- have you run any of those?