-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Cache docker.getDependencies
and skip inspecting remote images with old manifest
#4896
Cache docker.getDependencies
and skip inspecting remote images with old manifest
#4896
Conversation
I don't think we should be throwing the same error three times, even if it's only on the first cycle of the dev loop - our output is already getting cluttered enough as it is. also, this error doesn't seem particularly useful...what should a user do when they see this? if skaffold is resolving this itself by pulling the image, it seems to me like we should knock the severity of this log message to |
It self-recovers because when application is deployed, the images with old manifest will be retrieved. Another approach we could do is
to
And follow up with a code change to re-compute build dependencies for artifact. WDYT? |
yeah it sucks. Do you suggest we refactor the code and cache the results of |
We covered this in today's standup. Currently, we don't re-compute build dependencies, sync dependencies at end of dev loop. In case user add a new file which matches sync rules or is part of build, it never gets watched. |
c23d49a
to
80ad681
Compare
Codecov Report
@@ Coverage Diff @@
## master #4896 +/- ##
==========================================
+ Coverage 72.15% 72.17% +0.01%
==========================================
Files 363 363
Lines 12777 12811 +34
==========================================
+ Hits 9219 9246 +27
- Misses 2874 2877 +3
- Partials 684 688 +4
Continue to review full report at Codecov.
|
TODO: Don't merge. I am seeing some race conditions. |
80ad681
to
d8d3b56
Compare
docker.getDependencies
and skip inspecting remote docker images if they can
docker.getDependencies
and skip inspecting remote docker images if they candocker.getDependencies
and skip inspecting remote images with old manifest
cbe3ee2
to
222a0cf
Compare
@@ -42,30 +54,48 @@ func NormalizeDockerfilePath(context, dockerfile string) (string, error) { | |||
return filepath.Abs(rel) | |||
} | |||
|
|||
// GetDependencies finds the sources dependencies for the given docker artifact. | |||
// GetDependencies finds the sources dependency for the given docker artifact. | |||
// All paths are relative to the workspace. | |||
func GetDependencies(ctx context.Context, workspace string, dockerfilePath string, buildArgs map[string]*string, cfg Config) ([]string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is doing exactly what SyncStore provides should we use that instead of adding new package dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or modify the SyncStore
implementation to this since this looks to be using fewer underlying constructs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but maybe we should change the implementation of SyncStore to the way you did with a single singleflight.Group{}
and sync.Map
and use that struct. Would help the next person wanting something similar.
222a0cf
to
00557b0
Compare
I started making making changes to |
Unblock #3367 by
ONBULD [COPY/ADD]
triggers defined in the base image will not be watched.COPY/ADD
command.In this PR, for iterative watch mode (
dev
,debug
), show a warning to user if they depend on an Image pushed with older v1 Manifest.In watch mode,
Skaffold relies on go-containerregistry to retrieve a remote image to check if
ONBUILD
triggers exists and parse onbuild copy/add commands.However, go-containerregistry does not support retrieving image with old manifests and throw an unsupported media type error reported by the user in #3367 ( see google/go-containerregistry#377) for more context.
Note:
docker pull
can pull the older v1 manifests images and hence deploy as well build could succeed ifONBUILD
triggers are configured to just run executables in base imageIn this PR,
docker pull
to retrieve the image locally and re-runIn non-watch modes like
build
,deploy
andrun
we don't show the warning to user.Impact of skipping and continuing with the skaffold build VS Failing
In
build
,deploy
andrun
, skaffold callsGetDependencies
to create a tar context which makes a call to parseOnBUILD
triggers.There is a chance, that file dependencies in a base image
OnBUILD
triggers might fail the build.e.g. Consider a base image
foo
pushed with v1 manifestNow, the above base image defines a
ONBUILD
trigger to copy a filefoo
present in derived artifacts workspace to src/Consider, the project workspace where
foo
base image is used is as follows.The Dockerfile contents are
In this case, because skaffold skipped parsing the
ONBUILD COPY
command, file foo will not be watched or included in the context.tarRunning
skaffold build
for this setup will fail,So the impact of this change is, we are now failing later (attempting to perform a build) than fast. However, by failing fast, we are also failing for cases where these
ONBUILD
triggers justRUN
executables in base image vsCOPY
a file from derived artifact's workspace.If we do well on translating this build error to give users more help e.g.
Skaffold build, after a pull is successful.
Future Work
Testing this out
Trying this out.
Modify any examples to use this image
FROM library/ruby:2.3.0
in their DockerfileRun
skaffold dev
For
skaffold build
: no warning is shown. In my case, the build passes asONBUILD
trigger for base imageruby
just runs a command.Note:
docker.getDependencies
only prints the warning once for a dev loop.