-
Notifications
You must be signed in to change notification settings - Fork 314
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
Introduce framwork of RuntimeManager and cri-proxy #105
Conversation
Codecov Report
@@ Coverage Diff @@
## main #105 +/- ##
=======================================
Coverage 44.19% 44.19%
=======================================
Files 73 73
Lines 5965 5965
=======================================
Hits 2636 2636
Misses 3036 3036
Partials 293 293
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
package utils | ||
|
||
const ( | ||
DefaultRuntimeManagerSocketPath = "/var/run/runtimemanger/runtimemanager.sock" |
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.
please add TODO if we may customize these paths
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.
please add TODO if we may customize these paths
fixed path, would not be changed
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.
ok
3998f94
to
047c1fa
Compare
) | ||
|
||
const ( | ||
RunPodSandbox RuntimeServiceType = iota |
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.
why not reuse the definition in cri repo?
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.
it seems no const or variable "RunPodSandbox" exposed in cri repo?
why not reuse the definition in cri repo?
it seems no const or variable "RunPodSandbox" exposed in cri repo?
return err | ||
} | ||
|
||
func (ci *RuntimeManagerCriServer) getRuntimeHookInfo(serviceType RuntimeServiceType) (config.RuntimeRequestPath, |
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.
what's the difference between RuntimeRequestPath and ServiceType?
type RuntimeResourceType string | ||
|
||
const ( | ||
RuntimePodResource RuntimeResourceType = "RuntimePodResource" |
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.
I think we can merge the pod resource and container resource, because a container must belong to a pod.
the meta info is same for docker and cri, we need a unified struct for docker and cri
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.
I think we can merge the pod resource and container resource, because a container must belong to a pod.
the meta info is same for docker and cri, we need a unified struct for docker and cri
because in StartContainer/UpdateContainer api, there is only containerID, so we should have a db indexed by containerID, otherwise we may loop the whole db to find specified container info.
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.
I think we can merge the pod resource and container resource, because a container must belong to a pod.
the meta info is same for docker and cri, we need a unified struct for docker and cribecause in StartCcontainer/UpdateContainer api, there is only containerID, so we should have a db indexed by containerID, otherwise we may loop the whole db to find specify container info.
the repo stores unified struct of container/pod for both containerd/dockerd sceario. the docker resource-executor should convert the docker request to the unified struct and store
|
||
type RuntimeManagerCriServer struct { | ||
hookDispatcher *dispatcher.RuntimeHookDispatcher | ||
runtimeClient runtimeapi.RuntimeServiceClient |
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 we use it to communicate with hook server? change to hookClient, WDYT?
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 we use it to communicate with hook server? change to hookClient, WDYT?
runtimeClient is used to talk with containerd not hookserver.
but it maybe changed to BackendClient to avoid confusing
Signed-off-by: pengyang.hpy <honpey@gmail.com>
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eahydra, hormes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: pengyang.hpy honpey@gmail.com
Ⅰ. Describe what this PR does
Introduce framwork of RuntimeManager and cri-proxy
framework of runtimemanager, including:
server: intercept request from kubelet, and forward request to backend containerd/dockerd
resource-executor: parse intercepted request, genereate request to hook server and do checkpoint
dispatcher: mange hook server client and dispatch hook request to hook server
cri-proxy
part of 'server', intercept request from kubelet under containerd scenario
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how to verify it
Ⅳ. Special notes for reviews