Allow draining when DaemonSet kind has custom API Group #6412
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/assign @x13n @vadasambar |
| @@ -57,7 +58,10 @@ func (r *Rule) Drainable(drainCtx *drainability.DrainContext, pod *apiv1.Pod) dr | |||
| if controllerRef == nil { | |||
| return drainability.NewUndefinedStatus() | |||
| } | |||
| refKind := controllerRef.Kind | |||
|
|
|||
| groupVersionKind := schema.FromAPIVersionAndKind(controllerRef.APIVersion, controllerRef.Kind) | |||
There was a problem hiding this comment.
Docs for FromAPIVersionAndKind says:
// FromAPIVersionAndKind returns a GVK representing the provided fields for types that
// do not use TypeMeta. This method exists to support test types and legacy serializations
// that have a distinct group and kind.
// TODO: further reduce usage of this method.DaemonSet uses TypeMeta: https://pkg.go.dev/k8s.io/api/apps/v1#DaemonSet
Should we be using TypeMeta's GroupVersionKind() function here instead? GroupVersionKind() uses FromAPIVersionAndKInd() internally but there is less chance of GroupVersionKind() getting removed in the future (while FromAPIVersionKind() might)
Something like (at the cost of more verbosity):
v1.TypeMeta{Kind: controllerRef.Kind, APIVersion: controllerRef.APIVersion}.GroupVersionKind()
There was a problem hiding this comment.
How about:
refGroup := schema.ParseGroupVersion(controllerRef.APIVersion)
refKind := controllerRef.Kind
ParseGroupVersion isn't annotated as something that is planned for removal.
There was a problem hiding this comment.
Hi @vadasambar, @x13n
schema.ParseGroupVersion returns error in addition to schema.GroupVersion, will the following code be sufficient (which is similar to what we do for controllerRef)?
refGroup, err := schema.ParseGroupVersion(controllerRef.APIVersion)
if err != nil {
return drainability.NewUndefinedStatus()
}
refKind := controllerRef.KindOr should I return drainability.NewBlockedStatus instead?
There was a problem hiding this comment.
Undefined is ok in this case. If it didn't parse, it means it wasn't a correctly formatted APIVersion string.
There was a problem hiding this comment.
(In particular, if it doesn't parse, it means it is not apps/v1.)
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shamil, x13n The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind bug
What this PR does / why we need it:
The issue is described in #5977
Which issue(s) this PR fixes:
Fixes #5977
Suppresses #6328
Special notes for your reviewer:
This PR is a continuation from #6328. Made the changes as requested by @x13n
Does this PR introduce a user-facing change?
NONE