Skip to content
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

cirrus run: introduce --lazy-pull flag #517

Merged
merged 4 commits into from
May 16, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
22 changes: 21 additions & 1 deletion internal/commands/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ var affectedFilesGitRevision string
var affectedFilesGitCachedRevision string
var verbose bool

// Common instance-related flags.
var lazyPull bool

// Container-related flags.
var containerBackend string
var containerLazyPull bool
Expand All @@ -41,6 +44,9 @@ var containerLazyPull bool
var dockerfileImageTemplate string
var dockerfileImagePush bool

// Tart-related flags.
var tartLazyPull bool

// Flags useful for debugging.
var debugNoCleanup bool

Expand Down Expand Up @@ -120,13 +126,18 @@ func run(cmd *cobra.Command, args []string) error {

// Container-related options
executorOpts = append(executorOpts, executor.WithContainerOptions(options.ContainerOptions{
EagerPull: !containerLazyPull,
LazyPull: lazyPull || containerLazyPull,
NoCleanup: debugNoCleanup,

DockerfileImageTemplate: dockerfileImageTemplate,
DockerfileImagePush: dockerfileImagePush,
}))

// Tart-related options
executorOpts = append(executorOpts, executor.WithTartOptions(options.TartOptions{
LazyPull: lazyPull || tartLazyPull,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it also be called EagerPull for consistency&

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was called like that before ef89082, where I've changed the meaning so that each newly-initialized TartOptions struct would reflect the defaults.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the container option be renamed into LazyPull then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See 241da4f.

}))

// Environment
executorOpts = append(executorOpts,
executor.WithBaseEnvironmentOverride(baseEnvironment),
Expand Down Expand Up @@ -171,6 +182,11 @@ func newRunCmd() *cobra.Command {
cmd.PersistentFlags().StringVarP(&output, "output", "o", logs.DefaultFormat(), fmt.Sprintf("output format of logs, "+
"supported values: %s", strings.Join(logs.Formats(), ", ")))

// Common instance-related flags
cmd.PersistentFlags().BoolVar(&lazyPull, "lazy-pull", false,
"attempt to pull container and VM images only if they are missing locally "+
"(helpful in case of registry rate limits; enables --container-lazy-pull and --tart-lazy-pull)")

// Container-related flags
cmd.PersistentFlags().StringVar(&containerBackend, "container-backend", containerbackend.BackendAuto,
fmt.Sprintf("container engine backend to use, either \"%s\", \"%s\" or \"%s\"",
Expand All @@ -184,6 +200,10 @@ func newRunCmd() *cobra.Command {
cmd.PersistentFlags().BoolVar(&dockerfileImagePush, "dockerfile-image-push",
false, "whether to push whe image produced by the Dockerfile as CI environment feature")

// Tart-related flags
cmd.PersistentFlags().BoolVar(&tartLazyPull, "tart-lazy-pull", false,
"attempt to pull Tart VM images only if they are missing locally (helpful in case of registry rate limits)")

// Flags useful for debugging
cmd.PersistentFlags().BoolVar(&debugNoCleanup, "debug-no-cleanup", false,
"don't remove containers and volumes after execution")
Expand Down
2 changes: 2 additions & 0 deletions internal/executor/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ type Executor struct {
dirtyMode bool
containerBackend containerbackend.ContainerBackend
containerOptions options.ContainerOptions
tartOptions options.TartOptions
}

func New(projectDir string, tasks []*api.Task, opts ...Option) (*Executor, error) {
Expand Down Expand Up @@ -166,6 +167,7 @@ func (e *Executor) runSingleTask(ctx context.Context, task *build.Task) error {
TaskID: task.ID,
DirtyMode: e.dirtyMode,
ContainerOptions: e.containerOptions,
TartOptions: e.tartOptions,
}

instanceRunOpts.SetLogger(taskLogger)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ func New(vmName string, sshUser string, sshPassword string, cpu uint32, memory u
}

func (tart *Tart) Run(ctx context.Context, config *runconfig.RunConfig) (err error) {
vm, err := NewVMClonedFrom(ctx, tart.vmName, tart.cpu, tart.memory, config.Logger())
vm, err := NewVMClonedFrom(ctx, tart.vmName, tart.cpu, tart.memory, config.TartOptions.LazyPull,
config.Logger())
if err != nil {
return fmt.Errorf("%w: failed to create VM cloned from %q: %v", ErrFailed, tart.vmName, err)
}
Expand Down
23 changes: 22 additions & 1 deletion internal/executor/instance/persistentworker/isolation/tart/vm.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,14 @@ type VM struct {
errChan chan error
}

func NewVMClonedFrom(ctx context.Context, from string, cpu uint32, memory uint32, logger *echelon.Logger) (*VM, error) {
func NewVMClonedFrom(
ctx context.Context,
from string,
cpu uint32,
memory uint32,
lazyPull bool,
logger *echelon.Logger,
) (*VM, error) {
subCtx, subCtxCancel := context.WithCancel(ctx)

vm := &VM{
Expand All @@ -28,6 +35,20 @@ func NewVMClonedFrom(ctx context.Context, from string, cpu uint32, memory uint32
errChan: make(chan error, 1),
}

pullLogger := logger.Scoped("pull virtual machine")
if !lazyPull {
pullLogger.Infof("Pulling virtual machine %s...", from)

if _, _, err := CmdWithLogger(ctx, pullLogger, "pull", from); err != nil {
pullLogger.FinishWithType(echelon.FinishTypeSucceeded)
return nil, err
}

pullLogger.FinishWithType(echelon.FinishTypeSucceeded)
} else {
pullLogger.FinishWithType(echelon.FinishTypeSkipped)
fkorotkov marked this conversation as resolved.
Show resolved Hide resolved
}

cloneLogger := logger.Scoped("clone virtual machine")
cloneLogger.Infof("Cloning virtual machine %s...", from)

Expand Down
2 changes: 1 addition & 1 deletion internal/executor/instance/prebuilt.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ func (prebuilt *PrebuiltInstance) Run(ctx context.Context, config *runconfig.Run
Tags: []string{prebuilt.Image},
Dockerfile: prebuilt.Dockerfile,
BuildArgs: prebuilt.Arguments,
Pull: config.ContainerOptions.EagerPull,
Pull: !config.ContainerOptions.LazyPull,
})

Outer:
Expand Down
1 change: 1 addition & 0 deletions internal/executor/instance/runconfig/runconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ type RunConfig struct {
logger *echelon.Logger
DirtyMode bool
ContainerOptions options.ContainerOptions
TartOptions options.TartOptions
agentVersion string
}

Expand Down
6 changes: 6 additions & 0 deletions internal/executor/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,9 @@ func WithContainerBackend(containerBackend containerbackend.ContainerBackend) Op
e.containerBackend = containerBackend
}
}

func WithTartOptions(tartOptions options.TartOptions) Option {
return func(e *Executor) {
e.tartOptions = tartOptions
}
}
4 changes: 2 additions & 2 deletions internal/executor/options/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
)

type ContainerOptions struct {
EagerPull bool
LazyPull bool
NoPullImages []string
NoCleanup bool

Expand All @@ -26,7 +26,7 @@ func (copts ContainerOptions) ShouldPullImage(
}
}

if copts.EagerPull {
if !copts.LazyPull {
return true
}

Expand Down
3 changes: 1 addition & 2 deletions internal/executor/options/container_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (

func TestForcePull(t *testing.T) {
do := options.ContainerOptions{
EagerPull: true,
NoPullImages: []string{"nonexistent.invalid/should/not/be:pulled"},
}

Expand All @@ -24,7 +23,7 @@ func TestForcePull(t *testing.T) {
// Shouldn't be pulled because it's blacklisted
assert.False(t, do.ShouldPullImage(ctx, backend, "nonexistent.invalid/should/not/be:pulled"))

// Should be pulled because EagerPull is set to true
// Should be pulled because lazy pull is disabled by default
image := canaryImage()

if err := backend.ImagePull(ctx, image); err != nil {
Expand Down
5 changes: 5 additions & 0 deletions internal/executor/options/tart.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package options

type TartOptions struct {
LazyPull bool
}