From 80ad6813bf909057a625ac16f40cbae7e0326d52 Mon Sep 17 00:00:00 2001 From: tejal29 Date: Thu, 22 Oct 2020 11:39:44 -0700 Subject: [PATCH] fix race condition --- pkg/skaffold/docker/dependencies.go | 15 ++++-- pkg/skaffold/docker/dependencies_test.go | 8 +-- pkg/skaffold/docker/parse.go | 2 +- pkg/skaffold/errors/err_map.go | 9 ++-- pkg/skaffold/errors/errors_test.go | 66 ++++++++++++++++++++++++ 5 files changed, 86 insertions(+), 14 deletions(-) diff --git a/pkg/skaffold/docker/dependencies.go b/pkg/skaffold/docker/dependencies.go index 4dba83cd177..952c5c4c1a0 100644 --- a/pkg/skaffold/docker/dependencies.go +++ b/pkg/skaffold/docker/dependencies.go @@ -22,6 +22,7 @@ import ( "os" "path/filepath" "sort" + "sync" "github.com/docker/docker/builder/dockerignore" @@ -36,7 +37,7 @@ type dependency struct { // dependencyCache caches the results for `GetDependencies` for individual dockerfile. var ( - dependencyCache = map[string]dependency{} + dependencyCache = sync.Map{} ) // NormalizeDockerfilePath returns the absolute path to the dockerfile. @@ -61,14 +62,18 @@ func GetDependencies(ctx context.Context, workspace string, dockerfilePath strin return nil, fmt.Errorf("normalizing dockerfile path: %w", err) } - if _, ok := dependencyCache[absDockerfilePath]; !ok { + if v, ok := dependencyCache.Load(absDockerfilePath); !ok { paths, err := getDependencies(workspace, dockerfilePath, absDockerfilePath, buildArgs, cfg) - dependencyCache[absDockerfilePath] = dependency{ + dependencyCache.Store(absDockerfilePath, dependency{ files: paths, err: err, - } + }) + return paths, err + } else if cv, ok := v.(dependency); ok { + return cv.files, cv.err } - return dependencyCache[absDockerfilePath].files, dependencyCache[absDockerfilePath].err + // TODO: tejaldesai + return nil, fmt.Errorf("unexpected skaffold internal error encountered") } func getDependencies(workspace string, dockerfilePath string, absDockerfilePath string, buildArgs map[string]*string, cfg Config) ([]string, error) { diff --git a/pkg/skaffold/docker/dependencies_test.go b/pkg/skaffold/docker/dependencies_test.go index 2306d9d0e67..07243474856 100644 --- a/pkg/skaffold/docker/dependencies_test.go +++ b/pkg/skaffold/docker/dependencies_test.go @@ -21,6 +21,7 @@ import ( "fmt" "os" "path/filepath" + "sync" "testing" v1 "github.com/google/go-containerregistry/pkg/v1" @@ -683,14 +684,13 @@ func TestGetDependenciesCached(t *testing.T) { tmpDir := t.NewTempDir().Touch("server.go", "random.go") tmpDir.Write("Dockerfile", copyServerGo) // construct cache for abs dockerfile paths. - cache := map[string]dependency{} + defer func() { dependencyCache = sync.Map{} }() for k, v := range test.dependencyCache { - cache[tmpDir.Path(k)] = v + dependencyCache.Store(tmpDir.Path(k), v) } - t.Override(&dependencyCache, cache) deps, err := GetDependencies(context.Background(), tmpDir.Root(), "Dockerfile", map[string]*string{}, nil) t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, deps) - if _, ok := dependencyCache[tmpDir.Path("Dockerfile")]; !ok { + if _, ok := dependencyCache.Load(tmpDir.Path("Dockerfile")); !ok { t.Fatal("expected a cache entry for Dockerfile, did not found") } }) diff --git a/pkg/skaffold/docker/parse.go b/pkg/skaffold/docker/parse.go index 98b1e8ffac0..b286eb06a86 100644 --- a/pkg/skaffold/docker/parse.go +++ b/pkg/skaffold/docker/parse.go @@ -25,13 +25,13 @@ import ( "path/filepath" "strings" - sErrors "github.com/GoogleContainerTools/skaffold/pkg/skaffold/errors" v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/moby/buildkit/frontend/dockerfile/command" "github.com/moby/buildkit/frontend/dockerfile/parser" "github.com/moby/buildkit/frontend/dockerfile/shell" "github.com/sirupsen/logrus" + sErrors "github.com/GoogleContainerTools/skaffold/pkg/skaffold/errors" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" ) diff --git a/pkg/skaffold/errors/err_map.go b/pkg/skaffold/errors/err_map.go index 090f899fcd3..33e628a91ea 100644 --- a/pkg/skaffold/errors/err_map.go +++ b/pkg/skaffold/errors/err_map.go @@ -104,11 +104,12 @@ var ( regexp: re(retrieveFailedOldManifest), description: func(err error) string { matchExp := re(retrieveFailedOldManifest) - imageName := "specified image" - if match := matchExp.FindStringSubmatch(fmt.Sprintf("%s", err)); len(match) >= 2 { - imageName = fmt.Sprintf("image %s", match[1]) + match := matchExp.FindStringSubmatch(fmt.Sprintf("%s", err)) + pre := "Could not retrieve image pushed with the deprecated manifest v1" + if len(match) >= 3 && match[2] != "" { + pre = fmt.Sprintf("Could not retrieve image %s pushed with the deprecated manifest v1", match[2]) } - return fmt.Sprintf("Could not retrieve %s pushed with the deprecated manifest v1. Ignoring files dependencies for all ONBUILD triggers", imageName) + return fmt.Sprintf("%s. Ignoring files dependencies for all ONBUILD triggers", pre) }, errCode: proto.StatusCode_DEVINIT_UNSUPPORTED_V1_MANIFEST, suggestion: func(opts config.SkaffoldOptions) []*proto.Suggestion { diff --git a/pkg/skaffold/errors/errors_test.go b/pkg/skaffold/errors/errors_test.go index 475acd61d02..44e1f525928 100644 --- a/pkg/skaffold/errors/errors_test.go +++ b/pkg/skaffold/errors/errors_test.go @@ -258,6 +258,72 @@ func TestShowAIError(t *testing.T) { } } +func TestIsOldImageManifestProblem(t *testing.T) { + tests := []struct { + description string + command string + err error + expectedMsg string + expected bool + }{ + { + description: "dev command older manifest with image name", + command: "dev", + err: fmt.Errorf(`listing files: parsing ONBUILD instructions: retrieving image "library/ruby:2.3.0": unsupported MediaType: "application/vnd.docker.distribution.manifest.v1+prettyjws", see https://github.com/google/go-containerregistry/issues/377`), + expectedMsg: "Could not retrieve image library/ruby:2.3.0 pushed with the deprecated manifest v1. Ignoring files dependencies for all ONBUILD triggers. To avoid, hit Cntrl-C and run `docker pull` to fetch the specified image and retry.", + expected: true, + }, + { + description: "dev command older manifest without image name", + command: "dev", + err: fmt.Errorf(`unsupported MediaType: "application/vnd.docker.distribution.manifest.v1+prettyjws", see https://github.com/google/go-containerregistry/issues/377`), + expectedMsg: "Could not retrieve image pushed with the deprecated manifest v1. Ignoring files dependencies for all ONBUILD triggers. To avoid, hit Cntrl-C and run `docker pull` to fetch the specified image and retry.", + expected: true, + }, + { + description: "dev command with random name", + command: "dev", + err: fmt.Errorf(`listing files: parsing ONBUILD instructions: retrieve image "noimage" image does not exits`), + }, + { + description: "debug command older manifest", + command: "debug", + err: fmt.Errorf(`unsupported MediaType: "application/vnd.docker.distribution.manifest.v1+prettyjws", see https://github.com/google/go-containerregistry/issues/377`), + expectedMsg: "Could not retrieve image pushed with the deprecated manifest v1. Ignoring files dependencies for all ONBUILD triggers. To avoid, hit Cntrl-C and run `docker pull` to fetch the specified image and retry.", + expected: true, + }, + { + description: "build command older manifest", + command: "build", + err: fmt.Errorf(`unsupported MediaType: "application/vnd.docker.distribution.manifest.v1+prettyjws", see https://github.com/google/go-containerregistry/issues/377`), + expected: true, + }, + { + description: "run command older manifest", + command: "run", + err: fmt.Errorf(`unsupported MediaType: "application/vnd.docker.distribution.manifest.v1+prettyjws", see https://github.com/google/go-containerregistry/issues/377`), + expected: true, + }, + { + description: "deploy command older manifest", + command: "deploy", + err: fmt.Errorf(`unsupported MediaType: "application/vnd.docker.distribution.manifest.v1+prettyjws", see https://github.com/google/go-containerregistry/issues/377`), + expected: true, + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + skaffoldOpts = config.SkaffoldOptions{ + Command: test.command, + } + actualMsg, actual := IsOldImageManifestProblem(test.err) + fmt.Println(actualMsg) + t.CheckDeepEqual(test.expectedMsg, actualMsg) + t.CheckDeepEqual(test.expected, actual) + }) + } +} + func stringOrUndefined(s string) config.StringOrUndefined { c := &config.StringOrUndefined{} c.Set(s)