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.

Sign up for GitHub

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

Jump to bottom

pkg/cri: remove support for the obsolete v1alpha2 CRI protocol. #1081

Merged
merged 3 commits into from
Jan 5, 2024
Merged

pkg/cri: remove support for the obsolete v1alpha2 CRI protocol. #1081

merged 3 commits into from
Jan 5, 2024

Conversation

klihub
Copy link
Contributor

@klihub klihub commented Jan 5, 2024 •
edited
Loading

This patch set

  • removes support and protocol conversion for the obsolete v1alpha2 version of the CRI protocol
  • bumps our CRI dependency to release 1.29/v0.29.0
  • implements missing RuntimeService client and server interfaces
  • implements relaying of all the non-streaming Runtime client and server interfaces

Note that relaying for the streaming PLEG/GetContainerEvents interface is not implemented.

@klihub klihub force-pushed the devel/remove-cri-v1alpha2 branch 2 times, most recently from 3712af4 to ca1d9e3 Compare January 5, 2024 12:10
marquiz
marquiz reviewed Jan 5, 2024
View reviewed changes
Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

Thanks @klihub, looks good. If you ask me, I'd take this in after the other deps (#1079) and golang (#1080) version have been updated

@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cfe4bc2) 32.12% compared to head (3ffff03) 32.12%.
Report is 7 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1081   +/-   ##
=======================================
  Coverage   32.12%   32.12%           
=======================================
  Files          64       64           
  Lines        9937     9937           
=======================================
  Hits         3192     3192           
  Misses       6449     6449           
  Partials      296      296

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

askervin
askervin reviewed Jan 5, 2024
View reviewed changes
Copy link
Contributor

@askervin askervin left a comment

Choose a reason for hiding this comment

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

Looks good.

I suppose this is something to be emphasized in release notes... CRI-RM 0.9 requires Kubernetes 1.23+ and containerd 1.6+ or something. Which is fine as earlier versions are already EOL.

@marquiz
Copy link
Contributor

marquiz commented Jan 5, 2024

LGTM

@marquiz
Copy link
Contributor

marquiz commented Jan 5, 2024

I suppose this is something to be emphasized in release notes... CRI-RM 0.9 requires Kubernetes 1.23+ and containerd 1.6+ or something.

Yes, definitely.

@klihub please rebase and let's see how it goes. I think this would be "Ready for review" 😊

@klihub klihub marked this pull request as ready for review January 5, 2024 14:13
@marquiz
Copy link
Contributor

marquiz commented Jan 5, 2024

Or should we wait for #1079?

@klihub
Copy link
Contributor Author

klihub commented Jan 5, 2024

@marquiz, @askervin I think merging this requires/warrants rerunning the end-to-end tests.

@marquiz
Copy link
Contributor

marquiz commented Jan 5, 2024

I think merging this requires/warrants rerunning the end-to-end tests.

Fair point

@klihub
pkg/cri: remove support for CRI v1alpha2.
94fb0b1 
Ditch support (and protocol conversion) for the obsolete
v1alpha2 version of the CRI protocol. Implement the missing
v1 RuntimeService client and server interfaces. Implement
relaying for these same interfaces with the exception of the
GetContainerEvents/PLEG interface

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
@klihub
go.{mod,sum}: update CRI deps to release 1.29/v0.29.0.
3ffff03 
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
@klihub
test/e2e: run static-pools test on default distro.
30e1c64 
Don't force static-pools e2e test distro to debian/sid. It
looks like govm fails to boot it on recent fedora/docker
combos.

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
@klihub
Copy link
Contributor Author

klihub commented Jan 5, 2024

I think merging this requires/warrants rerunning the end-to-end tests.

Fair point

Okay, with the latest commit at the tip, all tests pass.

@marquiz
Copy link
Contributor

marquiz commented Jan 5, 2024

Okay, with the latest commit at the tip, all tests pass.

Yeah, same here.

LGTM

askervin
askervin approved these changes Jan 5, 2024
View reviewed changes
Copy link
Contributor

@askervin askervin left a comment

Choose a reason for hiding this comment

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

Thanks @klihub!

@askervin askervin merged commit db28d55 into intel:master Jan 5, 2024
6 checks passed
@klihub klihub deleted the devel/remove-cri-v1alpha2 branch January 8, 2024 07:58
@marquiz marquiz mentioned this pull request Jan 8, 2024
Closed
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@askervin askervin askervin approved these changes

@marquiz marquiz marquiz approved these changes

@kad kad Awaiting requested review from kad kad is a code owner

@mythi mythi Awaiting requested review from mythi mythi is a code owner

@fmuyassarov fmuyassarov Awaiting requested review from fmuyassarov fmuyassarov is a code owner

Assignees

@askervin askervin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@klihub @codecov-commenter @marquiz @askervin