CSI migration doesn't count inline volumes for attach limit #107787

Jiawei0227 · 2022-01-26T10:05:06Z

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:

Unit tests
E2E tests by inspecting logs added to the CSI volume predicate.

Does this PR introduce a user-facing change?

Fix a bug where CSI migration doesn't count inline volumes for attach limit.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig storage
/sig scheduler
/assign @msau42

k8s-ci-robot · 2022-01-26T10:05:11Z

@Jiawei0227: The label(s) sig/scheduler cannot be applied, because the repository doesn't have them.

In response to this:

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.

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:

Unit tests

E2E tests by inspecting logs added to the CSI volume predicate.

Does this PR introduce a user-facing change?
Fix a bug where CSI migration doesn't count inline volumes for attach limit.
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/sig storage
/sig scheduler
/assign @msau42

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.

k8s-ci-robot · 2022-01-26T10:05:13Z

@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 triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 · 2022-01-26T10:07:34Z

/cc @dobsonj @jsafrane
/sig scheduling

Jiawei0227 · 2022-01-26T16:31:01Z

/retest

Jiawei0227 · 2022-01-26T19:50:30Z

/retest

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

pkg/scheduler/framework/plugins/nodevolumelimits/csi_test.go

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

msau42

Just one comment. Otherwise LGTM

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

alculquicondor

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.

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

msau42 · 2022-03-17T02:36:28Z

/lgtm
/approve

msau42 · 2022-03-17T02:36:55Z

/assign @alculquicondor

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

Jiawei0227 · 2022-05-03T23:03:03Z

Gentle ping. Any update on this?

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

alculquicondor

Looking better.

@msau42 still good?

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

alculquicondor · 2022-05-05T21:32:57Z

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

@@ -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


So the implementations were already implementing these functions?

Yes. https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/csi-translation-lib/translate.go#L197

alculquicondor · 2022-05-05T21:34:12Z

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

+		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)


Suggested change

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.

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

alculquicondor · 2022-05-05T21:37:45Z

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

+	// 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)


Error have different rules 😅

https://github.com/golang/go/wiki/CodeReviewComments#error-strings

Suggested change

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)

Although you should probably have a different error message for translatedPV == nil

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

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.

k8s-ci-robot · 2022-05-10T16:04:05Z

[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:

~~pkg/scheduler/OWNERS~~ [alculquicondor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alculquicondor · 2022-05-10T16:08:02Z

/retest

msau42 · 2022-05-12T03:23:10Z

/lgtm

Thanks!

k8s-triage-robot · 2022-05-12T05:36:24Z

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:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

b0b0haha · 2022-11-23T08:50:47Z

It seems the non-csi predicate will count the in-line volume if csi migrate not enabled ,just as the code below:
https://github.com/Jiawei0227/kubernetes/blob/9d01f0bf52891d777078daafcd146b6eea1c98eb/pkg/scheduler/framework/plugins/nodevolumelimits/csi.go#L169-L180

but the code in non-csi predicate don't deal with the in-line volume case.
https://github.com/Jiawei0227/kubernetes/blob/9d01f0bf52891d777078daafcd146b6eea1c98eb/pkg/scheduler/framework/plugins/nodevolumelimits/non_csi.go#L298

Could you show me where it handle the in-line volume limit in non-csi predicate?
@Jiawei0227

alculquicondor · 2023-01-18T17:03:11Z

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

+	}
+	if !isCSIMigrationOn(csiNode, inTreeProvisionerName) {
+		klog.V(5).InfoS("CSI Migration is not enabled for provisioner", "provisioner", inTreeProvisionerName,
+			"pod", klog.KObj(pod), "csiNode", csiNode.Name)


Is it possible under regular circumstances that csiNode is nil?

csiNode is checked in L96. And the Lister implementation returns not found error

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)

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?

What does the "2 releases" refer to? Does this mean if we return the error it will break some releases?

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.

k8s-ci-robot assigned msau42 Jan 26, 2022

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jan 26, 2022

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jan 26, 2022

k8s-ci-robot requested review from alculquicondor and denkensk January 26, 2022 10:07

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Jan 26, 2022

k8s-ci-robot requested review from dobsonj and jsafrane January 26, 2022 10:07

msau42 reviewed Jan 29, 2022

View reviewed changes

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go Outdated Show resolved Hide resolved

pkg/scheduler/framework/plugins/nodevolumelimits/csi_test.go Show resolved Hide resolved

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go Outdated Show resolved Hide resolved

Jiawei0227 force-pushed the volumelimit branch 3 times, most recently from 37d9096 to a442d7f Compare January 29, 2022 08:41

binacs reviewed Jan 30, 2022

View reviewed changes

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go Outdated Show resolved Hide resolved

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go Outdated Show resolved Hide resolved

Jiawei0227 force-pushed the volumelimit branch from a442d7f to 51b37e4 Compare February 8, 2022 03:09

msau42 reviewed Mar 10, 2022

View reviewed changes

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go Outdated Show resolved Hide resolved

alculquicondor reviewed Mar 10, 2022

View reviewed changes

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go Outdated Show resolved Hide resolved

Jiawei0227 force-pushed the volumelimit branch 2 times, most recently from b1bc50a to 84de6a5 Compare March 16, 2022 06:20

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2022

k8s-ci-robot assigned alculquicondor Mar 17, 2022

alculquicondor reviewed Mar 17, 2022

View reviewed changes

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go Outdated Show resolved Hide resolved

alculquicondor reviewed May 4, 2022

View reviewed changes

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go Outdated Show resolved Hide resolved

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go Outdated Show resolved Hide resolved

Jiawei0227 force-pushed the volumelimit branch from 84de6a5 to a8e6e18 Compare May 5, 2022 21:15

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2022

alculquicondor reviewed May 5, 2022

View reviewed changes

Count inline volume for NodeVolumeLimit when CSI migration enabled

9d01f0b

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.

Jiawei0227 force-pushed the volumelimit branch from a8e6e18 to 9d01f0b Compare May 10, 2022 07:43

alculquicondor approved these changes May 10, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2022

k8s-ci-robot merged commit 8aaea8e into kubernetes:master May 12, 2022

k8s-ci-robot added this to the v1.25 milestone May 12, 2022

github-actions bot mentioned this pull request May 17, 2022

Week Ending May 15, 2022 dev-obs/actus#438

Open

alculquicondor reviewed Jan 18, 2023

View reviewed changes

alculquicondor mentioned this pull request Jan 18, 2023

Seg Fault in nodeVolumeLimits of scheduler plugin causing panic #115178

Closed

msau42 mentioned this pull request Jan 19, 2023

Fix nil pointer error in nodevolumelimits csi logging #115179

Merged

	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)

	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)

	// 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)

CSI migration doesn't count inline volumes for attach limit #107787

CSI migration doesn't count inline volumes for attach limit #107787

Conversation

Jiawei0227 commented Jan 26, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Jan 26, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Jan 26, 2022

Jiawei0227 commented Jan 26, 2022

Jiawei0227 commented Jan 26, 2022

Jiawei0227 commented Jan 26, 2022

msau42 left a comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

msau42 commented Mar 17, 2022

msau42 commented Mar 17, 2022

Jiawei0227 commented May 3, 2022

alculquicondor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 10, 2022

alculquicondor commented May 10, 2022

msau42 commented May 12, 2022

k8s-triage-robot commented May 12, 2022

b0b0haha commented Nov 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor Jan 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jiawei0227 commented Jan 26, 2022 •

edited

alculquicondor Jan 18, 2023 •

edited