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

cri: Drop support for CRI v1alpha2 #1965

Merged
merged 2 commits into from
Aug 23, 2023
Merged

Conversation

mqasimsarfraz
Copy link
Member

@mqasimsarfraz mqasimsarfraz commented Aug 21, 2023

Currently, we support CRI v1/v1alpha2 simultaneously. The approach we were using (via unsafe.Pointer) works fine till cri-api v1.27.x but since cri-api v1.28.x added a field ('user_specified_image' field to type 'ImageSpec') it breaks the conversion.

The main reason to support both the CRI API version was to support older version of containerd (1.4.x), which has already been addressed since we migrated to container API directly. So this changes doesn't effect containerd runtimeclient in anyway. In terms of CRI-O, this means we will now only support CRI-O clusters newer than Kubernetes version: v1.20.0.

Copy link
Member

@eiffel-fl eiffel-fl left a comment

Choose a reason for hiding this comment

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

Perfect if we can drop some old API support!
Just check the commit message "sincewe" -> "since we".

Also, should the minimal supported version be documented somewhere?

Currently, we support CRI v1/v1apha2 simultaneously. The approach
we were using (via unsafe.Pointer) works fine till cri-api v1.27.x
but since cri-api v1.28.x added a field ('user_specified_image' field to type 'ImageSpec')
it breaks the conversion.

The main reason to support both the CRI API version was
to support older version of containerd (1.4.x), which has already been
addressed since we migrated to using containerd API directly. So this change
doen't effect containerd runtimeclient in anyway. In terms of CRI-O, this
means we will now only support CRI-O clusters v1.20.0+ (Kubernetes: v1.20.0+)

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>
Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>
@mqasimsarfraz mqasimsarfraz force-pushed the qasim/drop-support-for-v1alpha2 branch from 82be723 to 7551448 Compare August 22, 2023 08:45
@mqasimsarfraz
Copy link
Member Author

Just check the commit message "sincewe" -> "since we".

Done

Also, should the minimal supported version be documented somewhere?

Good idea. I created a commit to document it. Please let me know if it looks good!

Copy link
Member

@eiffel-fl eiffel-fl left a comment

Choose a reason for hiding this comment

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

The documentation is cool!
I nonetheless would like to have someone else reviewing it before you press "merge".


By default, `ig` will try to communicate with the Docker Engine
API and the CRI API of containerd and CRI-O:
**Note:** We only support CRI v1 meaning that only CRI-O v1.20+ (compatible with Kubernetes v1.20+) is supported.
Copy link
Member

Choose a reason for hiding this comment

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

This note and the details of support for containerd should be moved away for this "Usage" section into docs/requirements.md. At the moment, docs/requirements.md only has one section about the kernel config. It could have another section about Kubernetes & container runtimes.

Copy link
Member

@alban alban left a comment

Choose a reason for hiding this comment

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

LGTM

I added a comment about the documentation, but it was already problematic before your patch, so I think you could merge this PR and address the comment in a future PR if you prefer.

@mqasimsarfraz
Copy link
Member Author

I added a comment about the documentation, but it was already problematic before your patch, so I think you could merge this PR and address the comment in a future PR if you prefer.

Thanks, I will merge this one and open a separate PR for documentation.

@mqasimsarfraz mqasimsarfraz merged commit cc08778 into main Aug 23, 2023
46 checks passed
@mqasimsarfraz mqasimsarfraz deleted the qasim/drop-support-for-v1alpha2 branch August 23, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants