Skip to content

fix: expand cache probe support to more directives #17

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

Merged
merged 3 commits into from
Jul 17, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions pkg/commands/arg.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ func (r *ArgCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bui
return nil
}

func (r *ArgCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
return r.ExecuteCommand(config, buildArgs)
}

func ParseArg(key string, val *string, env []string, ba *dockerfile.BuildArgs) (string, *string, error) {
replacementEnvs := ba.ReplacementEnvs(env)
resolvedKey, err := util.ResolveEnvironmentReplacement(key, replacementEnvs, false)
Expand Down
4 changes: 4 additions & 0 deletions pkg/commands/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ func (c *CmdCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bui
return nil
}

func (c *CmdCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
return c.ExecuteCommand(config, buildArgs)
}

// String returns some information about the command for the image config history
func (c *CmdCommand) String() string {
return c.cmd.String()
Expand Down
4 changes: 4 additions & 0 deletions pkg/commands/entrypoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ func (e *EntrypointCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerf
return nil
}

func (e *EntrypointCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
return e.ExecuteCommand(config, buildArgs)
}

// String returns some information about the command for the image config history
func (e *EntrypointCommand) String() string {
return e.cmd.String()
Expand Down
4 changes: 4 additions & 0 deletions pkg/commands/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ func (e *EnvCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bui
return util.UpdateConfigEnv(newEnvs, config, replacementEnvs)
}

func (e *EnvCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
return e.ExecuteCommand(config, buildArgs)
}

// String returns some information about the command for the image config history
func (e *EnvCommand) String() string {
return e.cmd.String()
Expand Down
4 changes: 4 additions & 0 deletions pkg/commands/expose.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ func (r *ExposeCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.
return nil
}

func (r *ExposeCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
return r.ExecuteCommand(config, buildArgs)
}

func validProtocol(protocol string) bool {
validProtocols := [2]string{"tcp", "udp"}
for _, p := range validProtocols {
Expand Down
4 changes: 4 additions & 0 deletions pkg/commands/healthcheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ func (h *HealthCheckCommand) ExecuteCommand(config *v1.Config, buildArgs *docker
return nil
}

func (h *HealthCheckCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
return h.ExecuteCommand(config, buildArgs)
}

// String returns some information about the command for the image config history
func (h *HealthCheckCommand) String() string {
return h.cmd.String()
Expand Down
4 changes: 4 additions & 0 deletions pkg/commands/label.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ func (r *LabelCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.B
return updateLabels(r.cmd.Labels, config, buildArgs)
}

func (r *LabelCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
return r.ExecuteCommand(config, buildArgs)
}

func updateLabels(labels []instructions.KeyValuePair, config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
existingLabels := config.Labels
if existingLabels == nil {
Expand Down
4 changes: 4 additions & 0 deletions pkg/commands/onbuild.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ func (o *OnBuildCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile
return nil
}

func (o *OnBuildCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
return o.ExecuteCommand(config, buildArgs)
}

// String returns some information about the command for the image config history
func (o *OnBuildCommand) String() string {
return o.cmd.String()
Expand Down
6 changes: 6 additions & 0 deletions pkg/commands/run_marker.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ func (r *RunMarkerCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfi
return nil
}

func (r *RunMarkerCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
// TODO(mafredri): Check if we need to do more here.
r.Files = []string{}
return nil
}

// String returns some information about the command for the image config
func (r *RunMarkerCommand) String() string {
return r.cmd.String()
Expand Down
4 changes: 4 additions & 0 deletions pkg/commands/shell.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ func (s *ShellCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.B
return nil
}

func (s *ShellCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
return s.ExecuteCommand(config, buildArgs)
}

// String returns some information about the command for the image config history
func (s *ShellCommand) String() string {
return s.cmd.String()
Expand Down
4 changes: 4 additions & 0 deletions pkg/commands/stopsignal.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ func (s *StopSignalCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerf
return nil
}

func (s *StopSignalCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
return s.ExecuteCommand(config, buildArgs)
}

// String returns some information about the command for the image config history
func (s *StopSignalCommand) String() string {
return s.cmd.String()
Expand Down
4 changes: 4 additions & 0 deletions pkg/commands/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ func (r *UserCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bu
return nil
}

func (r *UserCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
return r.ExecuteCommand(config, buildArgs)
}

func (r *UserCommand) String() string {
return r.cmd.String()
}
22 changes: 22 additions & 0 deletions pkg/commands/volume.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,28 @@ func (v *VolumeCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.
return nil
}

func (v *VolumeCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
logrus.Info("Cmd: VOLUME")
volumes := v.cmd.Volumes
replacementEnvs := buildArgs.ReplacementEnvs(config.Env)
resolvedVolumes, err := util.ResolveEnvironmentReplacementList(volumes, replacementEnvs, true)
if err != nil {
return err
}
existingVolumes := config.Volumes
if existingVolumes == nil {
existingVolumes = map[string]struct{}{}
}
for _, volume := range resolvedVolumes {
var x struct{}
existingVolumes[volume] = x
util.AddVolumePathToIgnoreList(volume)
}
config.Volumes = existingVolumes
Copy link
Member

Choose a reason for hiding this comment

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

maybe add logrus.Info() to indicate which volumes will be ignored while building?

Copy link
Member Author

Choose a reason for hiding this comment

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

This logic is copy paste from the uncached execute command. I don't think we should alter the behavior here 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking only about improvements of debugging life, but it can be left as is 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

That's valid. We could always add it to both but maybe that's suited for a separate debugging PR.


return nil
}

func (v *VolumeCommand) FilesToSnapshot() []string {
return []string{}
}
Expand Down
24 changes: 24 additions & 0 deletions pkg/commands/workdir.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,30 @@ func (w *WorkdirCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile
return nil
}

func (w *WorkdirCommand) CachedExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
logrus.Info("Cmd: workdir")
Copy link
Member

Choose a reason for hiding this comment

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

nit: wouldn't it be noisy?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is copy paste from the uncached execute, felt it's better to mimic the behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha 👍

workdirPath := w.cmd.Path
replacementEnvs := buildArgs.ReplacementEnvs(config.Env)
resolvedWorkingDir, err := util.ResolveEnvironmentReplacement(workdirPath, replacementEnvs, true)
if err != nil {
return err
}
if filepath.IsAbs(resolvedWorkingDir) {
config.WorkingDir = resolvedWorkingDir
} else {
if config.WorkingDir != "" {
config.WorkingDir = filepath.Join(config.WorkingDir, resolvedWorkingDir)
} else {
config.WorkingDir = filepath.Join("/", resolvedWorkingDir)
}
}
logrus.Infof("Changed working directory to %s", config.WorkingDir)

// TODO(mafredri): Check if we need to do more here.
Copy link
Member

Choose a reason for hiding this comment

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

What are you concerned about?

Copy link
Member Author

Choose a reason for hiding this comment

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

If this command runs in a build (uncached) and the target directory doesn't exist, it will be created and added to w.snapshotFiles. My concern is if we need to mimic this behavior or not when running cached.

Testing or experience should tell us, though, so maybe I'll just remove the TODO.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with leaving TODO here, we can always remove it later. Thanks for sharing the context!

w.snapshotFiles = []string{}
return nil
}

// FilesToSnapshot returns the workingdir, which should have been created if it didn't already exist
func (w *WorkdirCommand) FilesToSnapshot() []string {
return w.snapshotFiles
Expand Down
16 changes: 10 additions & 6 deletions pkg/executor/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,6 @@ func (s *stageBuilder) build() error {

// probeCache builds a stage entirely from the build cache.
// All COPY and RUN commands are faked.
// Note: USER and ENV commands are not supported.
func (s *stageBuilder) probeCache() error {
// Set the initial cache key to be the base image digest, the build args and the SrcContext.
var compositeKey *CompositeCache
Expand Down Expand Up @@ -501,12 +500,17 @@ func (s *stageBuilder) probeCache() error {
}
} else {
switch command.(type) {
case *commands.UserCommand:
case *commands.AddCommand:
// The ADD directive does not support caching.
return errors.Errorf("ADD is not supported in cache probe mode, use COPY instead")
case *commands.CopyCommand:
// If the cache is valid, we expect CachingCopyCommand.
return errors.Errorf("uncached COPY command is not supported in cache probe mode")
case *commands.RunCommand:
// If the cache is valid, we expect CachingRunCommand.
return errors.Errorf("uncached RUN command is not supported in cache probe mode")
default:
return errors.Errorf("uncached command %T encountered when probing cache", command)
}
if err := command.ExecuteCommand(&s.cf.Config, s.args); err != nil {
return errors.Wrap(err, "failed to execute command")
return errors.Errorf("unsupported command %T encountered in cache probe mode, missing CachedExecuteCommand", command)
}
}
files = command.FilesToSnapshot()
Expand Down
Loading
Loading