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
Resource Claims must be a map type, not set #114585
Resource Claims must be a map type, not set #114585
Conversation
Thanks for tracking this down. I have no idea what What worries me is that this wasn't detected by any check in Kubernetes itself. If this annotation is required, shouldn't it be checked for in Kubernetes? |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
@@ -2328,6 +2328,9 @@ type ResourceRequirements struct { | |||
} | |||
|
|||
// ResourceClaim references one entry in PodSpec.ResourceClaims. | |||
// This struct must be atomic as it is used within a slice with the |
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.
Hmm, this line is going to be picked up in openapi, I wonder if I can hack this to prevent it being picked up there
In the past I've put +
at the beginning of the line to prevent generators including dev targeted comments, do we think that would be appropriate here?
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.
I don't think this should be allowed tbh, though that looks like something can be fixed by having listType=map
, listMapKey=name
?
With regards to validating these, yeah, we should do a better job.
https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy
I don't know if the validation is correct, it should probably only allow scalar types.
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.
I don't think this should be allowed tbh, though that looks like something can be fixed by having listType=map, listMapKey=name?
In a more complex example where you do want a set, eg with 3 or 4 members in the struct, would that then mean you have to make every field a map key to get the same semantic?
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.
I guess, but the keys have to be scalars too
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.
I don't think this should be allowed tbh, though that looks like something can be fixed by having listType=map, listMapKey=name?
Would this be your suggested path forward for this particular API then? Convert it to a map?
I think if we can add a check for this somewhere (not sure where yet) then that would help others down the line, perhaps the sig-apimachinery folks have some ideas on where best to stick that |
That's how I would write it if this was new code, but I don't know the history here, so it's a little concerning to change it. how long has this been broken? |
It was merged just over a month ago |
This API first appeared in 1.26.0. |
corev1.ResourceRequirements "claims" field has missing annotations that cause API server to reject it Can be reverted once kubernetes/kubernetes#114585 is fixed Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Includes a bump to controller-runtime and k8s API etc. since Gateway API bumped those as dependencies as well. Also inclused a hack for now to make API server happy corev1.ResourceRequirements "claims" field has missing annotations that cause API server to reject it Those changes can be reverted once kubernetes/kubernetes#114585 is fixed Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
I've just noticed controller-tools have put their own fix into the CRD generator kubernetes-sigs/controller-tools#753 It definitely seems to solve the issue, but I'm concerned given the conversation whether this is the right path forward? Should we recommend against this? Looking at the issue that it's linked to, this has been a direct result of the bug I'm trying to resolve |
Thanks for finding that. Where is the type here used? Is it used only in (currently broken) CRDs or is it used in built-in types? |
The type that's mentioned here, |
Yeah, probably :-( |
Given we think that the behaviour intended, would be the same with |
Looks good to me. The map type also matches possible future extensions with additional parameters attached to each name, which was the whole reason why |
In the event that those parameters were added, names would still be required to be unique in the list (as they are today), right? If so, type=map, key=name is the right approach |
Correct. |
/hold cancel |
looks like verify is happy. @JoelSpeed can you go ahead and pick this to release-1.26? |
Will the cherry pick bot work? Is there a reason not to use it? I'm away from my laptop but can manually pick later if need be |
cherry-pick bot isn't enabled for k/k, |
…14585-upstream-release-1.26 Automated cherry pick of #114585: Resource claims should be a map type
This was fixed upstream: kubernetes/kubernetes#114585
Includes a bump to controller-runtime and k8s API etc. since Gateway API bumped those as dependencies as well. Also inclused a hack for now to make API server happy corev1.ResourceRequirements "claims" field has missing annotations that cause API server to reject it Those changes can be reverted once kubernetes/kubernetes#114585 is fixed Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: yy <yang.yang@daocloud.io>
Includes a bump to controller-runtime and k8s API etc. since Gateway API bumped those as dependencies as well. Also inclused a hack for now to make API server happy corev1.ResourceRequirements "claims" field has missing annotations that cause API server to reject it Those changes can be reverted once kubernetes/kubernetes#114585 is fixed Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: yy <yang.yang@daocloud.io>
Includes a bump to controller-runtime and k8s API etc. since Gateway API bumped those as dependencies as well. Also inclused a hack for now to make API server happy corev1.ResourceRequirements "claims" field has missing annotations that cause API server to reject it Those changes can be reverted once kubernetes/kubernetes#114585 is fixed Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
What type of PR is this?
/kind bug
/kind api-change
What this PR does / why we need it:
This PR adds the
structType
to theResourceClaim
struct within thecorev1
API group.When the
structType
is not set, it defaults to granular (out of a choice of granular or atomic).Granular structs are not compatible with being used in a
listType=set
slice.This type was introduced for the
Claims
member ofResourceRequirements
which has thelistType=set
semantic.Without this fix, CRD schemas generated, including the
ResourceRequirements
struct are not valid and produce an error when installed into a cluster.Which issue(s) this PR fixes:
Currently, if you import the
corev1.ResourceRequirements
type into a custom resource definition, the generator generates an invalid schema, which results in errors such as below when you attempt to create the CRD.Special notes for your reviewer:
The validation error we are hitting is this piece of code.
It was introduced by @sttts in e4e1c2c, and we are specifically hitting the
object
case here. Looking at the PR, seems @apelisse may be the expert on why this validation was introduced and may be able to help ascertain that this is the correct fix.This validation sets a requirement that any
struct
used as the underlying type of a slice, must be an atomic struct when the slice itself is a set.The
ResourceRequirements
was recently updated to add the new field Claims in #111023 which then introduced this bug. CC @pohly. Tthis is a very obscure bug and I have no idea how anyone would know to set the struct tag correctly.I noticed while search k/k that there are many instances of this throughout the codebase where we have invalid combinations of
listType=set
and nostructType
set. We may want to fix the other instances too.CC @deads2k
Does this PR introduce a user-facing change?