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

RFC: KEP-4381: DRA: inline parameters #4613

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pohly
Copy link
Contributor

@pohly pohly commented May 2, 2024

This simplifies using the API for those cases where no vendor CRD is needed. The downside is that there are now multiple ways to specify in-tree parameters (inline and in separate object).

This simplifies using the API for those cases where no vendor CRD is
needed. The downside is that there are now multiple ways to specify
in-tree parameters (inline and in separate object).
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 2, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pohly
Once this PR has been reviewed and has the lgtm label, please assign derekwaynecarr for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 2, 2024
@@ -1376,6 +1385,10 @@ type ResourceClassParameters struct {
// to some unknown type.
GeneratedFrom *ResourceClassParametersReference

ResourceClassParametersSpec
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inlined, so the YAML is the same as before.

@@ -258,6 +258,10 @@ At a high-level, DRA with structured parameters takes the following form:
schedule a pod on (as well as allocate resources from its `ResourceSlice`
in the process).

* Alternatively, a `ResourceClaim` may also directly reference a
Copy link

Choose a reason for hiding this comment

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

So, if users want to embed ResourceClaimParameters into ResourceClaim or generate ResourceClaimParameters manually, they may need to write some CEL expressions to filter out resources, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

There's an ongoing debate between @johnbelamaric and @klueska whether asking users to write CEL expressions is going to be a good API. I suspect it is both: it's okay in some cases and too complex in others, so we'll need support for CRDs as an optional feature.

@@ -1535,6 +1553,10 @@ type ResourceClaimParameters struct {
// to some unknown type.
GeneratedFrom *ResourceClaimParametersReference

ResourceClaimParametersSpec
Copy link

Choose a reason for hiding this comment

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

This may be unrelated to this PR, but what happens to existing Pods if users later change the fields in the ResourceClaimParameter's embedded spec like selector? Just ignore it and keep running as is? I understand that ResourceClaim is immutable, so inline parameters cannot be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point: for the sake of feature parity, changing the ResourceClaim.Parameters content should be allowed.

In the current design, parameters matter right until allocation happens. After that, the parameters are ignored.

With classic DRA, a DRA driver could have watched parameters after allocation and somehow adapt the allocation result on-the-fly internally. This is not part of "structured parameters" at the moment. It's also not clear what that would mean for real-world hardware, so it's low priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the ResourceClaimSpec is already mutable. That's because even if it wasn't (i.e. ResourceClassName and ParametersRef don't change after creation), that still wouldn't prevent someone from changing the content of those objects or, if they were immutable themselves, delete and recreate them.

What is protected against undesirable changes is the status. In particular, the allocation result is immutable.

Copy link

Choose a reason for hiding this comment

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

That's because even if it wasn't (i.e. ResourceClassName and ParametersRef don't change after creation), that still wouldn't prevent someone from changing the content of those objects or, if they were immutable themselves, delete and recreate them.

I see, indeed, that perspective makes sense. It might be possible to protect deletions by setting the finalizer (dra.k8s.io/delete-protection) not only on the ResourceClaim but also on the ResourceClaimParameters referenced by ResourceClaim.

Anyway, for now, it seems better to make the ResourceClaimParametersSpec immutable if the allocation result of ResourceClaim cannot be changed and changes to already allocated resources are ignored. I think we should avoid behavior that confuses users. If a use case arises where users want to modify allocated resources by changing parameters, we can simply change the field from immutable to mutable, as shown in https://github.com/kubernetes/kubernetes/blob/v1.30.0/pkg/apis/storage/types.go#L287.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be possible to protect deletions by setting the finalizer (dra.k8s.io/delete-protection) not only on the ResourceClaim but also on the ResourceClaimParameters referenced by ResourceClaim.

I'm not sure who should be doing this and when it's safe to remove the finalizer again. A finalizer also doesn't make the content immutable.

it seems better to make the ResourceClaimParametersSpec immutable if the allocation result of ResourceClaim cannot be changed and changes to already allocated resources are ignored

This seems like a valid usage:

  • create claim with parameters
  • create a pod using the claim -> allocated
  • change parameters
  • kill pod to get it started again -> deallocated (because allocations get removed as soon as the claim is unused)
  • recreate pod -> allocated with new parameters

From a conceptual perspective it seems odd to me to treat inline parameters differently from referenced parameters.

Copy link

Choose a reason for hiding this comment

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

From a conceptual perspective it seems odd to me to treat inline parameters differently from referenced parameters.

Yes, I thought that referenced ResourceClaimParameters should be made immutable. However, I was convinced by the use cases mentioned above. It's certainly cumbersome to have to recreate ResourceClaim or ResourceClaimParameters 😄
Currently, ResourceClaimSpec is immutable, but only inline parameters will be mutable, right?
https://github.com/kubernetes/kubernetes/blob/v1.30.0/pkg/apis/resource/validation/validation.go#L112

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, how did I miss that ("Actually, the ResourceClaimSpec is already mutable.")? You are right, it is not.

As I argued above, I think we have to accept that claim parameters are mutable and should allow that consistently everywhere. That includes making the ResourceClaimSpec mutable. The KEP and API description don't describe this anywhere - need to fix that!

@bart0sh bart0sh added this to Needs Reviewer in SIG Node PR Triage May 4, 2024
pohly added a commit to pohly/wg-device-management that referenced this pull request May 13, 2024
This proposal takes the existing KEP as base and includes:
- vendor-independent classes and attributes (kubernetes/enhancements#4614)
- optional allocation (kubernetes/enhancements#4619)
- inline parameters (kubernetes/enhancements#4613)
- management access (kubernetes/enhancements#4611)
- renaming "named resources" to "devices" wherever it makes sense and is
  user-facing (Slack discussion)
- MatchAttributes (from k8srm-prototype)
- OneOf (from k8srm-prototype)

`pkg/api` currently builds, but the rest doesn't. None of the YAML examples
have been updated yet.
pohly added a commit to pohly/wg-device-management that referenced this pull request May 13, 2024
This proposal takes the existing KEP as base and includes:
- vendor-independent classes and attributes (kubernetes/enhancements#4614)
- optional allocation (kubernetes/enhancements#4619)
- inline parameters (kubernetes/enhancements#4613)
- management access (kubernetes/enhancements#4611)
- renaming "named resources" to "devices" wherever it makes sense and is
  user-facing (Slack discussion)
- MatchAttributes (from k8srm-prototype)
- OneOf (from k8srm-prototype)

`pkg/api` currently builds, but the rest doesn't. None of the YAML examples
have been updated yet.
pohly added a commit to pohly/wg-device-management that referenced this pull request May 13, 2024
This proposal takes the existing KEP as base and includes:
- vendor-independent classes and attributes (kubernetes/enhancements#4614)
- optional allocation (kubernetes/enhancements#4619)
- inline parameters (kubernetes/enhancements#4613)
- management access (kubernetes/enhancements#4611)
- renaming "named resources" to "devices" wherever it makes sense and is
  user-facing (Slack discussion)
- MatchAttributes (from k8srm-prototype)
- OneOf (from k8srm-prototype)

`pkg/api` currently builds, but the rest doesn't. None of the YAML examples
have been updated yet.
pohly added a commit to pohly/wg-device-management that referenced this pull request May 13, 2024
This proposal takes the existing KEP as base and includes:
- vendor-independent classes and attributes (kubernetes/enhancements#4614)
- optional allocation (kubernetes/enhancements#4619)
- inline parameters (kubernetes/enhancements#4613)
- management access (kubernetes/enhancements#4611)
- renaming "named resources" to "devices" wherever it makes sense and is
  user-facing (Slack discussion)
- MatchAttributes (from k8srm-prototype)
- OneOf (from k8srm-prototype)

`pkg/api` currently builds, but the rest doesn't. None of the YAML examples
have been updated yet.
pohly added a commit to pohly/wg-device-management that referenced this pull request May 16, 2024
This proposal takes the existing KEP as base and includes:
- vendor-independent classes and attributes (kubernetes/enhancements#4614)
- optional allocation (kubernetes/enhancements#4619)
- inline parameters (kubernetes/enhancements#4613)
- management access (kubernetes/enhancements#4611)
- renaming "named resources" to "devices" wherever it makes sense and is
  user-facing (Slack discussion)
- MatchAttributes (from k8srm-prototype)
- OneOf (from k8srm-prototype)

`pkg/api` currently builds, but the rest doesn't. None of the YAML examples
have been updated yet.
pohly added a commit to pohly/wg-device-management that referenced this pull request May 16, 2024
This proposal takes the existing KEP as base and includes:
- vendor-independent classes and attributes (kubernetes/enhancements#4614)
- optional allocation (kubernetes/enhancements#4619)
- inline parameters (kubernetes/enhancements#4613)
- management access (kubernetes/enhancements#4611)
- renaming "named resources" to "devices" wherever it makes sense and is
  user-facing (Slack discussion)
- MatchAttributes (from k8srm-prototype)
- OneOf (from k8srm-prototype)

`pkg/api` currently builds, but the rest doesn't. None of the YAML examples
have been updated yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
Status: Needs Reviewer
SIG Node PR Triage
Needs Reviewer
Development

Successfully merging this pull request may close these issues.

None yet

3 participants