volumebinding: scheduler queueing hints - StorageClass

[bells17]

What type of PR is this?

/kind feature

What this PR does / why we need it:

kube-scheduler implements scheduling hints for the VolumeBinding plugin.
The scheduling hints allow the scheduler to determine whether to retry or skip scheduling a Pod based on the changes made to the StorageClass resource referenced by the plugin.

Which issue(s) this PR fixes:

Part of #118893
KEP: https://github.com/kubernetes/enhancements/blob/master/keps/sig-scheduling/4247-queueinghint/README.md
Base PR: #124939

Special notes for your reviewer:

Fields Impacting QueueingHintFn

PersistentVolume (PV) is not included in this table because it can undergo extensive changes when a conversion is performed by csi-translation-lib.

resource	field	Referenced in PreFilter+Filter?	Admission Overwrite Prevention Config	Need to Check Changes in QHint?
StorageClass	.metadata.labels	x	x	x
StorageClass	.metadata.annotations	x	x	x
StorageClass	.provisioner	o	o	x
StorageClass	.parameters	x	o	x
StorageClass	.reclaimPolicy	x	o	x
StorageClass	.mountOptions	x	o	x
StorageClass	.allowVolumeExpansion	x	x	x
StorageClass	.volumeBindingMode	o	x	o
StorageClass	.allowedTopologies	o	x	o

ref(ja): https://zenn.dev/bells17/scraps/65bd6891012bdc

Does this PR introduce a user-facing change?

kube-scheduler implements scheduling hints for the VolumeBinding plugin.
The scheduling hints allow the scheduler to retry scheduling a Pod that was previously rejected by the VolumeBinding plugin only if a new resource referenced by the plugin was created or an existing resource referenced by the plugin was updated.

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

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bells17
Once this PR has been reviewed and has the lgtm label, please assign saad-ali for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• pkg/scheduler/framework/plugins/volumebinding/OWNERS

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]

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.

[bells17]

/cc @sanposhiho @utam0k

[bells17]

/retest

[sanposhiho]

/kind feature

[bells17]

/retest

[bells17]

/cc @carlory

[bells17]

/cc @jsafrane @xing-yang

[carlory]

It's forbidden to update the volume binding mode of a storage class. If a storage class's binding mode isn't VolumeBindingWaitForFirstConsumer, when it is created or updated, the action itself doesn't make the pod become schedulable directly. If then some PVCs is bound by PVs due to the storage class (re-)created, it should be reflected in the PVC update event. so in this function, we should only take care of special storage classes with the VolumeBindingWaitForFirstConsumer binding mode.

[carlory]

If we only consider VolumeBindingWaitForFirstConsumer, this PR will depend on the other hint func which handle PVC events.

So let's merge another firstly once that PR is ready to be merged. @sanposhiho

[bells17]

Thank you for your suggestion. In that case, would you mind reviewing the following PR first, which adds QHint for PVC?
#124959

[carlory]

Why return an error? If so, the pod will put into BackOffQ. It doesn't make pod become schedulable.

[bells17]

Thank you for your comment. After considering your feedback, I think it might be better to make the following changes:

for _, vol := range pod.Spec.Volumes {
	var pvc *v1.PersistentVolumeClaim
	switch {
	case vol.PersistentVolumeClaim != nil:
		pvcName := vol.PersistentVolumeClaim.ClaimName
		pvc, err = pl.PVCLister.PersistentVolumeClaims(pod.Namespace).Get(pvcName)
		if err != nil {
			return framework.Queue, err
		}
	case vol.Ephemeral != nil:
		pvc = &v1.PersistentVolumeClaim{
			ObjectMeta: vol.Ephemeral.VolumeClaimTemplate.ObjectMeta,
			Spec:       vol.Ephemeral.VolumeClaimTemplate.Spec,
		}
	default:
		continue
	}

	if pvc.Spec.VolumeName != "" {
		// Skipping the check for CSIStorageCapacity as the PVC is configured
		// to be bound to an existing PV.
		continue
	}

	className := volume.GetPersistentVolumeClaimClass(pvc)
	if className == newSC.Name {
		if oldSC == nil {
			logger.V(4).Info("StorageClass was created")
			return framework.Queue, nil
		}

		if !apiequality.Semantic.DeepEqual(newSC.AllowedTopologies, oldSC.AllowedTopologies) {
			logger.V(4).Info("StorageClass was created or updated, and changed Provisioner", "AllowedTopologies", newSC.AllowedTopologies)
			return framework.Queue, nil
		}
	}
}

What do you think about this approach?

[bells17]

I've tried committing the code content mentioned in the above comment.

[sanposhiho]

similar to #124961 (comment); if we can consider StorageClass update is not that frequent, it's not worthy for a fine but time-consuming filtering (using PVCLister.PersistentVolumeClaims().Get()).

[sanposhiho]

As a first step, we should make it minimum, at least until we establish some observability around QHint and could ensure this level of fine filtering doesn't impact a large cluster negatively (#124566).

[bells17]

@sanposhiho Thank you for your review. I have made the necessary changes, so please take another look.

[bells17]

/close
#124961 (comment)

[k8s-ci-robot]

@bells17: Closed this PR.

In response to this:

/close
#124961 (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.