Add crio runtime #31
Add crio runtime #31
Conversation
maiqueb
force-pushed
the
add-crio-runtime
branch
6 times, most recently
from
9c0d9f0
to
721bc72
Compare
| - name: containerd-socket | ||
| mountPath: {{ CRI_SOCKET_PATH }} | ||
| - name: cri-socket | ||
| mountPath: /host{{ CRI_SOCKET_PATH }} |
There was a problem hiding this comment.
You can remove the attribute from here. You can parametrize the host path in volumes to fit the system. Inside the container you may be able to keep it always on the same path, removing some of the complexity
There was a problem hiding this comment.
In the host, I'll have either:
- /run/containerd/containerd.sock
- /run/crio/crio.sock
IIUC, your suggestion is to mount to the pod something like: /host/cri-socket.sock .
Not saying this is bad, but I don't see the proposed alternative as more complex than hard-coding.
Plus, if I have access to the container - but not its configuration - I will always know which type of runtime I'm using.
(I do wonder if this scenario is common ...)
There was a problem hiding this comment.
I prefer keeping an abstract path as an "interface" in a way, so the manifest serves as a mapper between the two realms. I don't have any stronger reasoning for it, so I won't fight
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
maiqueb
force-pushed
the
add-crio-runtime
branch
2 times, most recently
from
c3b9449
to
d05ca4f
Compare
pkg/cri/crio/runtime.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| func getConnection(socketPath string, timeout time.Duration) (*grpc.ClientConn, error) { |
There was a problem hiding this comment.
Let's call this just connectSocket or openConnection so it's not "just" a getter.
There was a problem hiding this comment.
I'll see what I can do.
There was a problem hiding this comment.
Done - called it connect.
| @@ -146,5 +148,9 @@ func handleSignals(stopChannel chan struct{}, signals ...os.Signal) { | |||
|
|
|||
| func newContainerRuntime(configuration *config.Multus) (cri.ContainerRuntime, error) { | |||
| const withoutTimeout = 0 | |||
| if configuration.CriType == cri.Crio { | |||
| crioTimeout := 5 * time.Second | |||
There was a problem hiding this comment.
Why don't we need this for containerd ? maybe it's better to pass this as a "cmd" argument, so users decide what timeout they want.
There was a problem hiding this comment.
I have no clue why. I can only say a 0 will not work there - you get a Context deadline exceeded error instantly.
Going forward I agree this could be an interesting parameter to have in the configuration. Right now, I think it doesn't make much sense, given the early stage of the project.
I'll unify these for now.
There was a problem hiding this comment.
Actually, I do know why (just had to look).
If you pass a 0, containerd defaults to 10 seconds. Why 10 ? No clue.
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
This PR changes the manifest templating to also provide a manifest for CRIO based installations. This manifest features a different runtime configuration, which will cause the controller to use a different container runtime to access the pod's sandbox ID, and namespace. The golang code is also updated to use this new runtime, when configured. Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
maiqueb
force-pushed
the
add-crio-runtime
branch
from
d05ca4f
to
6ab9ab5
Compare
|
/lgtm |
This PR adds code to enable the controller to work with a CRIO runtime.
It also provides a manifest to deploy this option.
Fixes: #22