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

devel: better support for runtime integration #43

Merged
merged 3 commits into from
Jan 31, 2022

Conversation

klihub
Copy link
Contributor

@klihub klihub commented Jan 24, 2022

This PR improves support for runtime CDI integration. See this accompanying branch for cri-o CDI integration as an example of using it.

In particular this PR

  • improves CDI device validation
  • improves CDI device injection to be more in line with runtime expectations
  • adds support for annotating and extracting CDI devices from annotations

Requesting injection via annotation is a stop-gap solution until the CRI protocol gains dedicated CDI support.

Accept all OCI supported device types in CDI Spec files.
Address a few bugs and omissions in InjectDevices making
it more complete / better usable in runtimes:

  - generate cgroup device access rules
  - remove conflicting devices/mounts prior to injection
  - fill in missing device type, major/minor number from host
  - use container uid/gid if they are unspecified in CDI Spec
  - sort mounts after injection

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub klihub requested review from bartosh, elezar and kad January 24, 2022 12:01
@kad
Copy link
Contributor

kad commented Jan 25, 2022

from my perspective, looks good.

@bart0sh
Copy link
Contributor

bart0sh commented Jan 26, 2022

I've tested this on both sides - in the device plugin and in containerd. Worked just fine.
@elezar please review & merge

@bart0sh
Copy link
Contributor

bart0sh commented Jan 26, 2022

/assign @elezar

@elezar
Copy link
Contributor

elezar commented Jan 26, 2022

Hi sorry. Been off sick. Will have a look once I'm back.

Copy link
Contributor

@elezar elezar 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. This looks good.

I have some minor comments regarding naming and validation. In general this looks good though.

pkg/cdi/annotations.go Show resolved Hide resolved
pkg/cdi/annotations.go Outdated Show resolved Hide resolved
pkg/cdi/annotations.go Show resolved Hide resolved
pkg/cdi/annotations.go Outdated Show resolved Hide resolved
pkg/cdi/annotations.go Outdated Show resolved Hide resolved
pkg/cdi/container-edits.go Show resolved Hide resolved
pkg/cdi/container-edits.go Show resolved Hide resolved
pkg/cdi/container-edits.go Show resolved Hide resolved
pkg/cdi/container-edits.go Show resolved Hide resolved
pkg/cdi/container-edits.go Show resolved Hide resolved
pkg/cdi/annotations.go Outdated Show resolved Hide resolved
pkg/cdi/annotations.go Outdated Show resolved Hide resolved
@klihub klihub force-pushed the devel/crio-integration branch from a53df63 to b502a0b Compare January 27, 2022 15:24
Add functions for annotating containers for CDI device injection
and for extracting CDI device names from such annotations. These
functions can be used by CDI-aware devices plugins and runtimes,
to request the injection of CDI devices and to recover the names
of devices to inject correspondingly.

For example, the following call to AnnotateInjection
    annotations := map[string]string{}
    AnnotateInjection(annotations, "vendor.gpu", []string{
        "vendor.com/gpu=gpu0",
        "vendor.com/gpu=gpu1",
    })
produces the following annotations
    map[string]string{
        "cdi.k8s.io/vendor.gpu": "vendor.com/gpu=gpu0,vendor.com/gpu=gpu1",
    }

Subsequently parsing these annotations with
    devices, err := ParseAnnotations(annotations)
extracts and returns the original slice of devices to inject:
    []string{
        "vendor.com/gpu=gpu0",
        "vendor.com/gpu=gpu1",
    }

Notes:
  These functions provide a temporary solution for delivering
  CDI device injection requests from either a client or a K8s
  Device Plugin to the container runtime. They can be removed
  once dedicated fields to the same effect are added natively
  to the CRI protocol.

Signed-off-by: Krisztian Litkey <[email protected]>
Signed-off-by: Krisztian Litkey <[email protected]>
@klihub klihub force-pushed the devel/crio-integration branch from b502a0b to 10f8f04 Compare January 27, 2022 15:38
@elezar elezar self-requested a review January 29, 2022 08:45
Copy link
Contributor

@elezar elezar 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. Sorry for the delay.

@elezar elezar merged commit ee12044 into cncf-tags:master Jan 31, 2022
@klihub
Copy link
Contributor Author

klihub commented Jan 31, 2022

Thanks @klihub. Sorry for the delay.

No problemo, I think this was pretty fast. Thanks for the review !

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.

4 participants