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

(feat) Implement the ActorSpreadScaler #75

Merged
merged 3 commits into from Apr 12, 2023

Conversation

brooksmtownsend
Copy link
Member

@brooksmtownsend brooksmtownsend commented Apr 5, 2023

Fixes #51

This PR adds the ActorSpreadScaler, an implementation of the Scaler trait which spreads replicas of actors across a list of spread requirements. It does not attempt to evenly spread on hosts that match requirements, as that's more complicated and better suited for another Scaler implementation.

Currently marking as ready for review, but two (or three?) outstanding things I need to address

  • Host state is not modified in the store when an actor is stopped or started. Updated PR to ensure host state is updated when actors and providers start/stop
  • There may be an edge case where multiple spread requirements match on the same host, resulting in conflicting commands. Annotations will be required to solve this, may have to add support in this PR.
  • (future PR) This scaler ignores linkdef and provider events. If desired to show a similar implementation for those scalers, they will be in different files as different Scaler implementations

Feature or Problem

Related Issues

Release Information

Consumer Impact

Testing

Built on platform(s)

  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
  • x86_64-windows

Tested on platform(s)

  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
  • x86_64-windows

Unit Test(s)

Added multiple unit tests to ensure proper functionality of the spreadscaler

Manual Verification

I ran the tests but did not run for reals since the mechanism for publishing commands isn't hooked up quite yet

Base automatically changed from feat/scaler-trait to wadm_0.4 April 5, 2023 19:32
src/scaler/spreadscaler.rs Outdated Show resolved Hide resolved
src/scaler/spreadscaler.rs Outdated Show resolved Hide resolved
src/scaler/spreadscaler.rs Outdated Show resolved Hide resolved
@thomastaylor312 thomastaylor312 linked an issue Apr 5, 2023 that may be closed by this pull request
@brooksmtownsend brooksmtownsend changed the title [WIP] (feat) Spreadscaler impl (feat) Implement the ActorSpreadScaler Apr 10, 2023
@brooksmtownsend brooksmtownsend marked this pull request as ready for review April 10, 2023 15:10
Copy link
Contributor

@thomastaylor312 thomastaylor312 left a comment

Choose a reason for hiding this comment

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

Excellent testing and work here. There are some potential issues with the scaling algorithm we should discuss

src/commands/mod.rs Outdated Show resolved Hide resolved
src/commands/mod.rs Outdated Show resolved Hide resolved
src/scaler/spreadscaler.rs Outdated Show resolved Hide resolved
src/scaler/spreadscaler.rs Outdated Show resolved Hide resolved
src/scaler/spreadscaler.rs Outdated Show resolved Hide resolved
src/storage/state.rs Outdated Show resolved Hide resolved
src/storage/state.rs Outdated Show resolved Hide resolved
src/storage/state.rs Outdated Show resolved Hide resolved
src/workers/event.rs Outdated Show resolved Hide resolved
src/workers/event.rs Show resolved Hide resolved
Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

spreadscaler can compute commands

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>
Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

updated update_config to trigger reconcile

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

added event handling and tests

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

cleaned up spreadscaler for review

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

added note about conflicting spreads

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>
Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

satisfied clippy

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

mostly finished with state refactor, Scaler vec

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

Corrected simplescaler to match new state

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

modified tests in workers to use inventory

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

mostly updated spreadscaler tests

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

updated existing tests to work with annotations

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

reworked spreadscaler to use annotations

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

ensure spreadscaler annotations include wadm

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

pr cleanup, satisfy clippy overlord

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

updated state for event test

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

resolved nits in PR comments

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

compute spread requirements and store, once

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

removed Hash impl's and derives from command

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

efficiently initialize actor ID, remove extra annotation

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>
@brooksmtownsend brooksmtownsend merged commit 1f13a07 into wadm_0.4 Apr 12, 2023
2 checks passed
@brooksmtownsend brooksmtownsend deleted the feat/spreadscaler-impl branch April 12, 2023 20:46
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.

Fully implement the spreadscaler
2 participants