-
Notifications
You must be signed in to change notification settings - Fork 439
chore: concurrent image build with context cancellation #4760
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.
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
Changes from all commits
e444b27
bdfef7a
d3f05d8
331caa0
b95253b
82333d4
6f58be0
11ef4d8
e6e6f61
13665e2
4478437
c199567
526a906
272650c
9a52d48
9adf16b
2cf1bda
397b04b
97393fc
068d43f
4cd0f33
f0c9853
28bb762
bfd918c
0830e39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,12 +5,15 @@ package deploy | |||||||||||||||
|
|
||||||||||||||||
| import ( | ||||||||||||||||
| "bytes" | ||||||||||||||||
| "context" | ||||||||||||||||
| "errors" | ||||||||||||||||
| "fmt" | ||||||||||||||||
| "io" | ||||||||||||||||
| "os" | ||||||||||||||||
| "path/filepath" | ||||||||||||||||
| "strings" | ||||||||||||||||
| "sync" | ||||||||||||||||
| "time" | ||||||||||||||||
|
|
||||||||||||||||
| "github.com/aws/aws-sdk-go/aws" | ||||||||||||||||
| "github.com/aws/aws-sdk-go/aws/session" | ||||||||||||||||
|
|
@@ -35,11 +38,14 @@ import ( | |||||||||||||||
| "github.com/aws/copilot-cli/internal/pkg/template/artifactpath" | ||||||||||||||||
| "github.com/aws/copilot-cli/internal/pkg/template/diff" | ||||||||||||||||
| "github.com/aws/copilot-cli/internal/pkg/term/color" | ||||||||||||||||
| "github.com/aws/copilot-cli/internal/pkg/term/cursor" | ||||||||||||||||
| "github.com/aws/copilot-cli/internal/pkg/term/log" | ||||||||||||||||
| termprogress "github.com/aws/copilot-cli/internal/pkg/term/progress" | ||||||||||||||||
| "github.com/aws/copilot-cli/internal/pkg/term/syncbuffer" | ||||||||||||||||
| "github.com/aws/copilot-cli/internal/pkg/version" | ||||||||||||||||
| "github.com/aws/copilot-cli/internal/pkg/workspace" | ||||||||||||||||
| "github.com/spf13/afero" | ||||||||||||||||
| "golang.org/x/sync/errgroup" | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| const ( | ||||||||||||||||
|
|
@@ -56,6 +62,11 @@ const ( | |||||||||||||||
| labelForVersion = "com.aws.copilot.image.version" | ||||||||||||||||
| labelForContainerName = "com.aws.copilot.image.container.name" | ||||||||||||||||
| ) | ||||||||||||||||
| const ( | ||||||||||||||||
| paddingInSpacesForBuildAndPush = 5 | ||||||||||||||||
| pollIntervalForBuildAndPush = 60 * time.Millisecond | ||||||||||||||||
| defaultNumLinesForBuildAndPush = 5 | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| // ActionRecommender contains methods that output action recommendation. | ||||||||||||||||
| type ActionRecommender interface { | ||||||||||||||||
|
|
@@ -70,7 +81,7 @@ func (noopActionRecommender) RecommendedActions() []string { | |||||||||||||||
|
|
||||||||||||||||
| type repositoryService interface { | ||||||||||||||||
| Login() (string, error) | ||||||||||||||||
| BuildAndPush(args *dockerengine.BuildArguments) (string, error) | ||||||||||||||||
| BuildAndPush(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) (string, error) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| type templater interface { | ||||||||||||||||
|
|
@@ -105,6 +116,11 @@ type spinner interface { | |||||||||||||||
| Stop(label string) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| type labeledTermPrinter interface { | ||||||||||||||||
| IsDone() bool | ||||||||||||||||
| Print() error | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // StackRuntimeConfiguration contains runtime configuration for a workload CloudFormation stack. | ||||||||||||||||
| type StackRuntimeConfiguration struct { | ||||||||||||||||
| ImageDigests map[string]ContainerImageIdentifier // Container name to image. | ||||||||||||||||
|
|
@@ -150,18 +166,19 @@ type workloadDeployer struct { | |||||||||||||||
| workspacePath string | ||||||||||||||||
|
|
||||||||||||||||
| // Dependencies. | ||||||||||||||||
| fs afero.Fs | ||||||||||||||||
| s3Client uploader | ||||||||||||||||
| addons stackBuilder | ||||||||||||||||
| repository repositoryService | ||||||||||||||||
| deployer serviceDeployer | ||||||||||||||||
| tmplGetter deployedTemplateGetter | ||||||||||||||||
| endpointGetter endpointGetter | ||||||||||||||||
| spinner spinner | ||||||||||||||||
| templateFS template.Reader | ||||||||||||||||
| envVersionGetter versionGetter | ||||||||||||||||
| overrider Overrider | ||||||||||||||||
| customResources customResourcesFunc | ||||||||||||||||
| fs afero.Fs | ||||||||||||||||
| s3Client uploader | ||||||||||||||||
| addons stackBuilder | ||||||||||||||||
| repository repositoryService | ||||||||||||||||
| deployer serviceDeployer | ||||||||||||||||
| tmplGetter deployedTemplateGetter | ||||||||||||||||
| endpointGetter endpointGetter | ||||||||||||||||
| spinner spinner | ||||||||||||||||
| templateFS template.Reader | ||||||||||||||||
| envVersionGetter versionGetter | ||||||||||||||||
| overrider Overrider | ||||||||||||||||
| customResources customResourcesFunc | ||||||||||||||||
| labeledTermPrinter func(fw syncbuffer.FileWriter, bufs []*syncbuffer.LabeledSyncBuffer, opts ...syncbuffer.LabeledTermPrinterOption) labeledTermPrinter | ||||||||||||||||
|
|
||||||||||||||||
| // Cached variables. | ||||||||||||||||
| defaultSess *session.Session | ||||||||||||||||
|
|
@@ -251,6 +268,9 @@ func newWorkloadDeployer(in *WorkloadDeployerInput) (*workloadDeployer, error) { | |||||||||||||||
|
|
||||||||||||||||
| cfn := cloudformation.New(envSession, cloudformation.WithProgressTracker(os.Stderr)) | ||||||||||||||||
|
|
||||||||||||||||
| labeledTermPrinter := func(fw syncbuffer.FileWriter, bufs []*syncbuffer.LabeledSyncBuffer, opts ...syncbuffer.LabeledTermPrinterOption) labeledTermPrinter { | ||||||||||||||||
| return syncbuffer.NewLabeledTermPrinter(fw, bufs, opts...) | ||||||||||||||||
| } | ||||||||||||||||
| return &workloadDeployer{ | ||||||||||||||||
| name: in.Name, | ||||||||||||||||
| app: in.App, | ||||||||||||||||
|
|
@@ -275,6 +295,7 @@ func newWorkloadDeployer(in *WorkloadDeployerInput) (*workloadDeployer, error) { | |||||||||||||||
| envSess: envSession, | ||||||||||||||||
| store: store, | ||||||||||||||||
| envConfig: envConfig, | ||||||||||||||||
| labeledTermPrinter: labeledTermPrinter, | ||||||||||||||||
|
|
||||||||||||||||
| mft: in.Mft, | ||||||||||||||||
| rawMft: in.RawMft, | ||||||||||||||||
|
|
@@ -367,24 +388,66 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err | |||||||||||||||
| if err != nil { | ||||||||||||||||
| return fmt.Errorf("login to image repository: %w", err) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| var digestsMu sync.Mutex | ||||||||||||||||
| out.ImageDigests = make(map[string]ContainerImageIdentifier, len(buildArgsPerContainer)) | ||||||||||||||||
| var labeledBuffers []*syncbuffer.LabeledSyncBuffer | ||||||||||||||||
| g, ctx := errgroup.WithContext(context.Background()) | ||||||||||||||||
| cursor := cursor.New() | ||||||||||||||||
| cursor.Hide() | ||||||||||||||||
| for name, buildArgs := range buildArgsPerContainer { | ||||||||||||||||
| // create a copy of loop variables to avoid data race. | ||||||||||||||||
|
KollaAdithya marked this conversation as resolved.
|
||||||||||||||||
| name := name | ||||||||||||||||
| buildArgs := buildArgs | ||||||||||||||||
|
|
||||||||||||||||
| buildArgs.URI = uri | ||||||||||||||||
| buildArgsList, err := buildArgs.GenerateDockerBuildArgs(dockerengine.New(exec.NewCmd())) | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| return fmt.Errorf("generate docker build args: %w", err) | ||||||||||||||||
| } | ||||||||||||||||
| // TODO(adi) handle this log in syncBuffer's Print function | ||||||||||||||||
| log.Infof("Building your container image: docker %s\n", strings.Join(buildArgsList, " ")) | ||||||||||||||||
| digest, err := d.repository.BuildAndPush(buildArgs) | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| return fmt.Errorf("build and push image: %w", err) | ||||||||||||||||
| return fmt.Errorf("generate docker build args for %q: %w", name, err) | ||||||||||||||||
| } | ||||||||||||||||
| out.ImageDigests[name] = ContainerImageIdentifier{ | ||||||||||||||||
| Digest: digest, | ||||||||||||||||
| CustomTag: d.image.CustomTag, | ||||||||||||||||
| GitShortCommitTag: d.image.GitShortCommitTag, | ||||||||||||||||
| buf := syncbuffer.New() | ||||||||||||||||
| labeledBuffers = append(labeledBuffers, buf.WithLabel(fmt.Sprintf("Building your container image %q: docker %s", name, strings.Join(buildArgsList, " ")))) | ||||||||||||||||
| pr, pw := io.Pipe() | ||||||||||||||||
| g.Go(func() error { | ||||||||||||||||
| defer pw.Close() | ||||||||||||||||
| digest, err := d.repository.BuildAndPush(ctx, buildArgs, pw) | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| return fmt.Errorf("build and push the image %q: %w", name, err) | ||||||||||||||||
| } | ||||||||||||||||
| digestsMu.Lock() | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it good to unlock the map before |
||||||||||||||||
| defer digestsMu.Unlock() | ||||||||||||||||
| out.ImageDigests[name] = ContainerImageIdentifier{ | ||||||||||||||||
| Digest: digest, | ||||||||||||||||
| CustomTag: d.image.CustomTag, | ||||||||||||||||
| GitShortCommitTag: d.image.GitShortCommitTag, | ||||||||||||||||
| } | ||||||||||||||||
| return nil | ||||||||||||||||
| }) | ||||||||||||||||
| g.Go(func() error { | ||||||||||||||||
| if err := buf.Copy(pr); err != nil { | ||||||||||||||||
|
efekarakus marked this conversation as resolved.
|
||||||||||||||||
| return fmt.Errorf("copy build and push output for %q: %w", name, err) | ||||||||||||||||
| } | ||||||||||||||||
| return nil | ||||||||||||||||
| }) | ||||||||||||||||
| } | ||||||||||||||||
| opts := []syncbuffer.LabeledTermPrinterOption{syncbuffer.WithPadding(paddingInSpacesForBuildAndPush)} | ||||||||||||||||
| if os.Getenv("CI") != "true" { | ||||||||||||||||
| opts = append(opts, syncbuffer.WithNumLines(defaultNumLinesForBuildAndPush)) | ||||||||||||||||
| } | ||||||||||||||||
| ltp := d.labeledTermPrinter(os.Stderr, labeledBuffers, opts...) | ||||||||||||||||
| g.Go(func() error { | ||||||||||||||||
| for { | ||||||||||||||||
| if err := ltp.Print(); err != nil { | ||||||||||||||||
| return fmt.Errorf("print logs: %w", err) | ||||||||||||||||
| } | ||||||||||||||||
| if ltp.IsDone() { | ||||||||||||||||
| return nil | ||||||||||||||||
| } | ||||||||||||||||
| time.Sleep(pollIntervalForBuildAndPush) | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
so this function stops if ctx is cancelled! |
||||||||||||||||
| } | ||||||||||||||||
| }) | ||||||||||||||||
| if err := g.Wait(); err != nil { | ||||||||||||||||
| return err | ||||||||||||||||
| } | ||||||||||||||||
| return nil | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.