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

chore: start server in normal mode when there is no instruction by scheduler #3103

Merged
merged 5 commits into from
Mar 30, 2023

Conversation

BonapartePC
Copy link
Contributor

@BonapartePC BonapartePC commented Mar 15, 2023

Description

Server/Processor should start in normal mode if there is no mode set by the scheduler.
Note: The server starts in degraded mode and only goes to normal mode when there is no instruction from the scheduler via ETCD so far.

Notion Ticket

https://www.notion.so/rudderstacks/Server-should-start-in-normal-mode-by-default-d6f155d9375e4ca6a7fa25412c93a4e6

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

Copy link
Member

@lvrach lvrach left a comment

Choose a reason for hiding this comment

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

It would be safer if scheduler send/set the state to normal if the state is missing, instead of rudder server starting with it by default.

@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Patch coverage: 40.00% and project coverage change: +1.26 🎉

Comparison is base (0aab933) 52.27% compared to head (71cbb6a) 53.54%.

❗ Current head 71cbb6a differs from pull request most recent head 442e90b. Consider uploading reports for the commit 442e90b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3103      +/-   ##
==========================================
+ Coverage   52.27%   53.54%   +1.26%     
==========================================
  Files         321      350      +29     
  Lines       52765    54445    +1680     
==========================================
+ Hits        27583    29150    +1567     
- Misses      23525    23636     +111     
- Partials     1657     1659       +2     
Impacted Files Coverage Δ
app/cluster/dynamic.go 55.95% <40.00%> (+1.96%) ⬆️

... and 91 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@BonapartePC
Copy link
Contributor Author

It would be safer if scheduler send/set the state to normal if the state is missing, instead of rudder server starting with it by default.

I feel server should not depend on whether scheduler is putting values or not right? It should start working in normal mode by default. Scheduler will only decide the server mode during migrations when necessary. @cisse21 WDYT?

@cisse21
Copy link
Member

cisse21 commented Mar 16, 2023

@lvrach the way scheduler works now it would feel out of place and a bit hacky to have this logic present so instead we can always start the server in normal mode by default rather than degraded mode which would be a switch in default behaviour and nothing more right?

@fracasula
Copy link
Collaborator

It does feel weird that RS starts in normal though, because the scheduler is the one orchestrating the sagas via ETCD. If RS is supposed to be in DEGRADED due to a Saga initiated by the scheduler but then RS crashes, it will start in a wrong state and it might accept events that it wasn't supposed to accept/process, right?

@BonapartePC
Copy link
Contributor Author

BonapartePC commented Mar 23, 2023

It does feel weird that RS starts in normal though, because the scheduler is the one orchestrating the sagas via ETCD. If RS is supposed to be in DEGRADED due to a Saga initiated by the scheduler but then RS crashes, it will start in a wrong state and it might accept events that it wasn't supposed to accept/process, right?

RS always saves its state in etcd. Even if the RS crashes, it'll always start in the same state. With this PR, our only change is new servers (that doesn't have any instruction from scheduler so far) will start in NORMAL mode when we scale up rather than DEGRADED mode.

@fracasula
Copy link
Collaborator

RS always saves its state in etcd. Even if the RS crashes, it'll always start in the same state. With this PR, our only change is new servers (that doesn't have any instruction from scheduler so far) will start in NORMAL mode when we scale up rather than DEGRADED mode.

Do we have a test to ensure that interrupted sagas are not affected by this?

@BonapartePC BonapartePC force-pushed the chore.startServerInNormalMode branch from 71cbb6a to 740231c Compare March 29, 2023 13:28
@BonapartePC BonapartePC force-pushed the chore.startServerInNormalMode branch from 740231c to dc13f0f Compare March 29, 2023 13:33
@BonapartePC
Copy link
Contributor Author

Do we have a test to ensure that interrupted sagas are not affected by this?

Added a test for that as well

@BonapartePC BonapartePC requested a review from lvrach March 29, 2023 13:37
@BonapartePC
Copy link
Contributor Author

Note: The server starts in degraded mode and only goes to normal mode when there is no instruction from the scheduler via ETCD so far.

@BonapartePC BonapartePC changed the title chore: start server in normal mode by default chore: start server in normal mode when there is no instruction by scheduler Mar 30, 2023
@BonapartePC BonapartePC merged commit bad6a82 into master Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants