Add QueueingHintFn for pvc events in VolumeRestriction plugin #125280

HirazawaUi · 2024-06-02T14:07:29Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes Part of #118893

Special notes for your reviewer:

For review purposes, I splitd #119405.
this PR for adding QueueingHintFn for pvc events in VolumeRestriction plugin.

Does this PR introduce a user-facing change?

kube-scheduler implements scheduling hints for the VolumeRestriction plugin.
Scheduling hints allow the scheduler to retry scheduling Pods that were previously rejected by the VolumeRestriction plugin if a new pvc added, and the pvc belongs to pod.

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

k8s-ci-robot · 2024-06-02T14:07:38Z

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-sigs/prow repository.

HirazawaUi · 2024-06-02T14:15:33Z

/cc @sanposhiho

HirazawaUi · 2024-06-02T16:10:28Z

/retest

AxeZhan · 2024-06-03T06:31:25Z

pkg/scheduler/framework/plugins/volumerestrictions/volume_restrictions.go

+
+	pvcs := sets.New[string]()
+
+	for _, volume := range pod.Spec.Volumes {


We should only check volumes which can be rejected by volumeRestriction.

kubernetes/pkg/scheduler/framework/plugins/volumerestrictions/volume_restrictions.go

Lines 157 to 159 in 2d8a3ad

func needsRestrictionsCheck(v v1.Volume) bool {

return v.GCEPersistentDisk != nil || v.AWSElasticBlockStore != nil || v.RBD != nil || v.ISCSI != nil

}

I think that means we should only check these 4 types of volumes and pvc with readWriteOncePod accessmode?

And since we only cares PVC in this function, I think we should only do insertion in L365, if this pvc is a readWriteOncePod pvc.

The current code will function correctly. However, the purpose of QHInt is to allow the plugin to determine if a previously rejected pod can potentially pass through it again. If the PVC has not been created, it will not be rejected by VolumeRestriction (I'm not sure but maybe volume_binding?). Although adding this check will not make the performance faster, I believe we should maintain consistent behavior.

Sorry, I checked the code. And we DO check this in code:

kubernetes/pkg/scheduler/framework/plugins/volumerestrictions/volume_restrictions.go

Lines 249 to 252 in 548d50d

pvc, err := pl.pvcLister.PersistentVolumeClaims(pod.Namespace).Get(volume.PersistentVolumeClaim.ClaimName)

if err != nil {

return nil, err

}

So never mind. Let's keep the behavior for now😅.

Thank you for your careful review :)

carlory · 2024-06-04T03:00:21Z

pkg/scheduler/framework/plugins/volumerestrictions/volume_restrictions.go

@@ -326,10 +328,50 @@ func (pl *VolumeRestrictions) EventsToRegister() []framework.ClusterEventWithHin
 		{Event: framework.ClusterEvent{Resource: framework.Node, ActionType: framework.Add}},
 		// Pods may fail to schedule because the PVC it uses has not yet been created.
 		// This PVC is required to exist to check its access modes.
-		{Event: framework.ClusterEvent{Resource: framework.PersistentVolumeClaim, ActionType: framework.Add | framework.Update}},
+		{Event: framework.ClusterEvent{Resource: framework.PersistentVolumeClaim, ActionType: framework.Add | framework.Update},


Is it safe to remove the PVC's update event? The current Hint function will always ignore the event. After reading the plugin implementation, no update event registered makes sense to me.

I'd like to hear about others' opinions. @sanposhiho @AxeZhan

Related to #122158

The access modes can not be changed after a pvc is created.

Unlike node events, the scheduler never ignore the creation event of a pvc.

kubernetes/pkg/scheduler/eventhandlers.go

Line 441 in 6938c29

// MaxPDVolumeCountPredicate: add/update PVC will affect counts of PV when it is bound.

I'm +1 for removing update event, since related changes to the PVC spec are blocked by immutability protection.

Not sure why we added it first, the comments said it's a future-proof, need opinions from reviewers of #103082

/cc @alculquicondor @chrishenzie @ahg-g

/cc @jsafrane @xing-yang

Can you take a look?

https://github.com/kubernetes/kubernetes/pull/103082/files#r661392954

The question is whether anything has changed since that comment was made, whether any change to the PVC spec or status could cause a Pod to schedule.

ping @jsafrane @xing-yang

The only PVC field that this filter uses is Spec.AccessModes and it's indeed immutable:

kubernetes/pkg/apis/core/validation/validation.go

Line 2402 in 1589016

allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), fmt.Sprintf("spec is immutable after creation except resources.requests and volumeAttributesClassName for bound claims\n%v", specDiff)))

/approve
/hold cancel

Please remove Update, given, that, effectively, it doesn't matter.

removed, after this PR merge we close #122158 as we no longer need it.

pkg/scheduler/framework/plugins/volumerestrictions/volume_restrictions.go

pkg/scheduler/framework/plugins/volumerestrictions/volume_restrictions_test.go

sanposhiho

Thanks for all the fix, my /lgtm for it will come after the decision in https://github.com/kubernetes/kubernetes/pull/125280/files#r1625269171.

HirazawaUi · 2024-06-23T16:46:55Z

Thanks for all the fix, my /lgtm for it will come after the decision in https://github.com/kubernetes/kubernetes/pull/125280/files#r1625269171.

We can discuss this separately in #122158 :)

pkg/scheduler/framework/plugins/volumerestrictions/volume_restrictions.go

HirazawaUi · 2024-07-09T14:55:16Z

/retest

HirazawaUi · 2024-07-12T15:26:27Z

@sanposhiho Do you think we can add lgtm to it now.

alculquicondor

/lgtm
/approve

k8s-ci-robot · 2024-07-12T17:42:06Z

LGTM label has been added.

Git tree hash: 3a56488ff4c2912f8f6b7c6090d0a5ce08893e2e

k8s-ci-robot · 2024-07-12T17:42:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, HirazawaUi, jsafrane

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/framework/plugins/volumerestrictions/OWNERS~~ [alculquicondor,jsafrane]

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 carlory and saikat-royc June 2, 2024 14:08

k8s-ci-robot requested a review from sanposhiho June 2, 2024 14:15

AxeZhan reviewed Jun 3, 2024

View reviewed changes

carlory reviewed Jun 4, 2024

View reviewed changes

k8s-ci-robot requested review from ahg-g, alculquicondor and chrishenzie June 4, 2024 04:45

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2024

sanposhiho reviewed Jun 15, 2024

View reviewed changes

pkg/scheduler/framework/plugins/volumerestrictions/volume_restrictions.go Outdated Show resolved Hide resolved

HirazawaUi force-pushed the add-pvc-events-queueinghintfn branch from 676888d to d500e99 Compare June 15, 2024 16:04

sanposhiho reviewed Jun 17, 2024

View reviewed changes

HirazawaUi force-pushed the add-pvc-events-queueinghintfn branch from d500e99 to 6c10218 Compare June 17, 2024 14:33

k8s-ci-robot requested review from jsafrane and xing-yang June 18, 2024 08:51

sanposhiho reviewed Jun 18, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2024

HirazawaUi force-pushed the add-pvc-events-queueinghintfn branch from 6c10218 to 0fbf3e8 Compare June 23, 2024 13:37

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2024

sanposhiho reviewed Jun 24, 2024

View reviewed changes

pkg/scheduler/framework/plugins/volumerestrictions/volume_restrictions.go Outdated Show resolved Hide resolved

HirazawaUi force-pushed the add-pvc-events-queueinghintfn branch from 0fbf3e8 to 32ee19c Compare July 9, 2024 11:38

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 11, 2024

Add QueueingHintFn for pvc events in VolumeRestriction plugin

cd13be8

HirazawaUi force-pushed the add-pvc-events-queueinghintfn branch from 32ee19c to cd13be8 Compare July 12, 2024 16:25

alculquicondor reviewed Jul 12, 2024

View reviewed changes

k8s-ci-robot assigned alculquicondor Jul 12, 2024

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

k8s-ci-robot merged commit c7dab2a into kubernetes:master Jul 12, 2024
14 checks passed

k8s-ci-robot added this to the v1.31 milestone Jul 12, 2024

HirazawaUi mentioned this pull request Jul 13, 2024

Remove pvc update events from volume restriction #122158

Closed

carlory mentioned this pull request Jul 22, 2024

Fix a bug where QHint doesn't requeue Pods correctly if a deleted pod uses the same PVC with the ReadWriteOncePod access mode #126263

Merged

sanposhiho mentioned this pull request Sep 1, 2024

[Umbrella] Implement QueueingHintFn in in-tree plugins #118893

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add QueueingHintFn for pvc events in VolumeRestriction plugin #125280

Add QueueingHintFn for pvc events in VolumeRestriction plugin #125280

HirazawaUi commented Jun 2, 2024

k8s-ci-robot commented Jun 2, 2024

HirazawaUi commented Jun 2, 2024

HirazawaUi commented Jun 2, 2024

AxeZhan Jun 3, 2024 •

edited

Loading

AxeZhan Jun 3, 2024 •

edited

Loading

HirazawaUi Jun 3, 2024

carlory Jun 4, 2024

carlory Jun 4, 2024

carlory Jun 4, 2024 •

edited

Loading

AxeZhan Jun 4, 2024

carlory Jun 4, 2024

carlory Jun 18, 2024

alculquicondor Jun 24, 2024

jsafrane Jul 11, 2024

alculquicondor Jul 12, 2024

HirazawaUi Jul 12, 2024

sanposhiho left a comment

HirazawaUi commented Jun 23, 2024

HirazawaUi commented Jul 9, 2024

HirazawaUi commented Jul 12, 2024

alculquicondor left a comment

k8s-ci-robot commented Jul 12, 2024

k8s-ci-robot commented Jul 12, 2024


		pvcs := sets.New[string]()

		for _, volume := range pod.Spec.Volumes {

	func needsRestrictionsCheck(v v1.Volume) bool {
	return v.GCEPersistentDisk != nil \|\| v.AWSElasticBlockStore != nil \|\| v.RBD != nil \|\| v.ISCSI != nil
	}

	pvc, err := pl.pvcLister.PersistentVolumeClaims(pod.Namespace).Get(volume.PersistentVolumeClaim.ClaimName)
	if err != nil {
	return nil, err
	}

Add QueueingHintFn for pvc events in VolumeRestriction plugin #125280

Add QueueingHintFn for pvc events in VolumeRestriction plugin #125280

Conversation

HirazawaUi commented Jun 2, 2024

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 Jun 2, 2024

HirazawaUi commented Jun 2, 2024

HirazawaUi commented Jun 2, 2024

AxeZhan Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

AxeZhan Jun 3, 2024 • edited Loading

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

carlory Jun 4, 2024 • edited Loading

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

sanposhiho left a comment

Choose a reason for hiding this comment

HirazawaUi commented Jun 23, 2024

HirazawaUi commented Jul 9, 2024

HirazawaUi commented Jul 12, 2024

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 12, 2024

k8s-ci-robot commented Jul 12, 2024

AxeZhan Jun 3, 2024 •

edited

Loading

AxeZhan Jun 3, 2024 •

edited

Loading

carlory Jun 4, 2024 •

edited

Loading