Skip to content
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

WIP: volumebinding: scheduler queueing hints - phase1 #124939

Closed
wants to merge 1 commit into from

Conversation

bells17
Copy link
Contributor

@bells17 bells17 commented May 18, 2024

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

Which issue(s) this PR fixes:

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

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
PersistentVolumeClaim(PVC) .metadata.labels x x x
PersistentVolumeClaim(PVC) .metadata.annotations o o o
PersistentVolumeClaim(PVC) .spec.accessModes o x o
PersistentVolumeClaim(PVC) .spec.selector o o x
PersistentVolumeClaim(PVC) .spec.resources o o x
PersistentVolumeClaim(PVC) .spec.volumeName o o x
PersistentVolumeClaim(PVC) .spec.storageClassName o x o
PersistentVolumeClaim(PVC) .spec.volumeMode x o x
PersistentVolumeClaim(PVC) .spec.dataSource x o x
PersistentVolumeClaim(PVC) .spec.dataSourceRef x o x
PersistentVolumeClaim(PVC) .spec.volumeAttributesClassName x o x
PersistentVolumeClaim(PVC) .status.phase o x o
PersistentVolumeClaim(PVC) .status.accessModes x x x
PersistentVolumeClaim(PVC) .status.capacity x x x
PersistentVolumeClaim(PVC) .status.conditions x x x
PersistentVolumeClaim(PVC) .status.allocatedResources x x x
PersistentVolumeClaim(PVC) .status.allocatedResourceStatuses x x x
PersistentVolumeClaim(PVC) .status.currentVolumeAttributesClassName x x x
PersistentVolumeClaim(PVC) .status.modifyVolumeStatus x x x
Node .metadata.labels o x o
Node .metadata.annotations x o x
Node .spec.podCIDR x x x
Node .spec.podCIDRs x o x
Node .spec.providerID x o x
Node .spec.unschedulable x x x
Node .spec.taints x x x
Node .spec.configSource x x x
Node .spec.externalID x o x
Node .status.capacity x x x
Node .status.allocatable x x x
Node .status.phase x x x
Node .status.conditions x x x
Node .status.addresses x o x
Node .status.daemonEndpoints x x x
Node .status.nodeInfo x x x
Node .status.images x x x
Node .status.volumesInUse x x x
Node .status.volumesAttached x x x
Node .status.config x x x
Node .status.runtimeHandlers x x x
CSINode .metadata.labels x x x
CSINode .metadata.annotations o x o
CSINode .spec.drivers.name x o x
CSINode .spec.drivers.nodeID x o x
CSINode .spec.drivers.topologyKeys x o x
CSINode .spec.drivers.allocatable x o x
CSIDriver .metadata.labels x o x
CSIDriver .metadata.annotations o o x
CSIDriver .spec.attachRequired x o x
CSIDriver .spec.podInfoOnMount x x x
CSIDriver .spec.VolumeLifecycleMode x o x
CSIDriver .spec.storageCapacity o x o
CSIDriver .spec.fsGroupPolicy x x x
CSIDriver .spec.tokenRequests x x x
CSIDriver .spec.requiresRepublish x x x
CSIDriver .spec.seLinuxMount x x x
CSIStorageCapacity .metadata.labels x x x
CSIStorageCapacity .metadata.annotations x x x
CSIStorageCapacity .nodeTopology o o x
CSIStorageCapacity .storageClassName o o x
CSIStorageCapacity .capacity o x o
CSIStorageCapacity .maximumVolumeSize 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 k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. labels May 18, 2024
@k8s-ci-robot
Copy link
Contributor

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

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 k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels May 18, 2024
@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 18, 2024
@bells17 bells17 changed the title Qhint volume binding volumebinding: scheduler queueing hints May 18, 2024
@bells17 bells17 changed the title volumebinding: scheduler queueing hints volumebinding: scheduler queueing hints - phase1 May 18, 2024
@bells17 bells17 force-pushed the qhint-volume-binding branch 3 times, most recently from 05edaed to 4a49474 Compare May 18, 2024 09:48
@bells17 bells17 requested a review from toVersus May 18, 2024 09:58
@bells17
Copy link
Contributor Author

bells17 commented May 18, 2024

/cc @sanposhiho @utam0k @YamasouA

@k8s-ci-robot
Copy link
Contributor

@bells17: GitHub didn't allow me to request PR reviews from the following users: YamasouA.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @sanposhiho @utam0k @YamasouA

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
Copy link
Contributor Author

bells17 commented May 18, 2024

/assign @YamasouA

@k8s-ci-robot
Copy link
Contributor

@bells17: GitHub didn't allow me to assign the following users: YamasouA.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @YamasouA

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 bells17 changed the title volumebinding: scheduler queueing hints - phase1 WIP: volumebinding: scheduler queueing hints - phase1 May 18, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 18, 2024
@bells17 bells17 force-pushed the qhint-volume-binding branch 2 times, most recently from b2061f0 to 5bddd01 Compare May 19, 2024 02:47
@bells17
Copy link
Contributor Author

bells17 commented May 19, 2024

/retest

@bells17 bells17 force-pushed the qhint-volume-binding branch 3 times, most recently from b0a9563 to 326ae8b Compare May 19, 2024 11:56
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 19, 2024
for _, vol := range pod.Spec.Volumes {
if vol.Name == newPV.Name {
logger.V(4).Info("PersistentVolume referenced by the Pod was created or updated")
return framework.Queue, nil
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a PV was subject to processing by csi-translation-lib, it may undergo significant changes. Therefore, even if only a part of the PV changes, I believe it should be treated as a target for queueing. I would appreciate your thoughts on this.

)

result, err := processPodVolumesForQHint(pod, func(pvcName string, isEphemeral bool) (bool, error) {
if pvcName == newPVC.Name {
Copy link
Contributor Author

@bells17 bells17 May 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fields of PVC referenced within Volumebinding are wide-ranging.
Therefore, if we implement a logic that strictly checks for changes in the fields being used, it would be difficult to correctly reflect those changes in the QueueingHintFn when there are modifications to PreFilter or Filter, and I think it would result in high maintenance costs.
So, for the Phase, Annotations, and Spec fields, I'm using DeepEqual to check for differences.
I would appreciate your thoughts on about this approach.

@bells17
Copy link
Contributor Author

bells17 commented May 19, 2024

/retest

@sanposhiho
Copy link
Member

can we close this PR in favor of other smaller PRs you opened?

@bells17
Copy link
Contributor Author

bells17 commented May 20, 2024

/close

@k8s-ci-robot
Copy link
Contributor

@bells17: Closed this PR.

In response to this:

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants