-
Notifications
You must be signed in to change notification settings - Fork 402
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
Support other flavour of CRI #1437
Conversation
Hi @CiraciNicolo. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
Great stuff--this is a lot of work!
I did a quick review and just had some documentation suggestions. Also two general comments:
- please update the copyright year to
2024
in new files - please add a final newline to all files
/ok-to-test |
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.
Great work! A few comments on the public API for cri specific fields
Do we have a way to test this in CI?
cri_flavour: "containerd" | ||
crio_version: "" | ||
containerd_version: "" |
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.
thought: would be it be easier if this was just cri_version
? We would need to keep containerd_version
around for a release but then could remove it in a future release?
|
||
- name: Export crisocket | ||
set_fact: | ||
cri_socket: "{{ crio_cri_socket }}" |
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.
Same with crio_cri_socket
, should this be cri_socket
? And later we can remove the containerd_cri_socket
from the public api?
Thanks! I will check all your reports tomorrow. Great idea about removing specific variables for containerd, I was afraid doing that would introduce some issues. |
To be clear we should keep the variables for a release or two and mark them depreciated, otherwise it would could some hickups for folks I think. |
Yes, I was thinking the same thing. Will work on that, I'm a little bit rusty on packer and need to understand how can we manage if-not-defined-then for user variables. Will work on that this week |
Hi, since I messed up and worked on main instead of a branch I'm gonna close this PR and rework my commits into a dedicated branch. We can continue discuss there! Hope this is ok. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi! I cherry picked the commits to another branch, this is more cleaner. Please, refer to #1442. Sorry for the incorrect handling on the main branch. |
Change description
This PR implements support for different CRI implementations, instead of the default containerd. At this moment the only other CRI implementation supported is CRI-O. By default containerd is still the default CRI, to start using CRI-O references the packer configuration called "qemu-ubuntu-2204-crio.json".
Currently the only operating systems supported are debian based and centos based. It should also work on rhel but not tested.
The implementation can be done following this steps:
Both ansible role and default values should be called as your flavour, in this way you can set the
cri_flavour
build variable and all tasks/var should be included.Be also careful to correctly default the variables in
images/capi/packer/config/ansible-args.json
and implement the goss specs.Related issues
No issues are addressed by this PR
This PR also includes the following fixes:
Additional context