Add containerd cri #19
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maiqueb
force-pushed
the
add-containerd-cri
branch
3 times, most recently
from
b333e6d to
ea527a1
Compare
Signed-off-by: Miguel Duarte Barroso <[email protected]>
qinqon
requested changes
Sep 30, 2022
Comment on lines
+206
to
+218
| netnsPath, err := pnc.netnsPath(newPod) | ||
| if err != nil { | ||
| klog.Errorf("failed to figure out the pod's network namespace: %v", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
A DynamicAttachmentRequest without PodNetNS is legit ?
There was a problem hiding this comment.
Then why we don't return the error ?
There was a problem hiding this comment.
This is the UpdateFunc of the controller; how can it return errors?
podInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
UpdateFunc: podNetworksController.handlePodUpdate,
})There was a problem hiding this comment.
Then maybe better send an event more than an error ?
There was a problem hiding this comment.
Could be, but events go away after an hour. I prefer to just log.
maiqueb
force-pushed
the
add-containerd-cri
branch
from
ea527a1 to
184ee5a
Compare
Signed-off-by: Miguel Duarte Barroso <[email protected]>
Signed-off-by: Miguel Duarte Barroso <[email protected]>
maiqueb
force-pushed
the
add-containerd-cri
branch
from
184ee5a to
c80852f
Compare
Signed-off-by: Miguel Duarte Barroso <[email protected]>
maiqueb
force-pushed
the
add-containerd-cri
branch
from
c80852f to
4bcb812
Compare
By making the `DynamicAttachmentRequest` implement the stringer interface, we can have proper information whenever we log those requests. Signed-off-by: Miguel Duarte Barroso <[email protected]>
maiqueb
force-pushed
the
add-containerd-cri
branch
from
4bcb812 to
f5c801a
Compare
qinqon
reviewed
Sep 30, 2022
Comment on lines
+69
to
+83
| func (c Container) ID() string { | ||
| return c.id | ||
| } | ||
|
|
||
| func (Container) Info(context.Context, ...containerd.InfoOpts) (containers.Container, error) { | ||
| return containers.Container{}, nil | ||
| } | ||
|
|
||
| func (Container) Delete(context.Context, ...containerd.DeleteOpts) error { | ||
| return nil | ||
| } | ||
|
|
||
| func (Container) NewTask(context.Context, cio.Creator, ...containerd.NewTaskOpts) (containerd.Task, error) { | ||
| return nil, nil | ||
| } |
| "github.com/containerd/containerd" | ||
| ) | ||
|
|
||
| type Client interface { |
There was a problem hiding this comment.
My fault the containerd concept is called Client https://github.com/containerd/containerd/blob/main/client.go#L85
| @@ -9,3 +9,9 @@ const ( | |||
| // Containerd represents the containerd container runtime | |||
| Containerd RuntimeType = "containerd" | |||
| ) | |||
|
|
|||
| // ContainerRuntime interface | |||
| type ContainerRuntime interface { | |||
qinqon
approved these changes
Sep 30, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds the
containerdCRI implementation to the project, along with unit tests.Fixes: #20