Skip to content
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 cri request parsing/generate-hook-request/checkpoing logic #110

Merged
merged 1 commit into from
May 5, 2022

Conversation

honpey
Copy link
Contributor

@honpey honpey commented Apr 29, 2022

Ⅰ. Describe what this PR does

  1. parse cri protocol
  2. generate hook request to RuntimeHookServer(koordlet)
  3. merge response form RuntimeHookServer to cri request which would be transferred to backend containerd/dockerd
  4. checkpoint basing on bolt

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2022

Codecov Report

Merging #110 (11e1abe) into main (94bcf4d) will not change coverage.
The diff coverage is n/a.

❗ Current head 11e1abe differs from pull request most recent head af53e2b. Consider uploading reports for the commit af53e2b to get more accurate results

@@           Coverage Diff           @@
##             main     #110   +/-   ##
=======================================
  Coverage   54.34%   54.34%           
=======================================
  Files          77       77           
  Lines        7277     7277           
=======================================
  Hits         3955     3955           
  Misses       2972     2972           
  Partials      350      350           
Flag Coverage Δ
unittests 54.34% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94bcf4d...af53e2b. Read the comment docs.

Copy link
Member

@jasonliu747 jasonliu747 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work! only few suggestions.

return &NoopResourceExecutor{}
}

type NoopResourceExecutor struct {
Copy link
Member

@jasonliu747 jasonliu747 Apr 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does Noop stand for? Could you please elaborate on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does Noop mean? Could you explain briefly?

Noop means no-operation for cri request, where no hook exists like ListContainerStats/ExecSync etc.
comments has beed added.

pkg/runtime-manager/utils/utils.go Outdated Show resolved Hide resolved
vendor/modules.txt Outdated Show resolved Hide resolved
@honpey honpey force-pushed the runtime-dev branch 3 times, most recently from 11e1abe to 7913924 Compare May 5, 2022 03:03
Signed-off-by: pengyang.hpy <honpey@gmail.com>
Copy link
Member

@jasonliu747 jasonliu747 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@hormes
Copy link
Member

hormes commented May 5, 2022

/approve

@koordinator-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@koordinator-bot koordinator-bot bot merged commit ee19dce into koordinator-sh:main May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants