From 42c5ccac2bfd1bc2ebc2fd9f6b74934d4a2e17ae Mon Sep 17 00:00:00 2001 From: Alessandro Yuichi Okimoto Date: Fri, 2 Feb 2024 14:41:16 +0900 Subject: [PATCH] chore: change execute progressive rollout condition Signed-off-by: Alessandro Yuichi Okimoto --- pkg/autoops/api/error.go | 4 + pkg/autoops/api/progressive_rollout.go | 59 ++++++- .../api/progressive_rollout_operation.go | 148 +++++------------- .../api/progressive_rollout_operation_test.go | 66 +------- pkg/autoops/domain/progressive_rollout.go | 4 + test/e2e/autoops/auto_ops_test.go | 12 ++ test/e2e/autoops/progressive_rollout_test.go | 18 ++- 7 files changed, 131 insertions(+), 180 deletions(-) diff --git a/pkg/autoops/api/error.go b/pkg/autoops/api/error.go index dc9e4e00f5..0b830710aa 100644 --- a/pkg/autoops/api/error.go +++ b/pkg/autoops/api/error.go @@ -92,6 +92,10 @@ var ( codes.Internal, "autoops: internal error occurs for a progressive rollout", ) + statusProgressiveRolloutAlreadyStopped = gstatus.New( + codes.Internal, + "autoops: progressive rollout is already stopped", + ) statusProgressiveRolloutClauseVariationIDRequired = gstatus.New( codes.InvalidArgument, "autoops: clause variation id must be specified for a progressive rollout", diff --git a/pkg/autoops/api/progressive_rollout.go b/pkg/autoops/api/progressive_rollout.go index 9a84f51b2c..8f3266eeb6 100644 --- a/pkg/autoops/api/progressive_rollout.go +++ b/pkg/autoops/api/progressive_rollout.go @@ -28,6 +28,7 @@ import ( "github.com/bucketeer-io/bucketeer/pkg/autoops/command" "github.com/bucketeer-io/bucketeer/pkg/autoops/domain" v2as "github.com/bucketeer-io/bucketeer/pkg/autoops/storage/v2" + ftstorage "github.com/bucketeer-io/bucketeer/pkg/feature/storage/v2" "github.com/bucketeer-io/bucketeer/pkg/locale" "github.com/bucketeer-io/bucketeer/pkg/log" "github.com/bucketeer-io/bucketeer/pkg/storage/v2/mysql" @@ -435,6 +436,23 @@ func (s *AutoOpsService) ExecuteProgressiveRollout( if err != nil { return err } + ftStorage := ftstorage.NewFeatureStorage(tx) + feature, err := ftStorage.GetFeature(ctx, progressiveRollout.FeatureId, req.EnvironmentNamespace) + if err != nil { + return err + } + if err := s.checkStopStatus(progressiveRollout, localizer); err != nil { + s.logger.Error( + "Failed to execute progressive rollout", + log.FieldsFromImcomingContext(ctx).AddFields( + zap.Error(err), + zap.String("environmentNamespace", req.EnvironmentNamespace), + zap.String("id", progressiveRollout.Id), + zap.String("featureId", progressiveRollout.FeatureId), + )..., + ) + return err + } triggered, err := s.checkAlreadyTriggered( req.ChangeProgressiveRolloutTriggeredAtCommand, progressiveRollout, @@ -453,6 +471,12 @@ func (s *AutoOpsService) ExecuteProgressiveRollout( ) return nil } + // Enable the flag if it is disabled and it is the first rollout execution + if !feature.Enabled && progressiveRollout.Status == autoopsproto.ProgressiveRollout_WAITING { + if err := feature.Enable(); err != nil { + return err + } + } handler := command.NewProgressiveRolloutCommandHandler( editor, progressiveRollout, @@ -465,13 +489,28 @@ func (s *AutoOpsService) ExecuteProgressiveRollout( if err := storage.UpdateProgressiveRollout(ctx, progressiveRollout, req.EnvironmentNamespace); err != nil { return err } - return ExecuteProgressiveRolloutOperation( + if err := ExecuteProgressiveRolloutOperation( ctx, progressiveRollout, - s.featureClient, + feature, req.ChangeProgressiveRolloutTriggeredAtCommand.ScheduleId, req.EnvironmentNamespace, - ) + ); err != nil { + return err + } + if err := ftStorage.UpdateFeature(ctx, feature, req.EnvironmentNamespace); err != nil { + s.logger.Error( + "Failed to update feature flag", + log.FieldsFromImcomingContext(ctx).AddFields( + zap.Error(err), + zap.String("environmentNamespace", req.EnvironmentNamespace), + zap.String("id", progressiveRollout.Id), + zap.String("featureId", progressiveRollout.FeatureId), + )..., + ) + return err + } + return nil }) if err != nil { s.logger.Error( @@ -503,6 +542,20 @@ func (s *AutoOpsService) ExecuteProgressiveRollout( return &autoopsproto.ExecuteProgressiveRolloutResponse{}, nil } +func (s *AutoOpsService) checkStopStatus(p *domain.ProgressiveRollout, localizer locale.Localizer) error { + if p.IsStopped() { + dt, err := statusProgressiveRolloutAlreadyStopped.WithDetails(&errdetails.LocalizedMessage{ + Locale: localizer.GetLocale(), + Message: localizer.MustLocalize(locale.InternalServerError), + }) + if err != nil { + return statusProgressiveRolloutInternal.Err() + } + return dt.Err() + } + return nil +} + func (s *AutoOpsService) checkAlreadyTriggered( cmd *autoopsproto.ChangeProgressiveRolloutScheduleTriggeredAtCommand, p *domain.ProgressiveRollout, diff --git a/pkg/autoops/api/progressive_rollout_operation.go b/pkg/autoops/api/progressive_rollout_operation.go index adf9e09675..46fefabb76 100644 --- a/pkg/autoops/api/progressive_rollout_operation.go +++ b/pkg/autoops/api/progressive_rollout_operation.go @@ -21,7 +21,7 @@ import ( "github.com/golang/protobuf/ptypes" "github.com/bucketeer-io/bucketeer/pkg/autoops/domain" - featureclient "github.com/bucketeer-io/bucketeer/pkg/feature/client" + ftdomain "github.com/bucketeer-io/bucketeer/pkg/feature/domain" autoopsproto "github.com/bucketeer-io/bucketeer/proto/autoops" featureproto "github.com/bucketeer-io/bucketeer/proto/feature" ) @@ -33,159 +33,87 @@ const totalVariationWeight = int32(100000) func ExecuteProgressiveRolloutOperation( ctx context.Context, progressiveRollout *domain.ProgressiveRollout, - featureClient featureclient.Client, + feature *ftdomain.Feature, scheduleID, environmentNamespace string, ) error { + var variationID string + var weight int32 switch progressiveRollout.Type { case autoopsproto.ProgressiveRollout_MANUAL_SCHEDULE: c := &autoopsproto.ProgressiveRolloutManualScheduleClause{} if err := ptypes.UnmarshalAny(progressiveRollout.Clause, c); err != nil { return err } - s, err := getTargetSchedule(c.Schedules, scheduleID) + variationID = c.VariationId + var err error + weight, err = getTargetWeight(c.Schedules, scheduleID) if err != nil { return err } - if err := updateRolloutStrategy( - ctx, - s, - featureClient, - c.VariationId, - progressiveRollout.FeatureId, - environmentNamespace, - ); err != nil { - return err - } case autoopsproto.ProgressiveRollout_TEMPLATE_SCHEDULE: c := &autoopsproto.ProgressiveRolloutTemplateScheduleClause{} if err := ptypes.UnmarshalAny(progressiveRollout.Clause, c); err != nil { return err } - s, err := getTargetSchedule(c.Schedules, scheduleID) + variationID = c.VariationId + var err error + weight, err = getTargetWeight(c.Schedules, scheduleID) if err != nil { return err } - if err := updateRolloutStrategy( - ctx, - s, - featureClient, - c.VariationId, - progressiveRollout.FeatureId, - environmentNamespace, - ); err != nil { - return err - } default: return domain.ErrProgressiveRolloutInvalidType } + if err := updateRolloutStrategy( + ctx, + weight, + feature, + variationID, + progressiveRollout.FeatureId, + environmentNamespace, + ); err != nil { + return err + } return nil } -func getTargetSchedule( +func getTargetWeight( schedules []*autoopsproto.ProgressiveRolloutSchedule, scheduleID string, -) (*autoopsproto.ProgressiveRolloutSchedule, error) { +) (int32, error) { for _, s := range schedules { if s.ScheduleId == scheduleID { - return s, nil + return s.Weight, nil } } - return nil, domain.ErrProgressiveRolloutScheduleNotFound + return 0, domain.ErrProgressiveRolloutScheduleNotFound } func updateRolloutStrategy( ctx context.Context, - schedule *autoopsproto.ProgressiveRolloutSchedule, - featureClient featureclient.Client, + weight int32, + feature *ftdomain.Feature, targetVariationID, featureID, environmentNamespace string, ) error { - f, err := fetchFeature(ctx, featureClient, featureID, environmentNamespace) + variations, err := getRolloutStrategyVariations(feature, weight, targetVariationID) if err != nil { return err } - if err := updateFeatureTargeting( - ctx, - schedule, - featureClient, - f, - targetVariationID, - environmentNamespace, - ); err != nil { - return err - } - return nil -} - -func fetchFeature( - ctx context.Context, - featureClient featureclient.Client, - featureID, environmentNamespace string, -) (*featureproto.Feature, error) { - resp, err := featureClient.GetFeature(ctx, &featureproto.GetFeatureRequest{ - EnvironmentNamespace: environmentNamespace, - Id: featureID, - }) - if err != nil { - return nil, err - } - return resp.Feature, nil -} - -func updateFeatureTargeting( - ctx context.Context, - schedule *autoopsproto.ProgressiveRolloutSchedule, - featureClient featureclient.Client, - feature *featureproto.Feature, - targetVariationID, environmentNamespace string, -) error { - c, err := getChangeDefaultStrategyCmd(feature, schedule, targetVariationID) - if err != nil { - return err - } - _, err = featureClient.UpdateFeatureTargeting( - ctx, - &featureproto.UpdateFeatureTargetingRequest{ - EnvironmentNamespace: environmentNamespace, - Id: feature.Id, - Commands: []*featureproto.Command{c}, - From: featureproto.UpdateFeatureTargetingRequest_OPS, + strategy := &featureproto.Strategy{ + Type: featureproto.Strategy_ROLLOUT, + RolloutStrategy: &featureproto.RolloutStrategy{ + Variations: variations, }, - ) - if err != nil { + } + if err := feature.ChangeDefaultStrategy(strategy); err != nil { return err } return nil } -func getChangeDefaultStrategyCmd( - feature *featureproto.Feature, - schedule *autoopsproto.ProgressiveRolloutSchedule, - targetVariationID string, -) (*featureproto.Command, error) { - variations, err := getRolloutStrategyVariations(feature, schedule, targetVariationID) - if err != nil { - return nil, err - } - c := &featureproto.ChangeDefaultStrategyCommand{ - Strategy: &featureproto.Strategy{ - Type: featureproto.Strategy_ROLLOUT, - RolloutStrategy: &featureproto.RolloutStrategy{ - Variations: variations, - }, - }, - } - ac, err := ptypes.MarshalAny(c) - if err != nil { - return nil, err - } - return &featureproto.Command{ - Command: ac, - }, nil -} - func getRolloutStrategyVariations( - feature *featureproto.Feature, - schedule *autoopsproto.ProgressiveRolloutSchedule, + feature *ftdomain.Feature, + weight int32, targetVariationID string, ) ([]*featureproto.RolloutStrategy_Variation, error) { nonTargetVariationID, err := findNonTargetVariationID(feature, targetVariationID) @@ -195,17 +123,17 @@ func getRolloutStrategyVariations( return []*featureproto.RolloutStrategy_Variation{ { Variation: targetVariationID, - Weight: schedule.Weight, + Weight: weight, }, { Variation: nonTargetVariationID, - Weight: totalVariationWeight - schedule.Weight, + Weight: totalVariationWeight - weight, }, }, nil } func findNonTargetVariationID( - feature *featureproto.Feature, + feature *ftdomain.Feature, variationID string, ) (string, error) { for _, v := range feature.Variations { diff --git a/pkg/autoops/api/progressive_rollout_operation_test.go b/pkg/autoops/api/progressive_rollout_operation_test.go index 82b28b8336..463915622c 100644 --- a/pkg/autoops/api/progressive_rollout_operation_test.go +++ b/pkg/autoops/api/progressive_rollout_operation_test.go @@ -15,14 +15,11 @@ package api import ( - "context" "testing" "github.com/stretchr/testify/assert" - "go.uber.org/mock/gomock" - featureclientmock "github.com/bucketeer-io/bucketeer/pkg/feature/client/mock" - "github.com/bucketeer-io/bucketeer/proto/autoops" + ftdomain "github.com/bucketeer-io/bucketeer/pkg/feature/domain" featureproto "github.com/bucketeer-io/bucketeer/proto/feature" ) @@ -89,66 +86,13 @@ func TestGetRolloutStrategyVariations(t *testing.T) { } for _, p := range patterns { t.Run(p.desc, func(t *testing.T) { - actual, err := getRolloutStrategyVariations(p.feature, &autoops.ProgressiveRolloutSchedule{ - Weight: p.targetWeight, - }, p.targetVariationID) - assert.Equal(t, p.expectedErr, err) - assert.Equal(t, p.expected, actual) - }) - } -} - -func TestUpdateFeatureTargeting(t *testing.T) { - t.Parallel() - ctx := context.TODO() - mockController := gomock.NewController(t) - environmentNamespace := "en" - fID := "fid-1" - patterns := []struct { - desc string - setup func(*featureclientmock.MockClient) - feature *featureproto.Feature - targetVariationID string - targetWeight int32 - expectedErr error - }{ - { - desc: "success: weight is max", - setup: func(mock *featureclientmock.MockClient) { - mock.EXPECT().UpdateFeatureTargeting(gomock.Any(), gomock.Any()).Return(nil, nil) - }, - feature: &featureproto.Feature{ - Id: fID, - Variations: []*featureproto.Variation{ - { - Id: "vid-1", - }, - { - Id: "vid-2", - }, - }, - }, - targetVariationID: "vid-1", - targetWeight: totalVariationWeight, - }, - } - for _, p := range patterns { - t.Run(p.desc, func(t *testing.T) { - featureClientMock := featureclientmock.NewMockClient(mockController) - if p.setup != nil { - p.setup(featureClientMock) - } - err := updateFeatureTargeting( - ctx, - &autoops.ProgressiveRolloutSchedule{ - Weight: p.targetWeight, - }, - featureClientMock, - p.feature, + actual, err := getRolloutStrategyVariations( + &ftdomain.Feature{Feature: p.feature}, + p.targetWeight, p.targetVariationID, - environmentNamespace, ) assert.Equal(t, p.expectedErr, err) + assert.Equal(t, p.expected, actual) }) } } diff --git a/pkg/autoops/domain/progressive_rollout.go b/pkg/autoops/domain/progressive_rollout.go index 80d753b465..0bbb5aa297 100644 --- a/pkg/autoops/domain/progressive_rollout.go +++ b/pkg/autoops/domain/progressive_rollout.go @@ -106,6 +106,10 @@ func (p *ProgressiveRollout) setClause(c protoiface.MessageV1) error { return nil } +func (p *ProgressiveRollout) IsStopped() bool { + return p.Status == autoopsproto.ProgressiveRollout_STOPPED +} + func (p *ProgressiveRollout) IsFinished() bool { return p.Status == autoopsproto.ProgressiveRollout_FINISHED } diff --git a/test/e2e/autoops/auto_ops_test.go b/test/e2e/autoops/auto_ops_test.go index d070224479..af1a3753fa 100644 --- a/test/e2e/autoops/auto_ops_test.go +++ b/test/e2e/autoops/auto_ops_test.go @@ -526,6 +526,18 @@ func createFeature(ctx context.Context, t *testing.T, client featureclient.Clien enableFeature(t, featureID, client) } +func createDisabledFeature(ctx context.Context, t *testing.T, client featureclient.Client, featureID string) { + t.Helper() + cmd := newCreateFeatureCommand(featureID) + createReq := &featureproto.CreateFeatureRequest{ + Command: cmd, + EnvironmentNamespace: *environmentNamespace, + } + if _, err := client.CreateFeature(ctx, createReq); err != nil { + t.Fatal(err) + } +} + func getFeature(t *testing.T, client featureclient.Client, featureID string) *featureproto.Feature { t.Helper() getReq := &featureproto.GetFeatureRequest{ diff --git a/test/e2e/autoops/progressive_rollout_test.go b/test/e2e/autoops/progressive_rollout_test.go index 883caaab10..6f937518fe 100644 --- a/test/e2e/autoops/progressive_rollout_test.go +++ b/test/e2e/autoops/progressive_rollout_test.go @@ -43,7 +43,7 @@ func TestCreateAndListProgressiveRollout(t *testing.T) { defer featureClient.Close() featureID := createFeatureID(t) - createFeature(ctx, t, featureClient, featureID) + createDisabledFeature(ctx, t, featureClient, featureID) feature := getFeature(t, featureClient, featureID) schedules := createProgressiveRolloutSchedule() createProgressiveRollout( @@ -90,7 +90,7 @@ func TestGetProgressiveRollout(t *testing.T) { defer featureClient.Close() featureID := createFeatureID(t) - createFeature(ctx, t, featureClient, featureID) + createDisabledFeature(ctx, t, featureClient, featureID) feature := getFeature(t, featureClient, featureID) schedules := createProgressiveRolloutSchedule() createProgressiveRollout( @@ -137,7 +137,7 @@ func TestStopProgressiveRollout(t *testing.T) { defer featureClient.Close() featureID := createFeatureID(t) - createFeature(ctx, t, featureClient, featureID) + createDisabledFeature(ctx, t, featureClient, featureID) feature := getFeature(t, featureClient, featureID) schedules := createProgressiveRolloutSchedule() createProgressiveRollout( @@ -176,7 +176,7 @@ func TestDeleteProgressiveRollout(t *testing.T) { defer featureClient.Close() featureID := createFeatureID(t) - createFeature(ctx, t, featureClient, featureID) + createDisabledFeature(ctx, t, featureClient, featureID) feature := getFeature(t, featureClient, featureID) schedules := createProgressiveRolloutSchedule() createProgressiveRollout( @@ -220,7 +220,7 @@ func TestExecuteProgressiveRollout(t *testing.T) { defer featureClient.Close() featureID := createFeatureID(t) - createFeature(ctx, t, featureClient, featureID) + createDisabledFeature(ctx, t, featureClient, featureID) feature := getFeature(t, featureClient, featureID) schedules := createProgressiveRolloutSchedule() createProgressiveRollout( @@ -262,6 +262,9 @@ func TestExecuteProgressiveRollout(t *testing.T) { }, }, } + if !feature.Enabled { + t.Fatalf("Flag shouldn't be disabled at this point") + } if !proto.Equal(feature.DefaultStrategy.RolloutStrategy, expectedStrategy) { t.Fatalf("Strategy is not equal. Expected: %s actual: %s", expectedStrategy, feature.Rules[0].Strategy.RolloutStrategy) } @@ -291,7 +294,7 @@ func TestProgressiveRolloutBatch(t *testing.T) { defer featureClient.Close() featureID := createFeatureID(t) - createFeature(ctx, t, featureClient, featureID) + createDisabledFeature(ctx, t, featureClient, featureID) feature := getFeature(t, featureClient, featureID) now := time.Now() schedules := []*autoopsproto.ProgressiveRolloutSchedule{ @@ -338,6 +341,9 @@ func TestProgressiveRolloutBatch(t *testing.T) { if !proto.Equal(feature.DefaultStrategy.RolloutStrategy, expectedStrategy) { continue } + if !feature.Enabled { + t.Fatalf("Flag shouldn't be disabled at this point") + } actual := listProgressiveRollouts(t, autoOpsClient, featureID) if actual[0].Status != autoopsproto.ProgressiveRollout_FINISHED { t.Fatalf("different status, expected: %v, actual: %v", actual[0].Status, autoopsproto.ProgressiveRollout_FINISHED)