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

When the image is remote and cached, skaffold tries to pull it locally and ignores the cache. #9177

Closed
3droj7 opened this issue Nov 15, 2023 · 3 comments · Fixed by #9181
Closed

Comments

@3droj7
Copy link
Contributor

3droj7 commented Nov 15, 2023

Hey,
We're using helm to deploy our images to the EKS and the images themselves are saved in the gitlab registry.
When we're trying to use skaffold, it will always try to download the image locally when it shouldn't because the deployment doesn't use the local images.

There's seems to be a bug in

if entry, err = c.tryImport(ctx, a, tag, hash, pl); err != nil {

It tries to import the image when the image can be remote and doesn't need to be imported.
func isImageLocal(runCtx *runcontext.RunContext, imageName string) (bool, error) {

Also I would suggest to put some debug logs to understand if skaffold decided that the image is remote or local.

Expected behavior

Checking cache...

  • manager_microservice: Found

Actual behavior

Checking cache...

  • manager_microservice: Not found. Building

Information

  • Skaffold version: 2.9.0
  • Operating system: ubuntu 22.04
  • Installed via: skaffold.dev
  • Contents of skaffold.yaml:
apiVersion: skaffold/v4beta8
kind: Config
metadata:
  name: manager-microservice
build:
  cluster: {}
  artifacts:
    - image: manager_microservice
      context: ../../
      sync: {}
      custom:
        buildCommand: Build/buildx.sh "Manager/DtmMicroservice/Dockerfile" development
        dependencies:
          dockerfile:
            path: Manager/DtmMicroservice/Dockerfile
  tagPolicy:
    inputDigest: {}

skaffold build --default-repo registry.gitlab.com/.... --platform=linux/amd64 -v debug

@ericzzzzzzz
Copy link
Contributor

@3droj7 Hi, I'm trying to understand this issue.

You provided a build command but you also mentioned about deployment with a remote image. This sounds you don't need build and just want to use the image on the remote registry?

Or you want skaffold to compute the inputDigest then check cache, if not available, check local, check remote, tryImport happen in sequence, if any found then (store and) return result?

@3droj7
Copy link
Contributor Author

3droj7 commented Nov 16, 2023

@ericzzzzzzz I actually meant "skafold run". What I expect to happen is the following -

  1. It will compute the inputDigest
  2. Check remote cache
  3. If it found the image inside the cache then it moves on to the deployment stage
  4. The deployment stage will deploy the image to the EKS without pulling it locally
  5. If the image is not found in the cache then it will build it, push it into the repository and then deploy

What happens instead is -

  1. It computes the inputDigest
  2. Checks the remote cache
  3. If found then it pulls it - Downloads the whole image locally
  4. If pullMissing is False then it builds the image even if the repository cache contains it.
  5. Then after the image is local, it pushes it to the remote and then deploys it

I saw that by doing the following change, it solves it (specifically in the build phase) -

--- a/pkg/skaffold/build/cache/lookup.go
+++ b/pkg/skaffold/build/cache/lookup.go
@@ -75,22 +75,34 @@ func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tag string, plat
        pls := platforms.GetPlatforms(a.ImageName)
        // TODO (gaghosh): allow `tryImport` when the Docker daemon starts supporting multiarch images
        // See https://github.com/docker/buildx/issues/1220#issuecomment-1189996403
-       if !cacheHit && !pls.IsMultiPlatform() {
-               var pl v1.Platform
-               if len(pls.Platforms) == 1 {
-                       pl = util.ConvertToV1Platform(pls.Platforms[0])
-               }
-               if entry, err = c.tryImport(ctx, a, tag, hash, pl); err != nil {
-                       log.Entry(ctx).Debugf("Could not import artifact from Docker, building instead (%s)", err)
-                       return needsBuilding{hash: hash}
-               }
-       }

        if isLocal, err := c.isLocalImage(a.ImageName); err != nil {
                return failed{err}
        } else if isLocal {
+               if !cacheHit && !pls.IsMultiPlatform() {
+                       var pl v1.Platform
+                       if len(pls.Platforms) == 1 {
+                               pl = util.ConvertToV1Platform(pls.Platforms[0])
+                       }
+                       if entry, err = c.tryImport(ctx, a, tag, hash, pl); err != nil {
+                               log.Entry(ctx).Debugf("Could not import artifact from Docker, building instead (%s)", err)
+                               return needsBuilding{hash: hash}
+                       }
+       }
                return c.lookupLocal(ctx, hash, tag, entry)
        }
+       if !cacheHit {
+               entry := ImageDetails{}
+               if digest, err := docker.RemoteDigest(tag, c.cfg, nil); err == nil {
+                       log.Entry(ctx).Debugf("Added digest for %s to cache entry", tag)
+                       entry.Digest = digest
+               }
+               log.Entry(ctx).Debugf("remote digest Error %s", err)
+
+               c.cacheMutex.Lock()
+               c.artifactCache[hash] = entry
+               c.cacheMutex.Unlock()
+       }
        return c.lookupRemote(ctx, hash, tag, pls.Platforms, entry)
 }
 

But now, I have some issues in the deployment phase and not sure if it was because of the change or something else

@3droj7
Copy link
Contributor Author

3droj7 commented Nov 18, 2023

Apparently the deployment problem was on my end but the cache and the pulling issue were real problems so I've created a PR -
#9181

It probably could be better (with less code duplication) but I'm not that aware of the code base and it seems to work for me also when trying to use the cluster and local images.

ericzzzzzzz pushed a commit that referenced this issue Nov 30, 2023
* fix: puling images when working with a remote repository (#9177)

If the skaffold is configured to use the cluster and not the local images, it will not pull the images locally.
If everything is cached it will move to deployment stage instead of unnecessarily downloading the images.

* Fixed the tests

---------

Co-authored-by: daniel <daniel@cybellum.com>
ericzzzzzzz added a commit that referenced this issue Feb 15, 2024
* Revert "fix(lookupRemote): fixed lookup.go lookupRemote to compare remote and cached digests (#9278)"

This reverts commit 9ff4546.

* Revert "fix: Scope Issue with the 'entry' variable when looking up remote images and tests additions (#9211)"

This reverts commit ffe769c.

* Revert "fix: puling images when working with a remote repository (#9177) (#9181)"

This reverts commit 9376dc0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants