generic ephemeral volume checks #100482

pohly · 2021-03-23T11:01:45Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

When adding the ephemeral volume feature, the special case for
PersistentVolumeClaim volume sources was overlooked in three places:

host path check in kubelet
node limit check in the scheduler
volume zone

Of these, volume zone is deprecated an doesn't need to be changed to support ephemeral volumes.

Special notes for your reviewer:

@msau42 pointed out the gap in nodevolumelimits on chat. The other two places where found with:

git grep -i '\(name\|pvc\).*=.*ClaimName'

Does this PR introduce a user-facing change?

Generic ephemeral volumes were not considered properly by the the node limits scheduler filter and the kubelet hostpath check.

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

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/1698-generic-ephemeral-volumes

pohly · 2021-03-23T11:32:10Z

I can also split up this PR into three different PRs, one per modified directory.

pohly · 2021-03-23T12:42:30Z

/retest

damemi

A couple comments on general code patterns, I don't know the full context of this featuregate so I will have to look more into it but this looks like the right approach overall

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

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

pkg/scheduler/framework/plugins/volumezone/volume_zone.go

ehashman · 2021-04-07T17:55:49Z

/triage accepted
/priority important-longterm

ehashman · 2021-04-21T17:25:26Z

/cc @msau42
/assign @damemi
for scheduling

/assign @SergeyKanzhelev
for node

gnufied · 2021-09-22T17:16:23Z

/assign
/lgtm cancel

pohly · 2021-09-27T15:44:17Z

I had to rebase because the feature gate package imports in pkg/kubelet/kubelet_pods_test.go had been removed and I need them again.

ehashman · 2021-09-27T16:51:44Z

@msau42 can the hold be removed?

msau42 · 2021-09-28T16:26:27Z

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

+				// The PVC is required to proceed with
+				// scheduling of a new pod because it cannot
+				// run without it. Bail out immediately.
+				return fmt.Errorf("look up PVC %s/%s: %v", pod.Namespace, pvcName, err)


should we provide a little more context to the error message, like "failed to ..."?

I tend to write error messages as <operation>: <error>. Then if it gets wrapped higher up, we don't accumulate those "failed to " comments. I've seen errors which were something like:

ERROR: failed to <do abc>: failed to <do xyz>: could not <do fgh>: original error here

That always seemed a bit redundant to me. But let me add it here.... done.

We tend to say "looking up PVC ...." because it is clear this is an error from the context, so adding "failed to" is redundant.

I like "looking up PVC" best, so I've switched to that.

@msau42 okay? I still need LGTM for this PR.

msau42 · 2021-09-28T16:32:15Z

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

+				return fmt.Errorf("look up PVC %s/%s: %v", pod.Namespace, pvcName, err)
+			}
+			// If the PVC is invalid, we don't count the volume because
+			// there's no guarantee that it belongs to the running predicate.


opened #105328 to followup on if we should ignore or count them.

msau42 · 2021-09-28T16:32:31Z

/hold cancel

msau42 · 2021-09-29T17:58:23Z

/lgtm

klueska

Small comment, but otherwise looks good from a sig-node / kubelet perspective.

klueska · 2021-10-01T14:50:11Z

pkg/kubelet/kubelet_pods.go

+		}
+		pvc, err := kl.kubeClient.CoreV1().PersistentVolumeClaims(pod.Namespace).Get(context.TODO(), pvcName, metav1.GetOptions{})
+		if err != nil {
+			klog.InfoS("Unable to retrieve pvc", "pvc", klog.KRef(pod.Namespace, volume.PersistentVolumeClaim.ClaimName), "err", err)


Do you want to use pcvName instead of volume.PersistentVolumeClaim.ClaimName in this log message?

When adding the ephemeral volume feature, the special case for PersistentVolumeClaim volume sources in kubelet's host path and node limits checks was overlooked. An ephemeral volume source is another way of referencing a claim and has to be treated the same way.

Previously, the situation was ignored, which might have had the effect that Pod scheduling continued (?) even though the Pod+PVC weren't known to be in an acceptable state.

klueska · 2021-10-01T15:05:22Z

/lgtm
/approve

k8s-ci-robot · 2021-10-01T15:06:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, klueska, msau42, pohly

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/kubelet/OWNERS~~ [klueska]
~~pkg/scheduler/OWNERS~~ [ahg-g]
~~test/e2e/storage/OWNERS~~ [msau42,pohly]

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 requested review from damemi and odinuge March 23, 2021 11:02

damemi reviewed Mar 23, 2021

View reviewed changes

pohly force-pushed the generic-ephemeral-volume-checks branch from 28be27d to debff10 Compare March 23, 2021 16:47

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 23, 2021

ehashman added this to Triage in SIG Node PR Triage Mar 27, 2021

ehashman moved this from Triage to Needs Reviewer in SIG Node PR Triage Apr 7, 2021

k8s-ci-robot assigned damemi and SergeyKanzhelev Apr 21, 2021

k8s-ci-robot requested a review from msau42 April 21, 2021 17:25

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2021

pohly force-pushed the generic-ephemeral-volume-checks branch 2 times, most recently from 27937bc to 83b991d Compare September 27, 2021 15:43

ehashman moved this from Waiting on Author to Needs Reviewer in SIG Node PR Triage Sep 27, 2021

msau42 mentioned this pull request Sep 28, 2021

Volume limits predicate handling of other pods with misconfigured pvs #105328

Closed

2 tasks

msau42 reviewed Sep 28, 2021

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2021

pohly force-pushed the generic-ephemeral-volume-checks branch 2 times, most recently from f60ed36 to 3e7947e Compare September 29, 2021 05:55

pohly mentioned this pull request Sep 29, 2021

generic ephemeral volume util, base code and controller #105345

Merged

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2021

pacoxu moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Sep 30, 2021

klueska reviewed Oct 1, 2021

View reviewed changes

pohly added 2 commits October 1, 2021 17:03

scheduler: fail the volume attach limit when PVC is missing

1d181ad

Previously, the situation was ignored, which might have had the effect that Pod scheduling continued (?) even though the Pod+PVC weren't known to be in an acceptable state.

pohly force-pushed the generic-ephemeral-volume-checks branch from 3e7947e to 1d181ad Compare October 1, 2021 15:03

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2021

k8s-ci-robot assigned klueska Oct 1, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2021

k8s-ci-robot merged commit e414cf7 into kubernetes:master Oct 1, 2021

SIG Node PR Triage automation moved this from Needs Approver to Done Oct 1, 2021

k8s-ci-robot added this to the v1.23 milestone Oct 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generic ephemeral volume checks #100482

generic ephemeral volume checks #100482

pohly commented Mar 23, 2021 •

edited

pohly commented Mar 23, 2021

pohly commented Mar 23, 2021

damemi left a comment

ehashman commented Apr 7, 2021

ehashman commented Apr 21, 2021

gnufied commented Sep 22, 2021

pohly commented Sep 27, 2021

ehashman commented Sep 27, 2021

msau42 Sep 28, 2021

pohly Sep 28, 2021 •

edited

ahg-g Sep 28, 2021

pohly Sep 29, 2021

msau42 Sep 28, 2021

msau42 commented Sep 28, 2021

msau42 commented Sep 29, 2021

klueska left a comment

klueska Oct 1, 2021

klueska commented Oct 1, 2021

k8s-ci-robot commented Oct 1, 2021

generic ephemeral volume checks #100482

generic ephemeral volume checks #100482

Conversation

pohly commented Mar 23, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

pohly commented Mar 23, 2021

pohly commented Mar 23, 2021

damemi left a comment

Choose a reason for hiding this comment

ehashman commented Apr 7, 2021

ehashman commented Apr 21, 2021

gnufied commented Sep 22, 2021

pohly commented Sep 27, 2021

ehashman commented Sep 27, 2021

msau42 Sep 28, 2021

Choose a reason for hiding this comment

pohly Sep 28, 2021 • edited

Choose a reason for hiding this comment

ahg-g Sep 28, 2021

Choose a reason for hiding this comment

pohly Sep 29, 2021

Choose a reason for hiding this comment

msau42 Sep 28, 2021

Choose a reason for hiding this comment

msau42 commented Sep 28, 2021

msau42 commented Sep 29, 2021

klueska left a comment

Choose a reason for hiding this comment

klueska Oct 1, 2021

Choose a reason for hiding this comment

klueska commented Oct 1, 2021

k8s-ci-robot commented Oct 1, 2021

pohly commented Mar 23, 2021 •

edited

pohly Sep 28, 2021 •

edited