-
Notifications
You must be signed in to change notification settings - Fork 349
Add container Exec support. #115
Add container Exec support. #115
Conversation
Signed-off-by: Lantao Liu <lantaol@google.com>
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.
Just a couple questions and one nit.
@@ -100,16 +149,11 @@ func (c *criContainerdService) ExecSync(ctx context.Context, r *runtime.ExecSync | |||
if err != nil { | |||
return nil, fmt.Errorf("failed to wait for exec in container %q to finish: %v", id, err) |
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.
always throw out the exit code if there is an error? unknownExitCode?
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.
Maybe we should let waitContainerExec
also return pointer, so that we don't need the unknownExitStatus
for now.
We don't use the exit code when there is an error now.
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.
right.. just trying to figure out if that's on purpose. Seems at least possible to have an exit code and an error. Not a fan of throwing it out unless we need to.
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.
Done.
@@ -84,6 +86,8 @@ type criContainerdService struct { | |||
// imageStoreService is the containerd service to store and track | |||
// image metadata. | |||
imageStoreService images.Store | |||
// eventsService is the containerd task service client |
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.
just curious why this one is better up here?
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.
Just group containerd services together. No particular reason. :)
pkg/server/service.go
Outdated
return c, nil | ||
} | ||
|
||
func (c *criContainerdService) Start() { | ||
c.startEventMonitor() | ||
go func() { | ||
if err := c.streamServer.Start(true); err != nil { | ||
glog.Errorf("Failed to start streaing server: %v", err) |
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.
/s/streaing/streaming/
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.
Will do.
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.
Done.
Signed-off-by: Lantao Liu <lantaol@google.com>
d30058f
to
4421cab
Compare
@mikebrow Addressed comments. |
Signed-off-by: Lantao Liu <lantaol@google.com>
Signed-off-by: Lantao Liu <lantaol@google.com>
4421cab
to
75731c8
Compare
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.
/LGTM
…treaming Add container Exec support.
This PR:
Exec
support.Exec
CRI validation test./cc @kubernetes-incubator/maintainers-cri-containerd