Merge pull request #901 from traPtitech/feat/builder-timeout

Add builder timeout
traPtitech · Apr 18, 2024 · b5c6878 · b5c6878
2 parents 05d84de + b57001e
commit b5c6878
Show file tree

Hide file tree

Showing 6 changed files with 68 additions and 32 deletions.
diff --git a/.local-dev/config/ns.yaml b/.local-dev/config/ns.yaml
@@ -35,6 +35,7 @@ components:
     controller:
       url: http://ns-controller:10000
     priority: 0
+    stepTimeout: "1h"
 
   controller:
     port: 10000

diff --git a/cmd/config.go b/cmd/config.go
@@ -51,9 +51,10 @@ type BuilderConfig struct {
 	Buildkit struct {
 		Address string `mapstructure:"address" yaml:"address"`
 	} `mapstructure:"buildkit" yaml:"buildkit"`
-	Buildpack  buildpack.Config                   `mapstructure:"buildpack" yaml:"buildpack"`
-	Controller grpc.ControllerServiceClientConfig `mapstructure:"controller" yaml:"controller"`
-	Priority   int                                `mapstructure:"priority" yaml:"priority"`
+	Buildpack   buildpack.Config                   `mapstructure:"buildpack" yaml:"buildpack"`
+	Controller  grpc.ControllerServiceClientConfig `mapstructure:"controller" yaml:"controller"`
+	Priority    int                                `mapstructure:"priority" yaml:"priority"`
+	StepTimeout string                             `mapstructure:"stepTimeout" yaml:"stepTimeout"`
 }
 
 type ControllerConfig struct {
@@ -151,6 +152,7 @@ func init() {
 	viper.SetDefault("components.builder.controller.url", "http://ns-controller:10000")
 
 	viper.SetDefault("components.builder.priority", 0)
+	viper.SetDefault("components.builder.stepTimeout", "1h")
 
 	viper.SetDefault("components.controller.port", 10000)
 	viper.SetDefault("components.controller.tokenHeader", "X-NS-Controller-Token")

diff --git a/cmd/providers.go b/cmd/providers.go
@@ -101,6 +101,7 @@ var providers = wire.NewSet(
 	provideAuthDevServer,
 	provideBuildpackHelperServer,
 	buildpack.NewBuildpackBackend,
+	provideBuilderConfig,
 	provideBuildkitClient,
 	provideControllerServer,
 	provideContainerLogger,
@@ -172,6 +173,20 @@ func provideBuildpackHelperServer(
 	return &buildpackhelper.APIServer{H2CServer: web.NewH2CServer(wc)}
 }
 
+func provideBuilderConfig(c Config) (*ubuilder.Config, error) {
+	stepTimeoutStr := c.Components.Builder.StepTimeout
+	stepTimeout, err := time.ParseDuration(stepTimeoutStr)
+	if err != nil {
+		return nil, errors.Wrap(err, fmt.Sprintf("failed to parse components.builder.stepTimeout value: %s", stepTimeoutStr))
+	}
+	if stepTimeout <= 0 {
+		return nil, errors.Errorf("components.builder.stepTimeout must be positive: %s", stepTimeoutStr)
+	}
+	return &ubuilder.Config{
+		StepTimeout: stepTimeout,
+	}, nil
+}
+
 func provideBuildkitClient(c Config) (*buildkit.Client, error) {
 	cc := c.Components.Builder
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)

diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go
diff --git a/pkg/usecase/builder/build.go b/pkg/usecase/builder/build.go
@@ -8,7 +8,6 @@ import (
 	"github.com/friendsofgo/errors"
 	buildkit "github.com/moby/buildkit/client"
 	log "github.com/sirupsen/logrus"
-	gstatus "google.golang.org/grpc/status"
 
 	"github.com/traPtitech/neoshowcase/pkg/domain"
 	"github.com/traPtitech/neoshowcase/pkg/infrastructure/grpc/pb"
@@ -61,80 +60,80 @@ func (s *builderService) startBuild(req *domain.StartBuildRequest) error {
 
 type buildStep struct {
 	desc string
-	fn   func() error
+	fn   func(ctx context.Context) error
 }
 
-func (s *builderService) buildSteps(ctx context.Context, st *state) ([]buildStep, error) {
+func (s *builderService) buildSteps(st *state) ([]buildStep, error) {
 	var steps []buildStep
 
-	steps = append(steps, buildStep{"Repository Clone", func() error {
+	steps = append(steps, buildStep{"Repository Clone", func(ctx context.Context) error {
 		return s.cloneRepository(ctx, st)
 	}})
 
 	switch bc := st.app.Config.BuildConfig.(type) {
 	case *domain.BuildConfigRuntimeBuildpack:
-		steps = append(steps, buildStep{"Build (Runtime Buildpack)", func() error {
+		steps = append(steps, buildStep{"Build (Runtime Buildpack)", func(ctx context.Context) error {
 			return s.buildRuntimeBuildpack(ctx, st, bc)
 		}})
 	case *domain.BuildConfigRuntimeCmd:
-		steps = append(steps, buildStep{"Build (Runtime Command)", func() error {
+		steps = append(steps, buildStep{"Build (Runtime Command)", func(ctx context.Context) error {
 			return withBuildkitProgress(ctx, st.logWriter, func(ctx context.Context, ch chan *buildkit.SolveStatus) error {
 				return s.buildRuntimeCmd(ctx, st, ch, bc)
 			})
 		}})
 	case *domain.BuildConfigRuntimeDockerfile:
-		steps = append(steps, buildStep{"Build (Runtime Dockerfile)", func() error {
+		steps = append(steps, buildStep{"Build (Runtime Dockerfile)", func(ctx context.Context) error {
 			return withBuildkitProgress(ctx, st.logWriter, func(ctx context.Context, ch chan *buildkit.SolveStatus) error {
 				return s.buildRuntimeDockerfile(ctx, st, ch, bc)
 			})
 		}})
 	case *domain.BuildConfigStaticBuildpack:
-		steps = append(steps, buildStep{"Build (Static Buildpack)", func() error {
+		steps = append(steps, buildStep{"Build (Static Buildpack)", func(ctx context.Context) error {
 			return s.buildStaticBuildpackPack(ctx, st, bc)
 		}})
-		steps = append(steps, buildStep{"Extract from Temporary Image", func() error {
+		steps = append(steps, buildStep{"Extract from Temporary Image", func(ctx context.Context) error {
 			return withBuildkitProgress(ctx, st.logWriter, func(ctx context.Context, ch chan *buildkit.SolveStatus) error {
 				return s.buildStaticExtract(ctx, st, ch)
 			})
 		}})
-		steps = append(steps, buildStep{"Cleanup Temporary Image", func() error {
+		steps = append(steps, buildStep{"Cleanup Temporary Image", func(ctx context.Context) error {
 			return s.buildStaticCleanup(ctx, st)
 		}})
-		steps = append(steps, buildStep{"Save Artifact", func() error {
+		steps = append(steps, buildStep{"Save Artifact", func(ctx context.Context) error {
 			return s.saveArtifact(ctx, st)
 		}})
 	case *domain.BuildConfigStaticCmd:
-		steps = append(steps, buildStep{"Build (Static Command)", func() error {
+		steps = append(steps, buildStep{"Build (Static Command)", func(ctx context.Context) error {
 			return withBuildkitProgress(ctx, st.logWriter, func(ctx context.Context, ch chan *buildkit.SolveStatus) error {
 				return s.buildStaticCmd(ctx, st, ch, bc)
 			})
 		}})
-		steps = append(steps, buildStep{"Extract from Temporary Image", func() error {
+		steps = append(steps, buildStep{"Extract from Temporary Image", func(ctx context.Context) error {
 			return withBuildkitProgress(ctx, st.logWriter, func(ctx context.Context, ch chan *buildkit.SolveStatus) error {
 				return s.buildStaticExtract(ctx, st, ch)
 			})
 		}})
-		steps = append(steps, buildStep{"Cleanup Temporary Image", func() error {
+		steps = append(steps, buildStep{"Cleanup Temporary Image", func(ctx context.Context) error {
 			return s.buildStaticCleanup(ctx, st)
 		}})
-		steps = append(steps, buildStep{"Save Artifact", func() error {
+		steps = append(steps, buildStep{"Save Artifact", func(ctx context.Context) error {
 			return s.saveArtifact(ctx, st)
 		}})
 	case *domain.BuildConfigStaticDockerfile:
-		steps = append(steps, buildStep{"Build (Static Dockerfile)", func() error {
+		steps = append(steps, buildStep{"Build (Static Dockerfile)", func(ctx context.Context) error {
 			return withBuildkitProgress(ctx, st.logWriter, func(ctx context.Context, ch chan *buildkit.SolveStatus) error {
 				return s.buildStaticDockerfile(ctx, st, ch, bc)
 			})
 		}})
-		steps = append(steps, buildStep{"Extract from Temporary Image", func() error {
+		steps = append(steps, buildStep{"Extract from Temporary Image", func(ctx context.Context) error {
 			return withBuildkitProgress(ctx, st.logWriter, func(ctx context.Context, ch chan *buildkit.SolveStatus) error {
 				return s.buildStaticExtract(ctx, st, ch)
 			})
 		}})
-		steps = append(steps, buildStep{"Cleanup Temporary Image", func() error {
+		steps = append(steps, buildStep{"Cleanup Temporary Image", func(ctx context.Context) error {
 			return s.buildStaticCleanup(ctx, st)
 		}})
-		steps = append(steps, buildStep{"Save Artifact", func() error {
+		steps = append(steps, buildStep{"Save Artifact", func(ctx context.Context) error {
 			return s.saveArtifact(ctx, st)
 		}})
 	default:
@@ -152,7 +151,7 @@ func (s *builderService) process(ctx context.Context, st *state) domain.BuildSta
 	version, revision := cli.GetVersion()
 	st.WriteLog(fmt.Sprintf("[ns-builder] Version %v (%v)", version, revision))
 
-	steps, err := s.buildSteps(ctx, st)
+	steps, err := s.buildSteps(st)
 	if err != nil {
 		log.Errorf("calculating build steps: %+v", err)
 		st.WriteLog(fmt.Sprintf("[ns-builder] Error calculating build steps: %v", err))
@@ -162,22 +161,30 @@ func (s *builderService) process(ctx context.Context, st *state) domain.BuildSta
 	for i, step := range steps {
 		st.WriteLog(fmt.Sprintf("[ns-builder] ==> (%d/%d) %s", i+1, len(steps), step.desc))
 		start := time.Now()
-		err := step.fn()
+
+		// Execute step with timeout
+		childCtx, cancel := context.WithTimeout(ctx, s.config.StepTimeout)
+		err := step.fn(childCtx)
+		cancel()
+
+		// First, check if ctx was cancelled from parent
+		// - this (usually) means the user pressed 'Cancel' button to cancel the build
 		if cerr := ctx.Err(); cerr != nil {
 			st.WriteLog(fmt.Sprintf("[ns-builder] Build cancelled by user: %v", cerr))
 			return domain.BuildStatusCanceled
 		}
-		if errors.Is(err, context.Canceled) ||
-			errors.Is(err, context.DeadlineExceeded) ||
-			errors.Is(err, gstatus.FromContextError(context.Canceled).Err()) {
-			st.WriteLog(fmt.Sprintf("[ns-builder] Build cancelled: %v", err))
-			return domain.BuildStatusCanceled
+		// Check if the childCtx timed out from the configured "StepTimeout"
+		if errors.Is(err, context.DeadlineExceeded) {
+			st.WriteLog(fmt.Sprintf("[ns-builder] Build timed out: %v", err))
+			return domain.BuildStatusFailed
 		}
+		// Other reasons such as build script failure
 		if err != nil {
 			log.Errorf("Build failed for %v: %+v", st.build.ID, err)
 			st.WriteLog(fmt.Sprintf("[ns-builder] Build failed: %v", err))
 			return domain.BuildStatusFailed
 		}
+
 		st.WriteLog(fmt.Sprintf("[ns-builder] ==> (%d/%d) Done (%v).", i+1, len(steps), time.Since(start)))
 	}
 

diff --git a/pkg/usecase/builder/service.go b/pkg/usecase/builder/service.go
@@ -18,12 +18,17 @@ import (
 	"github.com/traPtitech/neoshowcase/pkg/util/retry"
 )
 
+type Config struct {
+	StepTimeout time.Duration
+}
+
 type Service interface {
 	Start(ctx context.Context) error
 	Shutdown(ctx context.Context) error
 }
 
 type builderService struct {
+	config    *Config
 	client    domain.ControllerBuilderServiceClient
 	buildkit  *buildkit.Client
 	buildpack builder.BuildpackBackend
@@ -39,6 +44,7 @@ type builderService struct {
 }
 
 func NewService(
+	config *Config,
 	client domain.ControllerBuilderServiceClient,
 	buildkit *buildkit.Client,
 	buildpack builder.BuildpackBackend,
@@ -52,6 +58,7 @@ func NewService(
 		return nil, errors.Wrap(err, "failed to convert into public key")
 	}
 	return &builderService{
+		config:    config,
 		client:    client,
 		buildkit:  buildkit,
 		buildpack: buildpack,