Skip to content

use dapr_scheduler.ha to allow for scaling independently of the global.ha#8304

Merged
yaron2 merged 3 commits into
dapr:masterfrom
cicoyle:enable-scheduler-ha
Nov 21, 2024
Merged

use dapr_scheduler.ha to allow for scaling independently of the global.ha#8304
yaron2 merged 3 commits into
dapr:masterfrom
cicoyle:enable-scheduler-ha

Conversation

@cicoyle

@cicoyle cicoyle commented Nov 21, 2024

Copy link
Copy Markdown
Contributor

allow the use of dapr_scheduler.ha to scale independely of the global ha flag. This allows for global ha to be false and to scale the schedulers to 3 instances.

I confirmed the following to be true by testing all cases locally.

High level (flag breakdown):

  • scheduler enabled is required for schedulers to exist (default is true)
  • global ha being true is fully respected
    • this mean, you cannot override the flag by setting the local ha flag to false. for example, if I have --set global.ha.enabled=true neither --set dapr_scheduler.ha=false --set dapr_placement.ha=false are respected meaning you are unable to be globally ha and have either of the placement or scheduler as a single instance. global being true overrides local being false (bc it defaults to false)
  • if global ha is false, you can have schedulers scaled to 3 instances by setting --set dapr_scheduler.ha=true

@cicoyle
cicoyle requested review from a team as code owners November 21, 2024 17:49
elena-kolevska
elena-kolevska suggested changes Nov 21, 2024
View reviewed changes

@elena-kolevska elena-kolevska left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should also support the case when global HA is true and scheduler HA is false. For comparison, placement will allow HA when its own local HA is true, and the global is false.
Otherwise, lgtm.

EDIT: Nevermind, I checked with placement, and it behaves the same way

@cicoyle

cicoyle commented Nov 21, 2024

Copy link
Copy Markdown
Contributor Author

I think we should also support the case when global HA is true and scheduler HA is false. For comparison, placement will allow HA when its own local HA is true, and the global is false. Otherwise, lgtm.

Hey @elena-kolevska! Thanks for the review!

re: your comment: I think we should also support the case when global HA is true and scheduler HA is false.

Are you saying you want the local scheduler flag to override the global ha flag specifically for the case where --set global.ha.enabled=true & allow someone to have 1 scheduler by putting --set dapr_scheduler.ha=false? I intentionally didn't acct for this for Scheduler bc this is not how it works for placement. In my local testing, when I set --set global.ha.enabled=true, I was unable to have 1 instance of either scheduler/placement. Meaning, I could not override --set global.ha.enabled=true by setting --set dapr_scheduler.ha=false --set dapr_placement.ha=false

re: your comment: For comparison, placement will allow HA when its own local HA is true, and the global is false. . This works the same for scheduler with this PR change, meaning I did account for using the local scheduler ha flag to scale to 3 scheduler instances when global ha is false.

@elena-kolevska

elena-kolevska commented Nov 21, 2024

Copy link
Copy Markdown
Contributor

Yeah, nevermind, I had commented relying just on the comment in the helm file itself without reading the PR comment, the behaviour is fully consistent with placement!

@yaron2
yaron2 merged commit b5d83e5 into dapr:master Nov 21, 2024
jake-engelberg pushed a commit to jake-engelberg/dapr that referenced this pull request Dec 10, 2024
@cicoyle @jake-engelberg
use dapr_scheduler.ha to allow for scaling independently of the globa…
7a6a67b 
…l.ha (dapr#8304)

* allow for local dapr_scheduler.ha to be used to scale outside of global.ha

Signed-off-by: Cassandra Coyle <[email protected]>

* update code comment local ha -> dapr_scheduler.ha for readability

Signed-off-by: Cassandra Coyle <[email protected]>

---------

Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Jake Engelberg <[email protected]>
@cicoyle cicoyle mentioned this pull request Dec 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yaron2 yaron2 yaron2 approved these changes
+1 more reviewer
@elena-kolevska elena-kolevska elena-kolevska approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cicoyle @elena-kolevska @yaron2