Skip to content

Allow configuring trigger frequency when application is out_of_sync #2793

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

Merged
pipecd-bot merged 2 commits into from
Nov 17, 2021
Merged

Allow configuring trigger frequency when application is out_of_sync #2793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9d34bd
Allow configuring trigger frequency when application is out_of_sync
nghialv Nov 16, 2021
6ea9013
Applied reviews
nghialv Nov 17, 2021
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
43 changes: 42 additions & 1 deletion pkg/app/piped/trigger/determiner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ import (
"context"
"fmt"
"strings"
"time"

"go.uber.org/zap"

"github.com/pipe-cd/pipe/pkg/app/api/service/pipedservice"
"github.com/pipe-cd/pipe/pkg/config"
"github.com/pipe-cd/pipe/pkg/filematcher"
"github.com/pipe-cd/pipe/pkg/git"
Expand Down Expand Up @@ -51,6 +53,10 @@ func (ds *determiners) Determiner(ck candidateKind) Determiner {
type OnCommandDeterminer struct {
}

func NewOnCommandDeterminer() *OnCommandDeterminer {
return &OnCommandDeterminer{}
}

// ShouldTrigger decides whether a given application should be triggered or not.
func (d *OnCommandDeterminer) ShouldTrigger(_ context.Context, _ *model.Application, appCfg *config.GenericDeploymentSpec) (bool, error) {
if appCfg.Trigger.OnCommand.Disabled {
Expand All @@ -61,14 +67,49 @@ func (d *OnCommandDeterminer) ShouldTrigger(_ context.Context, _ *model.Applicat
}

type OnOutOfSyncDeterminer struct {
client apiClient
}

func NewOnOutOfSyncDeterminer(client apiClient) *OnOutOfSyncDeterminer {
return &OnOutOfSyncDeterminer{
client: client,
}
}

// ShouldTrigger decides whether a given application should be triggered or not.
func (d *OnOutOfSyncDeterminer) ShouldTrigger(_ context.Context, _ *model.Application, appCfg *config.GenericDeploymentSpec) (bool, error) {
func (d *OnOutOfSyncDeterminer) ShouldTrigger(ctx context.Context, app *model.Application, appCfg *config.GenericDeploymentSpec) (bool, error) {
if *appCfg.Trigger.OnOutOfSync.Disabled {
return false, nil
}

// Find the most recently triggered deployment.
// Nil means it seems the application has been added recently
// and no deployment was triggered yet.
ref := app.MostRecentlyTriggeredDeployment
if ref == nil {
return true, nil
}

resp, err := d.client.GetDeployment(ctx, &pipedservice.GetDeploymentRequest{
Id: ref.DeploymentId,
})
if err != nil {
return false, err
}
deployment := resp.Deployment

// Check if it was already completed or not.
// Not yet completed means the application is deploying currently,
// so no need to trigger a new deployment for it.
if !model.IsCompletedDeployment(deployment.Status) {
return false, nil
}

// Check the elapsed time since the last deployment.
if time.Since(time.Unix(deployment.CompletedAt, 0)) < appCfg.Trigger.OnOutOfSync.MinWindow.Duration() {
return false, nil
}

Copy link
Member

@khanhtc1202 khanhtc1202 Nov 16, 2021

Choose a reason for hiding this comment

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

tbh, it looks like this determiner will trigger a new deployment whenever

  • no previous triggered deployment (newly added application).
  • time since that latest triggered deployment overs a minWindow of time.

Those two are good to me, but since this determiner is for the OUT_OF_SYNC state, I think we may need logic to trigger a new deployment on app.SyncState.Status is OUT_OF_SYNC. Please correct me if I'm wrong.

Copy link
Member Author

@nghialv nghialv Nov 17, 2021

Choose a reason for hiding this comment

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

it looks like this determiner will trigger a new deployment whenever

  • no previous triggered deployment (newly added application).
  • time since that latest triggered deployment overs a minWindow of time.

This is correct.

I think we may need logic to trigger a new deployment on app.SyncState.Status is OUT_OF_SYNC.

We don't need that logic because this determiner is accepting only OUT_OF_SYNC candidates.
https://github.com/pipe-cd/pipe/blob/c00e51ac61a93399ccc5e572468c1e23d28d9e8f/pkg/app/piped/trigger/trigger.go#L312
https://github.com/pipe-cd/pipe/blob/master/pkg/app/piped/trigger/trigger.go#L237

return true, nil
}

Expand Down
5 changes: 3 additions & 2 deletions pkg/app/piped/trigger/trigger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ const (
type apiClient interface {
GetApplicationMostRecentDeployment(ctx context.Context, req *pipedservice.GetApplicationMostRecentDeploymentRequest, opts ...grpc.CallOption) (*pipedservice.GetApplicationMostRecentDeploymentResponse, error)
CreateDeployment(ctx context.Context, in *pipedservice.CreateDeploymentRequest, opts ...grpc.CallOption) (*pipedservice.CreateDeploymentResponse, error)
GetDeployment(ctx context.Context, in *pipedservice.GetDeploymentRequest, opts ...grpc.CallOption) (*pipedservice.GetDeploymentResponse, error)
ReportApplicationMostRecentDeployment(ctx context.Context, req *pipedservice.ReportApplicationMostRecentDeploymentRequest, opts ...grpc.CallOption) (*pipedservice.ReportApplicationMostRecentDeploymentResponse, error)
}

Expand Down Expand Up @@ -214,8 +215,8 @@ func (t *Trigger) checkRepoCandidates(ctx context.Context, repoID string, cs []c
}

ds := &determiners{
onCommand: &OnCommandDeterminer{},
onOutOfSync: &OnOutOfSyncDeterminer{},
onCommand: NewOnCommandDeterminer(),
onOutOfSync: NewOnOutOfSyncDeterminer(t.apiClient),
onCommit: NewOnCommitDeterminer(gitRepo, headCommit.Hash, t.commitStore, t.logger),
}

Expand Down
5 changes: 3 additions & 2 deletions pkg/config/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,9 @@ type OnOutOfSync struct {
// when application is at OUT_OF_SYNC state.
// Default is true.
Disabled *bool `json:"disabled,omitempty" default:"true"`
// TODO: Add a field to control the trigger frequency.
// MinWindow Duration `json:"minWindow,omitempty"`
// Minimum amount of time must be elapsed since the last deployment.
// This can be used to avoid triggering unnecessary continuous deployments based on OUT_OF_SYNC status.
MinWindow Duration `json:"minWindow,omitempty" default:"5m"`
}

func (s *GenericDeploymentSpec) Validate() error {
Expand Down
3 changes: 2 additions & 1 deletion pkg/config/deployment_cloudrun_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ func TestCloudRunDeploymentConfig(t *testing.T) {
Disabled: false,
},
OnOutOfSync: OnOutOfSync{
Disabled: newBoolPointer(true),
Disabled: newBoolPointer(true),
MinWindow: Duration(5 * time.Minute),
},
},
},
Expand Down
3 changes: 2 additions & 1 deletion pkg/config/deployment_ecs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ func TestECSDeploymentConfig(t *testing.T) {
Disabled: false,
},
OnOutOfSync: OnOutOfSync{
Disabled: newBoolPointer(true),
Disabled: newBoolPointer(true),
MinWindow: Duration(5 * time.Minute),
},
},
},
Expand Down
3 changes: 2 additions & 1 deletion pkg/config/deployment_kubernetes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ func TestKubernetesDeploymentConfig(t *testing.T) {
Disabled: false,
},
OnOutOfSync: OnOutOfSync{
Disabled: newBoolPointer(true),
Disabled: newBoolPointer(true),
MinWindow: Duration(5 * time.Minute),
},
},
},
Expand Down
9 changes: 6 additions & 3 deletions pkg/config/deployment_lambda_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ func TestLambdaDeploymentConfig(t *testing.T) {
Disabled: false,
},
OnOutOfSync: OnOutOfSync{
Disabled: newBoolPointer(true),
Disabled: newBoolPointer(true),
MinWindow: Duration(5 * time.Minute),
},
},
},
Expand Down Expand Up @@ -93,7 +94,8 @@ func TestLambdaDeploymentConfig(t *testing.T) {
},
Trigger: Trigger{
OnOutOfSync: OnOutOfSync{
Disabled: newBoolPointer(true),
Disabled: newBoolPointer(true),
MinWindow: Duration(5 * time.Minute),
},
},
},
Expand Down Expand Up @@ -130,7 +132,8 @@ func TestLambdaDeploymentConfig(t *testing.T) {
},
Trigger: Trigger{
OnOutOfSync: OnOutOfSync{
Disabled: newBoolPointer(true),
Disabled: newBoolPointer(true),
MinWindow: Duration(5 * time.Minute),
},
},
},
Expand Down
12 changes: 8 additions & 4 deletions pkg/config/deployment_terraform_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ func TestTerraformDeploymentConfig(t *testing.T) {
Disabled: false,
},
OnOutOfSync: OnOutOfSync{
Disabled: newBoolPointer(true),
Disabled: newBoolPointer(true),
MinWindow: Duration(5 * time.Minute),
},
},
},
Expand All @@ -70,7 +71,8 @@ func TestTerraformDeploymentConfig(t *testing.T) {
Disabled: false,
},
OnOutOfSync: OnOutOfSync{
Disabled: newBoolPointer(true),
Disabled: newBoolPointer(true),
MinWindow: Duration(5 * time.Minute),
},
},
},
Expand All @@ -96,7 +98,8 @@ func TestTerraformDeploymentConfig(t *testing.T) {
Disabled: false,
},
OnOutOfSync: OnOutOfSync{
Disabled: newBoolPointer(false),
Disabled: newBoolPointer(false),
MinWindow: Duration(5 * time.Minute),
},
},
Encryption: &SecretEncryption{
Expand Down Expand Up @@ -150,7 +153,8 @@ func TestTerraformDeploymentConfig(t *testing.T) {
Disabled: false,
},
OnOutOfSync: OnOutOfSync{
Disabled: newBoolPointer(true),
Disabled: newBoolPointer(true),
MinWindow: Duration(5 * time.Minute),
},
},
},
Expand Down
12 changes: 8 additions & 4 deletions pkg/config/deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,8 @@ func TestGenericTriggerConfiguration(t *testing.T) {
},
},
OnOutOfSync: OnOutOfSync{
Disabled: newBoolPointer(true),
Disabled: newBoolPointer(true),
MinWindow: Duration(5 * time.Minute),
},
},
},
Expand Down Expand Up @@ -318,7 +319,8 @@ func TestTrueByDefaultBoolConfiguration(t *testing.T) {
Timeout: Duration(6 * time.Hour),
Trigger: Trigger{
OnOutOfSync: OnOutOfSync{
Disabled: newBoolPointer(true),
Disabled: newBoolPointer(true),
MinWindow: Duration(5 * time.Minute),
},
},
},
Expand All @@ -337,7 +339,8 @@ func TestTrueByDefaultBoolConfiguration(t *testing.T) {
Timeout: Duration(6 * time.Hour),
Trigger: Trigger{
OnOutOfSync: OnOutOfSync{
Disabled: newBoolPointer(false),
Disabled: newBoolPointer(false),
MinWindow: Duration(5 * time.Minute),
},
},
},
Expand All @@ -356,7 +359,8 @@ func TestTrueByDefaultBoolConfiguration(t *testing.T) {
Timeout: Duration(6 * time.Hour),
Trigger: Trigger{
OnOutOfSync: OnOutOfSync{
Disabled: newBoolPointer(true),
Disabled: newBoolPointer(true),
MinWindow: Duration(5 * time.Minute),
},
},
},
Expand Down