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.

Sign up for GitHub

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

Add ForceMove option on vacate parameters #489

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Add ForceMove option on vacate parameters #489

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ba19c5b
Add ForceMove option on vacate parameters
riccardomarinelli Oct 10, 2024
5ea6794
Update moveParams to add ForceMove
riccardomarinelli Oct 14, 2024
799ce06
Remove uneeded entries and Update vacate Tests
riccardomarinelli Oct 14, 2024
7d3cc67
Add previously removed force-move entries
riccardomarinelli Oct 14, 2024
9a53596
Remove platform_infrastructure.NewMoveClustersByTypeParams entry
riccardomarinelli Oct 14, 2024
d8b9dc8
Add FroceMove as PlanOverride
riccardomarinelli Oct 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/api/platformapi/allocatorapi/vacate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ func moveNodes(id string, params *VacateParams, p *pool.Pool) ([]pool.Validator,
WithAllocatorID(id).
WithMoveOnly(params.MoveOnly).
WithContext(api.WithRegion(context.Background(), params.Region)).
WithForceMove(params.ForceMove).
Copy link

Choose a reason for hiding this comment

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

Same here, won't this alter the behaviour of the soteria flow? for example to always set force_move to true?

Copy link
Author

@riccardomarinelli riccardomarinelli Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If I remove this one, This is the result ( see logs https://platform-logging.kb.eu-west-1.aws.qa.cld.elstc.co/app/r/s/g99SA )

  • Testing with Force_move: true in Fleetctl code:
    Our of 3 log entries,

The first one will have the wong value forceMove=[false]:

Request to move clusters: (allocatorId=[gi-5926589073801201158], authz=[Some(1)], moveRequest=[None], maybeClusterType=[None], forceUpdate=[false], allocatorDown=[None], validateOnly=[true], moveOnly=[true], forceMove=[false])

The second and the third one will have the correct value forceMove=[true]:

Request to move clusters: (allocatorId=[gi-5926589073801201158], authz=[Some(1)], moveRequest=[Some(MoveClustersRequest(Some(List(MoveElasticsearchClusterConfiguration(List(44e610693f534648a7ccacedcb16c65b),None))),None,None,None,None))], maybeClusterType=[None], forceUpdate=[false], allocatorDown=[Some(false)], validateOnly=[true], moveOnly=[true], forceMove=[true])

Request to move clusters: (allocatorId=[gi-5926589073801201158], authz=[Some(1)], moveRequest=[Some(MoveClustersRequest(Some(List(MoveElasticsearchClusterConfiguration(List(44e610693f534648a7ccacedcb16c65b),Some(TransientElasticsearchPlanConfiguration(Some(PlanStrategy(None,Some(GrowShrinkStrategyConfig()),None,None)),Some(EsPlanControlConfiguration(Some(32768),Some(5),None,Some(List(AllocatorMoveRequest(gi-5926589073801201158,None,Some(false)))),Some(true),None,None,Some(false),None,None,Some(false),Some(3),Some(300),Some(false),None,Some(false),Some(false),Some(false),Some(false),Some(false),None)),None,None,None))))),None,None,None,None))], maybeClusterType=[Some(ElasticsearchClusterType)], forceUpdate=[false], allocatorDown=[Some(false)], validateOnly=[false], moveOnly=[true], forceMove=[false])

image

  • Testing with Force_move: true in Fleetctl code:

all the logs entries will have forceMove=[false] see see logs https://platform-logging.kb.eu-west-1.aws.qa.cld.elstc.co/app/r/s/se28H

WithValidateOnly(ec.Bool(true)),
params.AuthWriter,
)
Expand Down Expand Up @@ -282,6 +283,7 @@ func newVacateClusterParams(params addAllocatorMovesToPoolParams, id, kind strin
Output: params.VacateParams.Output,
OutputFormat: params.VacateParams.OutputFormat,
MoveOnly: params.VacateParams.MoveOnly,
ForceMove: params.VacateParams.ForceMove,
PlanOverrides: params.VacateParams.PlanOverrides,
}

Expand Down Expand Up @@ -391,6 +393,7 @@ func newMoveClusterParams(params *VacateClusterParams) (*platform_infrastructure
platform_infrastructure.NewMoveClustersParams().
WithAllocatorDown(params.AllocatorDown).
WithMoveOnly(params.MoveOnly).
WithForceMove(params.ForceMove).
Copy link

Choose a reason for hiding this comment

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

Is this needed here? I would remove it because I think it alters the soteria behaviour i.e. currently we:

  1. Issue a request to validate the move request validate_only=true
  2. Use the params returned to issue the actual vacate

Now, if we set the force_move flag in the validation API, the admin console will respect what u asked.
So in case of soteria, what would be the behaviour? which value will the force_move param have?

Copy link
Author

@riccardomarinelli riccardomarinelli Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If I remove this one, This is the result ( see logs https://platform-logging.kb.eu-west-1.aws.qa.cld.elstc.co/app/r/s/1ElD8 )

Testing with Force_move: true in Fleetctl code:
Our of 3 log entries,

The first one will have the correct value forceMove=[true]:

Request to move clusters: (allocatorId=[gi-5926589073801201158], authz=[Some(1)], moveRequest=[None], maybeClusterType=[None], forceUpdate=[false], allocatorDown=[None], validateOnly=[true], moveOnly=[true], forceMove=[true])

The second and the third one will have the wrong value forceMove=[false]:

Request to move clusters: (allocatorId=[gi-5926589073801201158], authz=[Some(1)], moveRequest=[Some(MoveClustersRequest(Some(List(MoveElasticsearchClusterConfiguration(List(44e610693f534648a7ccacedcb16c65b),None))),None,None,None,None))], maybeClusterType=[None], forceUpdate=[false], allocatorDown=[Some(false)], validateOnly=[true], moveOnly=[true], forceMove=[false])

Request to move clusters: (allocatorId=[gi-5926589073801201158], authz=[Some(1)], moveRequest=[Some(MoveClustersRequest(Some(List(MoveElasticsearchClusterConfiguration(List(44e610693f534648a7ccacedcb16c65b),Some(TransientElasticsearchPlanConfiguration(Some(PlanStrategy(None,Some(GrowShrinkStrategyConfig()),None,None)),Some(EsPlanControlConfiguration(Some(32768),Some(5),None,Some(List(AllocatorMoveRequest(gi-5926589073801201158,None,Some(false)))),Some(true),None,None,Some(false),None,None,Some(false),Some(3),Some(300),Some(false),None,Some(false),Some(false),Some(false),Some(false),Some(false),None)),None,None,None))))),None,None,None,None))], maybeClusterType=[Some(ElasticsearchClusterType)], forceUpdate=[false], allocatorDown=[Some(false)], validateOnly=[false], moveOnly=[true], forceMove=[false])

image

Testing with Force_move: true in Fleetctl code:

all the logs entries will have forceMove=[false] see https://platform-logging.kb.eu-west-1.aws.qa.cld.elstc.co/app/r/s/NtaT0

WithAllocatorID(params.ID).
WithContext(api.WithRegion(context.Background(), params.Region)).
WithValidateOnly(ec.Bool(true)).
Expand Down
6 changes: 5 additions & 1 deletion pkg/api/platformapi/allocatorapi/vacate_params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ var (
)

// VacateParams used to vacate N allocators or clusters.
//nolint
// nolint
type VacateParams struct {
*api.API

Expand Down Expand Up @@ -98,6 +98,9 @@ type VacateParams struct {
// bare minimum to move the requested instances across to another allocator.
MoveOnly *bool

// Optional value to be set to force_move to true for primitive vacate or false for standard.
ForceMove *bool

// SkipTracking skips displaying and waiting for the individual vacates to complete.
// Setting it to true will render the concurrency flag pretty much ineffective since
// the vacate action is asynchronous and the only thing keeping the working items in
Expand Down Expand Up @@ -166,6 +169,7 @@ type VacateClusterParams struct {
TrackFrequency time.Duration
AllocatorDown *bool
MoveOnly *bool
ForceMove *bool
Output *output.Device
OutputFormat string
MaxPollRetries uint8
Expand Down
1 change: 1 addition & 0 deletions pkg/api/platformapi/allocatorapi/vacate_params_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ func TestVacateClusterParamsValidate(t *testing.T) {
TrackFrequency time.Duration
AllocatorDown *bool
MoveOnly *bool
ForceMove *bool
Output *output.Device
OutputFormat string
MaxPollRetries uint8
Expand Down
Loading