CRI: add support for CDI device injection #6654

bart0sh · 2022-03-09T19:34:17Z

This PR adds support for injecting CDI devices into containers.

CDI injection is already supported in Podman since v 3.20 and was recently implemented in CRI-O

This is also an essential part of work to implement Dynamic Resource Allocation in Kubernetes.

k8s-ci-robot · 2022-03-09T19:34:27Z

Hi @bart0sh. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

mikebrow · 2022-03-09T19:51:47Z

/ok-to-test

theopenlab-ci · 2022-03-09T20:38:42Z

Build succeeded.

containerd-build-arm64 : RETRY_LIMIT in 3m 37s (non-voting)
containerd-test-arm64 : RETRY_LIMIT in 5m 41s (non-voting)
containerd-integration-test-arm64 : RETRY_LIMIT in 25m 08s (non-voting)

pkg/cri/config/config_unix.go

container_opts.go

MikeZappa87 · 2022-03-10T17:03:48Z

container_opts.go

 	"errors"
 	"fmt"

+	"github.com/container-orchestrated-devices/container-device-interface/pkg/cdi"


It looks like the current spec version is 0.3.0. Does the git tag follow the spec version? How do you intend on handling upgrades and backward compatibility?

Yes the current spec version follows tag and set to 0.3.0 and tagged: https://github.com/container-orchestrated-devices/container-device-interface/blob/9dc97167c538437ff6076f9d087516f8c8b3f5d4/specs-go/config.go#L6

When you make a bug fix that is also a new spec? Also since this is an issue with the current CNI setup, what are your thoughts on upgrades and backward compatibility?

@MikeZappa87 so far it was the case. As CDI API is in alpha state we have been updating the spec and the code for any bug fixes and major API updates. We're going to do that until we reach first major release. After that we may consider using different API paths for incompatible API versions.

I think even if the code/git and the Spec versions are in sync now, going forward they will almost inevitably diverge. IMO the package version and the Spec version, while not completely orthogonal, they are not supposed to be updated in tandem. Instead, both should follow semantic versioning on their own. I think this implies that bumping the Spec version will always result in bumping the git tag, but not the other way around. For instance, changing the externally visible package programmatic interfaces/abstractions would result in a git tag bump, but it would leave the Spec version alone.

As for handling the evolution of the Spec, I think our default goal should be that when we change the Spec version, we provide transparent conversion (while loading the Spec) from a (reasonable window of) previous version(s) to the latest one, whenever this is possible. And when we design/decide Spec changes, we should keep this in mind and aim for such a possibility to exist.

Hi @MikeZappa87 we discussed this in our WG meeting yesterday and just wanted to check in.

I think the short answer is that we don't have versioning quite figured out at this stage. For the time being we are using the following approach:

API changes / bug fixes will result in a patch-version bump. This will only be applied to the git TAG and not affect the spec version.

Spec changes will result in a minor-version bump. Here, v0.3.0 is our first tagged release. There will be a git TAG associated with each spec version bump.

I don't think that we have hard guarantees on backward compatibility at this stage which we feel is reflected by a non-v1 spec version. This is not to say that we will be making breaking changes to the spec at every version bump, but rather that we cannot commit to not having to make breaking changes -- especially now that we are seeing adoption of CDI in podman, cri-o, and containerd.

I see the following immediate action items from our side:

Document the versioning strategy in our repository's README (Add note on versioning policy to README / SPEC cncf-tags/container-device-interface#57)

Call out our position on spec / API stability and the possibility of breaking changes in our repository's README (Add note on spec / API evolution to our README cncf-tags/container-device-interface#58)

We will also consider whether separating the spec and API makes sense at this stage and reevaluate this periodically.
If you have any insights on how to handle spec versioning going forward or examples of projects that you feel could be used as good examples, please reach out to us.

With the above points considered, do you see this as a blocker for getting this PR merged (given that this functionality is gated by a feature flag)?

Is it possible that a GitHub issue is created for this? Also, a TODO added to the code with a link to that Github issue?

I have created cncf-tags/container-device-interface#60 to track this on our end. Please let me know if there is insufficient context there.

that works.. we might need a longer term tracking issue for changes expected in containerd over time up to some desired golden state.. :-) and that could link back over to the cod/cod issues & prs

theopenlab-ci · 2022-03-11T10:26:50Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 31s (non-voting)
containerd-test-arm64 : FAILURE in 5m 40s (non-voting)
containerd-integration-test-arm64 : FAILURE in 24m 51s (non-voting)

container_opts.go

kzys · 2022-03-11T18:07:37Z

container_opts.go

+		registry := cdi.GetRegistry(cdi.WithSpecDirs(cdiSpecDirs...))
+		if err = registry.Refresh(); err != nil {
+			// We don't consider registry refresh failure a fatal error.
+			// For instance, a dynamically generated invalid CDI Spec file for
+			// any particular vendor shouldn't prevent injection of devices of
+			// different vendors. CDI itself knows better and it will fail the
+			// injection if necessary.
+			log.G(ctx).Warnf("CDI registry refresh failed: %v", err)
+		}


Correct me if I'm wrong, but it seems GetRegistry returns a singleton object and Refresh takes a lock on the object.

https://github.com/container-orchestrated-devices/container-device-interface/blob/46367ec063fda9da931d050b308ccd768e824364/pkg/cdi/registry.go

https://github.com/container-orchestrated-devices/container-device-interface/blob/ee12044b3895afdff3841fee894e3edf01d7fc26/pkg/cdi/cache.go

Wouldn't it introduce a lock contention here? Do we need to refresh the registry every time?

Correct me if I'm wrong, but it seems GetRegistry returns a singleton object and Refresh takes a lock on the object.
...
Wouldn't it introduce a lock contention here? Do we need to refresh the registry every time?

Yes, it is a singleton object which is locked during both Refresh() and CDI device resolution. We don't need to refresh the registry (at least not the brute force way we do now), as long as we make sure in some way that it's up to date. We haven't added any logic for that yet, but we'll get down to it now.

@kzys Once we have the implementation for automatic refresh of the CDI registry merged, we won't need to explicitly trigger a manual refresh here.

kzys · 2022-03-11T18:19:13Z

Is CDI Linux-only? Seems some of CDI dependencies are not build-able on Windows.

bart0sh · 2022-03-14T10:57:57Z

Is CDI Linux-only? Seems some of CDI dependencies are not build-able on Windows.

yes, it's Linux-only currently. I'll update the PR to fix build breaks on non-Linux platforms.

bart0sh · 2022-03-14T17:11:19Z

@kzys

I'll update the PR to fix build breaks on non-Linux platforms.

done

theopenlab-ci · 2022-03-14T18:12:51Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 29s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 50s (non-voting)
containerd-integration-test-arm64 : FAILURE in 29m 53s (non-voting)

bart0sh · 2022-03-15T10:09:51Z

@mikebrow can you re-trigger Windows integration test? I'm not sure its failure caused by the PR changes as they only change Linux code.

mikebrow · 2022-03-15T15:00:47Z

@mikebrow can you re-trigger Windows integration test? I'm not sure its failure caused by the PR changes as they only change Linux code.

that's a known issue you'll need to rebase

bart0sh · 2022-03-15T16:30:45Z

@mikebrow thanks. rebased.

Signed-off-by: Ed Bartosh <[email protected]>

theopenlab-ci · 2022-04-06T10:39:02Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 20s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 46s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 25m 07s (non-voting)

mikebrow

LGTM

bart0sh · 2022-04-06T15:30:32Z

@mxpv @mikebrow thank you for approving this. Can you merge or suggest whom to ask to merge it?

bart0sh · 2022-04-07T12:17:55Z

@mxpv @mikebrow Thank you for merging it! Very much appreciated.

Should I submit this PR for release/1.6 branch if I want it to be released sooner than in 1.7?

fuweid · 2022-04-07T12:26:20Z

backport is for bugfix and security fix. This change is new feature and big one. I don't think it should be backported.

klueska · 2022-08-04T07:25:54Z

@mikebrow, @fuweid
Should this be added to the 1.7 milestone so it doesn't get lost for release notes purposes?

k8s-ci-robot added the needs-ok-to-test label Mar 9, 2022

bart0sh force-pushed the cdi6 branch 2 times, most recently from 8dd2e31 to 9b8ad75 Compare March 9, 2022 19:43

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Mar 9, 2022

elezar reviewed Mar 10, 2022

View reviewed changes

pkg/cri/config/config_unix.go Show resolved Hide resolved

elezar reviewed Mar 10, 2022

View reviewed changes

container_opts.go Outdated Show resolved Hide resolved

MikeZappa87 reviewed Mar 10, 2022

View reviewed changes

bart0sh force-pushed the cdi6 branch from 9b8ad75 to 3ebda24 Compare March 11, 2022 09:59

TBBle mentioned this pull request Mar 11, 2022

Handle CRI Device.HostPath on Windows #6618

Merged

kzys reviewed Mar 11, 2022

View reviewed changes

kzys mentioned this pull request Mar 11, 2022

ctr: implement CDI support #6204

Closed

bart0sh force-pushed the cdi6 branch from 5aa92f2 to b81a655 Compare March 14, 2022 16:34

bart0sh force-pushed the cdi6 branch from b81a655 to ad4bccb Compare March 15, 2022 16:30

bart0sh force-pushed the cdi6 branch 5 times, most recently from e2a6153 to 66cc0d7 Compare March 22, 2022 21:14

bart0sh added 3 commits April 6, 2022 13:10

add configuration for CDI

c9b4ccf

Signed-off-by: Ed Bartosh <[email protected]>

move CDI calls to the linux-only code

ff5c558

Signed-off-by: Ed Bartosh <[email protected]>

CDI: update go.mod and vendor deps

825c1c5

Signed-off-by: Ed Bartosh <[email protected]>

bart0sh force-pushed the cdi6 branch from c3ce2b5 to 825c1c5 Compare April 6, 2022 10:11

elezar mentioned this pull request Apr 6, 2022

Support spec upgrades on read cncf-tags/container-device-interface#60

Closed

mikebrow approved these changes Apr 6, 2022

View reviewed changes

containerd deleted a comment from theopenlab-ci bot Apr 6, 2022

mxpv merged commit d6cf0d8 into containerd:main Apr 6, 2022

mxpv mentioned this pull request Apr 19, 2022

Implement CDI support #6195

Closed

fuweid added this to the 1.7 milestone Aug 4, 2022

This was referenced Aug 26, 2022

can not inject CDI device to container #7329

Closed

CRI: fix CDI device injection in k8s and update CDI version #7330

Closed

CRI: fix CDI device injection in k8s case #7331

Closed

soulseen mentioned this pull request Sep 7, 2022

CRI: update cdi version to v0.5.1 #7374

Merged

dmcgowan added the impact/changelog label Oct 21, 2022

dmcgowan mentioned this pull request Dec 10, 2022

Prepare release notes for v1.7.0-beta.1 #7793

Merged

dmcgowan mentioned this pull request Jan 3, 2023

Prepare release notes for v1.7.0-beta.2 #7903

Merged

dmcgowan mentioned this pull request Jan 28, 2023

Prepare release notes for v1.7.0-beta.3 #8014

Merged

dmcgowan mentioned this pull request Feb 15, 2023

Prepare release notes for v1.7.0 beta.4 #8110

Merged

This was referenced Feb 25, 2023

Prepare release notes for v1.7.0-rc.0 #8162

Merged

Prepare release notes for v1.7.0-rc.1 #8192

Merged

This was referenced Mar 7, 2023

Add release notes for v1.7.0-rc.2 #8216

Merged

Prepare release notes for v1.7.0-rc.3 #8233

Merged

Prepare release notes for v1.7.0 #8242

Merged

elezar mentioned this pull request May 17, 2023

Add support for CDI devices to ctr run command #8525

Merged

CRI: add support for CDI device injection #6654

CRI: add support for CDI device injection #6654

Uh oh!

Conversation

bart0sh commented Mar 9, 2022

Uh oh!

k8s-ci-robot commented Mar 9, 2022

Uh oh!

mikebrow commented Mar 9, 2022

Uh oh!

theopenlab-ci bot commented Mar 9, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klihub Mar 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeZappa87 Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theopenlab-ci bot commented Mar 11, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzys commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bart0sh commented Mar 14, 2022

Uh oh!

bart0sh commented Mar 14, 2022

Uh oh!

theopenlab-ci bot commented Mar 14, 2022

Uh oh!

bart0sh commented Mar 15, 2022

Uh oh!

mikebrow commented Mar 15, 2022

Uh oh!

bart0sh commented Mar 15, 2022

Uh oh!

theopenlab-ci bot commented Apr 6, 2022

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

bart0sh commented Apr 6, 2022

Uh oh!

bart0sh commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fuweid commented Apr 7, 2022

Uh oh!

klueska commented Aug 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

klihub Mar 15, 2022 •

edited

Loading

MikeZappa87 Apr 4, 2022 •

edited

Loading

mikebrow Apr 6, 2022 •

edited

Loading

kzys commented Mar 11, 2022 •

edited

Loading

bart0sh commented Apr 7, 2022 •

edited

Loading