From e6668ebbd0dedb83cde44ca6bf27da64178c3aa1 Mon Sep 17 00:00:00 2001 From: nghialv Date: Thu, 13 Aug 2020 16:01:00 +0900 Subject: [PATCH 1/3] Fix a bug that causes piped crashes while rolling back --- pkg/app/piped/controller/scheduler.go | 94 +++++++++++++-------------- 1 file changed, 46 insertions(+), 48 deletions(-) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index 1a6167ebbd..1e4377c0f6 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -31,7 +31,6 @@ import ( pln "github.com/pipe-cd/pipe/pkg/app/piped/planner" "github.com/pipe-cd/pipe/pkg/cache" "github.com/pipe-cd/pipe/pkg/config" - "github.com/pipe-cd/pipe/pkg/git" "github.com/pipe-cd/pipe/pkg/model" ) @@ -67,7 +66,8 @@ type scheduler struct { deploymentConfig *config.Config pipelineable config.Pipelineable - prepareOnce sync.Once + prepareMu sync.Mutex + prepared bool // Current status of each stages. // We stores their current statuses into this field // because the deployment model is readonly to avoid data race. @@ -468,61 +468,59 @@ func (s *scheduler) executeStage(sig executor.StopSignal, ps model.PipelineStage // The log of this preparing process will be written to the first executing stage // when a new scheduler has been created. func (s *scheduler) ensurePreparing(ctx context.Context, lp logpersister.StageLogPersister) error { - var err error - s.prepareOnce.Do(func() { - lp.Info("Start preparing for deployment") + s.prepareMu.Lock() + defer s.prepareMu.Unlock() + if s.prepared { + return nil + } - // Clone repository and checkout to the target revision. - var ( - gitRepo git.Repo - repoDirPath = filepath.Join(s.workingDir, workspaceGitRepoDirName) - ) - gitRepo, err = prepareDeployRepository(ctx, s.deployment, s.gitClient, repoDirPath, s.pipedConfig) - if err != nil { - lp.Error(err.Error()) - return - } - lp.Successf("Successfully cloned repository %s", s.deployment.GitPath.Repo.Id) + lp.Info("Start preparing for the deployment") - // Copy and checkout the running revision. - if s.deployment.RunningCommitHash != "" { - var ( - runningGitRepo git.Repo - runningRepoPath = filepath.Join(s.workingDir, workspaceGitRunningRepoDirName) - ) - runningGitRepo, err = gitRepo.Copy(runningRepoPath) - if err != nil { - lp.Error(err.Error()) - return - } - if err = runningGitRepo.Checkout(ctx, s.deployment.RunningCommitHash); err != nil { - lp.Error(err.Error()) - return - } - } + // Clone repository and checkout to the target revision. + repoDirPath := filepath.Join(s.workingDir, workspaceGitRepoDirName) + gitRepo, err := prepareDeployRepository(ctx, s.deployment, s.gitClient, repoDirPath, s.pipedConfig) + if err != nil { + lp.Error(err.Error()) + return err + } + lp.Successf("Successfully cloned repository %s", s.deployment.GitPath.Repo.Id) - // Load deployment configuration for this application. - var cfg *config.Config - cfg, err = loadDeploymentConfiguration(gitRepo.GetPath(), s.deployment) + // Copy and checkout the running revision. + if s.deployment.RunningCommitHash != "" { + runningRepoPath := filepath.Join(s.workingDir, workspaceGitRunningRepoDirName) + runningGitRepo, err := gitRepo.Copy(runningRepoPath) if err != nil { - err = fmt.Errorf("failed to load deployment configuration (%w)", err) lp.Error(err.Error()) - return + return err } - s.deploymentConfig = cfg - - pipelineable, ok := cfg.GetPipelineable() - if !ok { - err = fmt.Errorf("Unsupport non pipelineable application %s", cfg.Kind) + if err = runningGitRepo.Checkout(ctx, s.deployment.RunningCommitHash); err != nil { lp.Error(err.Error()) - return + return err } - s.pipelineable = pipelineable - lp.Success("Successfully loaded deployment configuration") + } - lp.Info("All preparations have been completed successfully") - }) - return err + // Load deployment configuration for this application. + cfg, err := loadDeploymentConfiguration(gitRepo.GetPath(), s.deployment) + if err != nil { + err = fmt.Errorf("failed to load deployment configuration (%w)", err) + lp.Error(err.Error()) + return err + } + s.deploymentConfig = cfg + + pipelineable, ok := cfg.GetPipelineable() + if !ok { + err = fmt.Errorf("Unsupport non pipelineable application %s", cfg.Kind) + lp.Error(err.Error()) + return err + } + s.pipelineable = pipelineable + lp.Success("Successfully loaded deployment configuration") + + s.prepared = true + lp.Info("All preparations have been completed successfully") + + return nil } func (s *scheduler) reportStageStatus(ctx context.Context, stageID string, status model.StageStatus, requires []string) error { From 6f22f914fcf4df878d53c9800b9720aea0b315a2 Mon Sep 17 00:00:00 2001 From: nghialv Date: Thu, 13 Aug 2020 16:34:59 +0900 Subject: [PATCH 2/3] Fix lints --- pkg/app/piped/controller/scheduler.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index 1e4377c0f6..640b4d418c 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -480,7 +480,7 @@ func (s *scheduler) ensurePreparing(ctx context.Context, lp logpersister.StageLo repoDirPath := filepath.Join(s.workingDir, workspaceGitRepoDirName) gitRepo, err := prepareDeployRepository(ctx, s.deployment, s.gitClient, repoDirPath, s.pipedConfig) if err != nil { - lp.Error(err.Error()) + lp.Errorf("Unable to prepare repository (%v)", err) return err } lp.Successf("Successfully cloned repository %s", s.deployment.GitPath.Repo.Id) @@ -490,11 +490,11 @@ func (s *scheduler) ensurePreparing(ctx context.Context, lp logpersister.StageLo runningRepoPath := filepath.Join(s.workingDir, workspaceGitRunningRepoDirName) runningGitRepo, err := gitRepo.Copy(runningRepoPath) if err != nil { - lp.Error(err.Error()) + lp.Errorf("Unable to copy repository (%v)", err) return err } if err = runningGitRepo.Checkout(ctx, s.deployment.RunningCommitHash); err != nil { - lp.Error(err.Error()) + lp.Errorf("Unable to checkout repository (%v)", err) return err } } @@ -503,18 +503,18 @@ func (s *scheduler) ensurePreparing(ctx context.Context, lp logpersister.StageLo cfg, err := loadDeploymentConfiguration(gitRepo.GetPath(), s.deployment) if err != nil { err = fmt.Errorf("failed to load deployment configuration (%w)", err) - lp.Error(err.Error()) + lp.Errorf("Failed to load deployment configuration (%v)", err) return err } s.deploymentConfig = cfg - pipelineable, ok := cfg.GetPipelineable() + pp, ok := cfg.GetPipelineable() if !ok { - err = fmt.Errorf("Unsupport non pipelineable application %s", cfg.Kind) - lp.Error(err.Error()) + err = fmt.Errorf("unsupport non pipelineable application %s", cfg.Kind) + lp.Errorf("Unsupport non pipelineable application %s", cfg.Kind) return err } - s.pipelineable = pipelineable + s.pipelineable = pp lp.Success("Successfully loaded deployment configuration") s.prepared = true From 1102d3195e8d1cdcba94ac31a8a982aded729d91 Mon Sep 17 00:00:00 2001 From: Le Van Nghia Date: Thu, 13 Aug 2020 16:42:57 +0900 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Ryo Nakao --- pkg/app/piped/controller/scheduler.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/app/piped/controller/scheduler.go b/pkg/app/piped/controller/scheduler.go index 640b4d418c..b84dfe69f1 100644 --- a/pkg/app/piped/controller/scheduler.go +++ b/pkg/app/piped/controller/scheduler.go @@ -502,17 +502,15 @@ func (s *scheduler) ensurePreparing(ctx context.Context, lp logpersister.StageLo // Load deployment configuration for this application. cfg, err := loadDeploymentConfiguration(gitRepo.GetPath(), s.deployment) if err != nil { - err = fmt.Errorf("failed to load deployment configuration (%w)", err) lp.Errorf("Failed to load deployment configuration (%v)", err) - return err + return fmt.Errorf("failed to load deployment configuration (%w)", err) } s.deploymentConfig = cfg pp, ok := cfg.GetPipelineable() if !ok { - err = fmt.Errorf("unsupport non pipelineable application %s", cfg.Kind) lp.Errorf("Unsupport non pipelineable application %s", cfg.Kind) - return err + return fmt.Errorf("unsupport non pipelineable application %s", cfg.Kind) } s.pipelineable = pp lp.Success("Successfully loaded deployment configuration")