Skip to content

Conversation

kaichiachen
Copy link
Contributor

@kaichiachen kaichiachen commented Apr 14, 2025

What is this PR for?

Implements scheduler name filtering in Yunikorn's admission controller to support environments with multiple schedulers

Key changes:

  • Added new configuration options:
    • admissionController.filtering.overrideCustomSchedulers
  • Modified admission controller logic to:
    • update pod scheduler to yunikorn forcely if admissionController.filtering.overrideCustomSchedulers is true or pod scheduler name is default-scheduler

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2981

How should this be tested?

  1. Set up Yunikonr with
admissionController:
  filtering:
    overrideCustomSchedulers: true
  1. Set up default-scheduler pods - Verify pod scheduler turn to yunikorn
  2. Set up pods with other scheduler name and turn on overrideCustomSchedulers - Verify pod scheduler turn to yunikorn
  3. Set up pods with other scheduler name and turn off overrideCustomSchedulers - Verify pod scheduler wont change

Screenshots (if appropriate)

Questions:

  • - The licenses files need update.
  • - There is breaking changes for older versions.
  • - It needs documentation.

Copy link

codecov bot commented Apr 15, 2025

Codecov Report

❌ Patch coverage is 23.07692% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.05%. Comparing base (a1b66d5) to head (533858b).
⚠️ Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
pkg/admission/admission_controller.go 25.00% 4 Missing and 2 partials ⚠️
pkg/admission/conf/am_conf.go 20.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #966      +/-   ##
==========================================
+ Coverage   68.01%   68.05%   +0.04%     
==========================================
  Files          72       72              
  Lines        9291     9304      +13     
==========================================
+ Hits         6319     6332      +13     
+ Misses       2760     2756       -4     
- Partials      212      216       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@wilfred-s
Copy link
Contributor

We should only ever override the default-scheduler not any custom scheduler that has been set on a pod by default. If we have a custom scheduler we should let the pod pass and not make any changes. That check should be made before we do anything else.
We can then add an override of custom scheduler option. We can use that if we want to force everything in the cluster to be scheduled by YuniKorn based on namespaces etc. Adding the option to override some but not all of the schedulers passed in, is I think not going to give the correct user experience.

So in processPod before checking user info:

  • check the schedulername is not default-scheduler or yunikorn
  • check if custom scheduler override is set to false: return admission response to allow the pod without any changes

Copy link
Contributor

@chenyulin0719 chenyulin0719 left a comment

Choose a reason for hiding this comment

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

Hi @kaichiachen, thanks for submitting the PR. One comment from me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants