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
CSI migration doesn't count inline volumes for attach limit #107787
Conversation
@Jiawei0227: The label(s) In response to this:
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. |
@Jiawei0227: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
/retest |
1 similar comment
/retest |
37d9096
to
a442d7f
Compare
a442d7f
to
51b37e4
Compare
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.
Just one comment. Otherwise LGTM
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.
How come sig-storage is not an OWNER of this plugin. We did it for others, so consider sending a separate PR to do that.
b1bc50a
to
84de6a5
Compare
/lgtm |
/assign @alculquicondor |
Gentle ping. Any update on this? |
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.
Looking better.
@msau42 still good?
@@ -40,10 +40,12 @@ import ( | |||
// and perform translations from InTree PV's to CSI | |||
type InTreeToCSITranslator interface { | |||
IsPVMigratable(pv *v1.PersistentVolume) bool | |||
IsInlineMigratable(vol *v1.Volume) bool |
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.
So the implementations were already implementing these functions?
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.
return fmt.Errorf("error looking up provisioner name for volume: %v. Error Msg: %v", vol, err) | ||
} | ||
if !isCSIMigrationOn(csiNode, inTreeProvisionerName) { | ||
klog.V(5).InfoS("CSI Migration of provisioner is not enabled", "provisioner", inTreeProvisionerName) |
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.
klog.V(5).InfoS("CSI Migration of provisioner is not enabled", "provisioner", inTreeProvisionerName) | |
klog.V(5).InfoS("CSI Migration is not enabled for provisioner", "provisioner", inTreeProvisionerName) |
But also, we probably need more context. Any other keys that would be useful? We should at least have the pod.
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.
Done.
// Do translation for the in-tree volume. | ||
translatedPV, err := pl.translator.TranslateInTreeInlineVolumeToCSI(&vol, pod.Namespace) | ||
if err != nil || translatedPV == nil { | ||
return fmt.Errorf("Cannot convert volume(%v) from inline to csi.", vol) |
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.
Error have different rules 😅
https://github.com/golang/go/wiki/CodeReviewComments#error-strings
return fmt.Errorf("Cannot convert volume(%v) from inline to csi.", vol) | |
return fmt.Errorf("converting volume(%v) from inline to csi: %w", vol, err) |
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.
Although you should probably have a different error message for translatedPV == nil
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.
Done
Previsouly, when kube-scheduler schedule a pod, it does not take inline intree volume into account when CSI migration is enabled. This could lead to failures where pod scheduled to a node but volume attachment fails.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, Jiawei0227, msau42 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 |
/retest |
/lgtm Thanks! |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
It seems the non-csi predicate will count the in-line volume if csi migrate not enabled ,just as the code below: but the code in non-csi predicate don't deal with the in-line volume case. Could you show me where it handle the in-line volume limit in non-csi predicate? |
} | ||
if !isCSIMigrationOn(csiNode, inTreeProvisionerName) { | ||
klog.V(5).InfoS("CSI Migration is not enabled for provisioner", "provisioner", inTreeProvisionerName, | ||
"pod", klog.KObj(pod), "csiNode", csiNode.Name) |
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.
Is it possible under regular circumstances that csiNode is nil?
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.
csiNode is checked in L96. And the Lister implementation returns not found error
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.
It looks like the error is ignored here:
kubernetes/pkg/scheduler/framework/plugins/nodevolumelimits/csi.go
Lines 98 to 99 in cc68c06
// TODO: return the error once CSINode is created by default (2 releases) | |
klog.V(5).InfoS("Could not get a CSINode object for the node", "node", klog.KObj(node), "err", err) |
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.
Ah good catch. Yes this could be a problem. We do have logic that's supposed to block the node from being ready until CSINode is created, but there could still be ordering issues with the informers.
Can you open a bug?
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.
What does the "2 releases" refer to? Does this mean if we return the error it will break some releases?
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.
It's referring to max supported 2-version skew between the control plane and kubelet. When CSI migration for a plugin is GA in version X, then in version X+2 we can assume that kubelets will be on version X.
However, I think due to #115178, we realize now that autoscaler will encounter a nil CSINode always (because it doesn't simulate CSINode objects). So this comment is obsolete.
What type of PR is this?
/kind bug
What this PR does / why we need it:
CSI volume predicate should count inline volumes for migrated plugins. Currently it is not handled in the csi predicates.
This could lead to failures when there is a pod uses inline volume being scheduled to a node that already meets its maximum volume limit.
Which issue(s) this PR fixes:
Fixes # #100474
Special notes for your reviewer:
Testing:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/sig storage
/sig scheduler
/assign @msau42