Add the Ability to Specify the Namespace Used for Discovering Scheduler Estimator Services #5478

jabellard · 2024-09-04T10:57:53Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
When the scheduler estimator feature is enabled for the Karmada scheduler component, we can then provision scheduler estimator instances for member clusters that will provide the scheduler with more accurate information regarding resources for those member clusters.

To gain that information, given some member cluster, the scheduler needs to contact the service for that member cluster's scheduler estimator deployment. As of today, it uses a naming convention for discovering those services. By default, given some member cluster m1, it will look for a service named karmada-scheduler-estimator-m1 in the karmada-system namespace.
The service name prefix is configurable, but the namespace is not. This feature request is for providing the ability to configure the namespace in which the scheduler should try to discover the service.

At Bloomberg, we're currently building a managed Karmada platform and want to use the Karmada operator to manage the entire lifecycle of managed Karmada instances. Each tenant's control plane will live in a namespace in our host/management cluster. As such, we would also like to keep scheduler estimator instances scoped to their respective tenancy's namespace to avoid polluting the karmada-system namespace with instances across all managed tenancies.

Which issue(s) this PR fixes:
Fixes #5448

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

The `scheduler-estimator-service-namespace` flag is introduced for the `scheduler` and `descheduler` components. That new flag can be  used to explicitly specify the namespace that should be used to discover scheduler estimator services. For backwards compatibility, when not explicitly set, the default value of `karmada-system` is retained.

codecov-commenter · 2024-09-04T11:11:09Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 33.33333% with 26 lines in your changes missing coverage. Please review.

Project coverage is 31.70%. Comparing base (7a2b493) to head (b6fbcb4).
Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/scheduler/scheduler.go	28.57%	10 Missing ⚠️
pkg/estimator/client/cache.go	0.00%	8 Missing ⚠️
pkg/descheduler/descheduler.go	57.14%	5 Missing and 1 partial ⚠️
cmd/descheduler/app/options/options.go	0.00%	1 Missing ⚠️
cmd/scheduler/app/scheduler.go	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5478      +/-   ##
==========================================
- Coverage   31.71%   31.70%   -0.01%     
==========================================
  Files         643      643              
  Lines       44429    44469      +40     
==========================================
+ Hits        14089    14100      +11     
- Misses      29310    29339      +29     
  Partials     1030     1030

Flag	Coverage Δ
unittests	`31.70% <33.33%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

RainbowMango · 2024-09-06T07:13:12Z

https://github.com/karmada-io/karmada/actions/runs/10700286619/job/29664050081?pr=5478#step:6:3792

 [FAILED] [300.280 seconds]
[Suspend] clusterPropagation testing suspend the ClusterPropagationPolicy dispatching [It] suspends Work
/home/runner/work/karmada/karmada/test/e2e/clusterpropagationpolicy_test.go:1129

  Timeline >>
  STEP: Creating ClusterPropagationPolicy(clusterrole-8nhrn) @ 09/04/24 11:20:25.299
  STEP: Creating ClusterRole(system:test-clusterrole-8nhrn) @ 09/04/24 11:20:25.414
  STEP: Updating ClusterPropagationPolicy(clusterrole-8nhrn) spec @ 09/04/24 11:20:25.424
  [FAILED] in [It] - /home/runner/work/karmada/karmada/test/e2e/clusterpropagationpolicy_test.go:1137 @ 09/04/24 11:25:25.453
  STEP: Removing ClusterPropagationPolicy(clusterrole-8nhrn) @ 09/04/24 11:25:25.571
  STEP: Remove ClusterRole(system:test-clusterrole-8nhrn) @ 09/04/24 11:25:25.575
  << Timeline

  [FAILED] Timed out after 300.000s.
  Expected
      <bool>: false
  to equal
      <bool>: true
  In [It] at: /home/runner/work/karmada/karmada/test/e2e/clusterpropagationpolicy_test.go:1137 @ 09/04/24 11:25:25.453

/retest
unrelated failure.
cc @XiShanYongYe-Chang

RainbowMango · 2024-09-06T07:15:32Z

@whitewindmills @zhzhuang-zju Do you want to take a look?

zhzhuang-zju · 2024-09-06T07:53:45Z

/assign

zhzhuang-zju · 2024-09-09T03:06:06Z

@jabellard Great job~ Generally LGTM, and ask @whitewindmills for another look.

Once this PR is merged, the Karmada operator will need to be adapted. The templates for the components karmada-scheduler and karmada-descheduler could include this field.

whitewindmills

thanks.
/lgtm

whitewindmills · 2024-09-09T03:25:25Z

Once this PR is merged, the Karmada operator will need to be adapted. The templates for the components karmada-scheduler and karmada-descheduler could include this field.

to add helm, karmadactl, etc. could you create a task list to track this?

whitewindmills · 2024-09-09T03:26:17Z

ask approval from @RainbowMango
/assign @RainbowMango

RainbowMango · 2024-09-09T10:16:17Z

pkg/scheduler/scheduler.go

@@ -776,7 +787,7 @@ func (s *Scheduler) reconcileEstimatorConnection(key util.QueueKey) error {
 		return nil
 	}

-	return estimatorclient.EstablishConnection(s.KubeClient, name, s.schedulerEstimatorCache, s.schedulerEstimatorServicePrefix, s.schedulerEstimatorClientConfig)
+	return estimatorclient.EstablishConnection(s.KubeClient, name, s.schedulerEstimatorCache, s.schedulerEstimatorServiceNamespace, s.schedulerEstimatorServicePrefix, s.schedulerEstimatorClientConfig)


With this parameter, the number of parameters exceeds 5, I remember some tools might check this and would bring a warning for this.

This is not a blocker, do you have any idea how to improve it?

Good point. Will push PR to address in a few.

…uler estimator services Signed-off-by: Joe Nathan Abellard <[email protected]> Address comments Signed-off-by: Joe Nathan Abellard <[email protected]> Address comments Signed-off-by: Joe Nathan Abellard <[email protected]>

RainbowMango

Perfect, thanks!

karmada-bot · 2024-09-10T10:34:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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:

~~cmd/OWNERS~~ [RainbowMango]
~~pkg/descheduler/OWNERS~~ [RainbowMango]
~~pkg/estimator/OWNERS~~ [RainbowMango]
~~pkg/scheduler/OWNERS~~ [RainbowMango]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

RainbowMango · 2024-09-10T11:41:18Z

/lgtm

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 4, 2024

karmada-bot requested review from chaunceyjiang, Garrybest and lonelyCZ September 4, 2024 10:58

karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 4, 2024

jabellard mentioned this pull request Sep 4, 2024

Add the Ability to Specify Namespace of Scheduler Estimator Services for Karmada Scheduler #5448

Closed

karmada-bot assigned zhzhuang-zju Sep 6, 2024

whitewindmills reviewed Sep 9, 2024

View reviewed changes

karmada-bot assigned whitewindmills Sep 9, 2024

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2024

karmada-bot assigned RainbowMango Sep 9, 2024

RainbowMango reviewed Sep 9, 2024

View reviewed changes

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 9, 2024

jabellard force-pushed the estimator_service_namespace branch from 1a33436 to 07f3d31 Compare September 9, 2024 11:11

jabellard force-pushed the estimator_service_namespace branch from 07f3d31 to b6fbcb4 Compare September 9, 2024 11:22

RainbowMango approved these changes Sep 10, 2024

View reviewed changes

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2024

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2024

RainbowMango added this to the v1.12 milestone Sep 10, 2024

karmada-bot merged commit 07e2976 into karmada-io:master Sep 10, 2024
12 checks passed

RainbowMango mentioned this pull request Oct 18, 2024

External etcd Support for Karmada Operator #5536

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the Ability to Specify the Namespace Used for Discovering Scheduler Estimator Services #5478

Add the Ability to Specify the Namespace Used for Discovering Scheduler Estimator Services #5478

jabellard commented Sep 4, 2024

codecov-commenter commented Sep 4, 2024 •

edited

Loading

RainbowMango commented Sep 6, 2024

RainbowMango commented Sep 6, 2024

zhzhuang-zju commented Sep 6, 2024

zhzhuang-zju commented Sep 9, 2024

whitewindmills left a comment

whitewindmills commented Sep 9, 2024

whitewindmills commented Sep 9, 2024

RainbowMango Sep 9, 2024

jabellard Sep 9, 2024

RainbowMango left a comment

karmada-bot commented Sep 10, 2024

RainbowMango commented Sep 10, 2024

Add the Ability to Specify the Namespace Used for Discovering Scheduler Estimator Services #5478

Add the Ability to Specify the Namespace Used for Discovering Scheduler Estimator Services #5478

Conversation

jabellard commented Sep 4, 2024

codecov-commenter commented Sep 4, 2024 • edited Loading

Codecov Report

RainbowMango commented Sep 6, 2024

RainbowMango commented Sep 6, 2024

zhzhuang-zju commented Sep 6, 2024

zhzhuang-zju commented Sep 9, 2024

whitewindmills left a comment

Choose a reason for hiding this comment

whitewindmills commented Sep 9, 2024

whitewindmills commented Sep 9, 2024

RainbowMango Sep 9, 2024

Choose a reason for hiding this comment

jabellard Sep 9, 2024

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Sep 10, 2024

RainbowMango commented Sep 10, 2024

codecov-commenter commented Sep 4, 2024 •

edited

Loading