Input digest tagger #5192

piotr-szybicki · 2020-12-28T13:03:22Z

Implementation of inputDigest tagger from the Improve taggers proposal This PR is much bigger than I originally intended. But whichever way I looked I was hitting the circular dependencies. So the first 2 commits are about extracting build.ArtifactGraph and build.Artifact into its own package.

It does mostly what internal skaffold hasher is doing a minus couple of things:

Doesn't hash the build arguments (I don't believe they are not relevant. )
Doesn't hash test dependencies (I don't believe they are not relevant. )

Some areas to think of (maybe in this or in future PR's):

following target in multi-stage docker builds
including image cotainerId in the tag. (But to get there the implementation of docker.GetDependencies would have to change including its contract of the method)
documentation what exactly is being used in the tag calculation (how only COPY and ADD commands can influence tag)

codecov · 2020-12-28T13:17:11Z

Codecov Report

Merging #5192 (4a6b4b2) into master (665b502) will decrease coverage by 0.16%.
The diff coverage is 53.60%.

@@            Coverage Diff             @@
##           master    #5192      +/-   ##
==========================================
- Coverage   70.68%   70.51%   -0.17%     
==========================================
  Files         408      410       +2     
  Lines       15581    15624      +43     
==========================================
+ Hits        11014    11018       +4     
- Misses       3757     3792      +35     
- Partials      810      814       +4

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/deploy.go	`56.52% <0.00%> (ø)`
cmd/skaffold/app/cmd/filter.go	`25.58% <ø> (ø)`
cmd/skaffold/app/cmd/render.go	`46.42% <0.00%> (ø)`
cmd/skaffold/app/flags/build_output.go	`85.71% <ø> (ø)`
pkg/skaffold/build/build.go	`0.00% <ø> (-85.72%)`	⬇️
pkg/skaffold/build/builder_mux.go	`34.37% <ø> (ø)`
pkg/skaffold/build/cache/cache.go	`52.00% <ø> (ø)`
pkg/skaffold/build/cache/hash.go	`71.28% <ø> (ø)`
pkg/skaffold/build/cache/types.go	`100.00% <ø> (ø)`
pkg/skaffold/build/gcb/types.go	`53.84% <ø> (ø)`
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 665b502...4a6b4b2. Read the comment docs.

briandealwis · 2021-01-14T15:42:52Z

Sorry for the delay in reviewing this @piotr-szybicki. I'll try to look today — this particular issue has been on my wishlist for a bit.

briandealwis

Hi @piotr-szybicki — I must apologize profusely for taking so long to review this PR. It generally looks great and is exactly what I was hoping for.

The one big quibble is the name of pkg/skaffold/build/dep. The team talked about this and we'd like to move this package up in the hierarchy and call it graph (so pkg/skaffold/graph).

If you're sick of this PR, I'm happy to take this on.

cmd/skaffold/app/cmd/run.go

pkg/skaffold/build/tag/input_digest_test.go

briandealwis · 2021-02-17T15:57:54Z

pkg/skaffold/build/tag/input_digest.go

+		return "", err
+	}
+	h.Write([]byte(fi.Mode().String()))
+	h.Write([]byte(fi.Name()))


Ideally this should be a file-path relative to the artifact workspace, and it should use filepath.ToPath() to ensure it's encoded consistently between Windows and *nix.

So there is no filepath.ToPath() function I used filepath.Clean() that seems to do the job. Also, Do you think I should change it in the hash.fileHasher()? For consistency.

briandealwis · 2021-02-17T15:58:37Z

pkg/skaffold/build/tag/input_digest.go

+	if err != nil {
+		return "", err
+	}
+	h.Write([]byte(fi.Mode().String()))


Since we're hashing inputs, I don't think the mode matters since that will be affected by my OS and local umask settings. And the builder itself may change these modes as it's writing files into the layers.

absolutely. Do you think I should also remove it from the hash.fileHasher()? Feels like the internal scaffold cache and the tagger should be consistent.

Ehh, never mind, they will never be consistent. I just took a look at the hash.go code and see that you are also hashing the build arguments. Totally make sense if you consider the development life cycle. Something that I don't think is appropriate to be a part of the image hash (Might be wrong on that one). I mean the way I see it we want to be close to what docker is doing with it's layer.DiffID.

pkg/skaffold/build/tag/input_digest.go

piotr-szybicki

Hi @piotr-szybicki — I must apologize profusely for taking so long to review this PR. It generally looks great and is exactly what I was hoping for.

The one big quibble is the name of pkg/skaffold/build/dep. The team talked about this and we'd like to move this package up in the hierarchy and call it graph (so pkg/skaffold/graph).

If you're sick of this PR, I'm happy to take this on.

Hi, no problem has been busy with other stuff myself and didn't chase. I looked over the comments and they make total sense I will implement all the changes you requested. I will also add some documentation as generally understanding how this works is not that trivial. As I found out. It's different for docker comparing to Bazel or jib.

piotr-szybicki · 2020-12-28T13:05:10Z

Makefile

@@ -14,7 +14,7 @@
 GOPATH ?= $(shell go env GOPATH)
 GOOS ?= $(shell go env GOOS)
 GOARCH ?= $(shell go env GOARCH)
-BUILD_DIR ?= ./out
+BUILD_DIR ?= /home/piotrszybicki/work/cube


I missed this, it's removed in the following commit.

briandealwis

Thanks @piotr-szybicki! I've put in a few comments. If we can get these addressed, then it'll be ready for commit.

#5492 (and #5504) brought to light that we need to incorporate more than just the inputs — and this input tagger should use the artifact-hashing mechanism that's used elsewhere in Skaffold. Unfortunately this artifact-hasher is not yet exposed in an easy-to-consume manner (mechanism, lister). This is a bigger change and can be done separately if you don't want to take it on (just let me know).

cmd/skaffold/app/cmd/run.go

pkg/skaffold/build/tag/input_digest.go

briandealwis · 2021-03-08T14:27:52Z

pkg/skaffold/build/tag/input_digest.go

+		inputs = append(inputs, h)
+	}
+
+	return encode(inputs)


Can we just inline the sha256 encoding of the file path and file contents directly into the for{} loop? inputs isn't used for any purpose and just adds memory pressure. Some artifacts — with their dependencies — may have hundreds or thousands of files.

I have very limited understanding of how this encoder works, but the json.Encoder that's being used takes the entire set of files and returns a single encoding based on that set. is it possible to encode each file individually and then combine the encodings to achieve the same result?

it's worth noting that the elements of inputs are the hashes of each file and its contents, not the entire file contents themselves. so while there could be thousands of files, this would be a set of thousands of hashes, each of which is only a few bytes.

the input slice contains only a file paths, I iterate over them in the encode method hashing as you said content and relative path. Maybe the name is misleading.

pkg/skaffold/build/tag/tag.go

nkubala · 2021-03-16T20:32:19Z

@piotr-szybicki I took the liberty of rebasing your PR (I made some changes that made the merge conflicts quite annoying for you) - I hope this is ok!

I also addressed a few of @briandealwis's last comments - mostly formatting and cleaning suggestions, though I made one change to the arguments to GenerateTag. please let me know if all of these changes are ok, and if so (pending @briandealwis's final review) we'll merge this!

piotr-szybicki · 2021-03-16T20:43:35Z

@nkubala this is absolutely fine. Thanks for doing it. I was about to do this myself but my full time job is killing me lately.

piotr-szybicki · 2021-03-16T20:49:55Z

Thanks @piotr-szybicki! I've put in a few comments. If we can get these addressed, then it'll be ready for commit.

#5492 (and #5504) brought to light that we need to incorporate more than just the inputs — and this input tagger should use the artifact-hashing mechanism that's used elsewhere in Skaffold. Unfortunately this artifact-hasher is not yet exposed in an easy-to-consume manner (mechanism, lister). This is a bigger change and can be done separately if you don't want to take it on (just let me know).

@briandealwis Sorry for the late response. But a year of my work is coming to a culmination release over coming weekend. I actually knew about this issue If you take a look at the original comment I made when creating this PR
including image cotainerId in the tag. (But to get there the implementation of docker.GetDependencies would have to change including its contract of the method)
I have a fix on a branch so I will create a different PR to address this particular issue. Because this one got big enough as it is.

briandealwis

LGTM once the InputDigest type is fixed up

docs/content/en/schemas/v2beta13.json

pkg/skaffold/kubernetes/owner_test.go

pkg/skaffold/schema/latest/config.go

pkg/skaffold/tag/tag.go

pkg/skaffold/tag/input_digest_test.go

piotr-szybicki · 2021-03-18T16:43:01Z

I fix all the comments, and tests. I of the Travis CI Jobs has flaked out on me. Can anyone rerun?

tejal29 · 2021-04-06T21:27:04Z

@piotr-szybicki sorry for the delay. do you have some cycles to rebase your PR. I would love to merge this.

nkubala · 2021-04-06T21:28:22Z

@tejal29 i'm on it :)

…orArtifact to a separate package to avoid cyclic dependecies problem

…st image name.

…over the file name with the full relative path. Remove mode from the hash. Fix some typos

…ew comments

nkubala · 2021-04-07T21:34:19Z

all merge conflicts resolved and CI finally showing all green, so I'm going to merge this! @piotr-szybicki thanks so much for the contribution and all of your patience!

mikekamornikov · 2021-04-09T15:17:03Z

@nkubala As I see InputDigest is not supported by CustomTemplateTagger (see CreateComponents in pkg/skaffold/tag/tagger_mux.go):

apiVersion: skaffold/v2beta14
kind: Config
profiles:
- name: test-custom-template
  build:
    tagPolicy:
      customTemplate:
        template: "release-aaa-{{.DIGEST}}"
        components:
        - name: DIGEST
          inputDigest: {}
    artifacts:
    - image: quay.io/my/project
      context: ../../..
      bazel:
        target: //project:docker.tar

$ skaffold build -p test-custom-template
creating runner: creating tagger: creating tagger: creating components: unknown component for custom template: DIGEST {GitTagger:<nil> ShaTagger:<nil> EnvTemplateTagger:<nil> DateTimeTagger:<nil> CustomTemplateTagger:<nil> InputDigest:0x7a479b8}
. If above error is unexpected, please open an issue https://github.com/GoogleContainerTools/skaffold/issues/new to report this error.

Is it by design? Should I file a bug?

piotr-szybicki · 2021-04-09T16:35:53Z

I just reread improve digest proposal. And yes I clearly missed {{.DIGEST}} when implementing. So filing a bug would be a good choice. I will find some time during the weekend to fix this.

piotr-szybicki requested review from loosebazooka, yuwenma and a team as code owners December 28, 2020 13:03

piotr-szybicki requested a review from ilya-zuyev December 28, 2020 13:03

pull-request-size bot added the size/XXL label Dec 28, 2020

google-cla bot added the cla: yes label Dec 28, 2020

piotr-szybicki mentioned this pull request Jan 4, 2021

proposal for user generated tagger. #5184

Merged

briandealwis reviewed Feb 25, 2021

View reviewed changes

piotr-szybicki commented Feb 25, 2021

View reviewed changes

piotr-szybicki force-pushed the InputDigestTagger branch 4 times, most recently from 663ebbe to bb37e68 Compare March 7, 2021 15:15

briandealwis reviewed Mar 8, 2021

View reviewed changes

nkubala force-pushed the InputDigestTagger branch from bb37e68 to fca78f9 Compare March 16, 2021 20:00

briandealwis approved these changes Mar 16, 2021

View reviewed changes

piotr-szybicki force-pushed the InputDigestTagger branch 4 times, most recently from 19ff291 to 75e30c9 Compare March 17, 2021 22:39

nkubala force-pushed the InputDigestTagger branch 2 times, most recently from 2951d84 to 2c44e0e Compare March 31, 2021 17:21

nkubala force-pushed the InputDigestTagger branch from 2c44e0e to 2388896 Compare April 1, 2021 21:19

piotr-szybicki and others added 9 commits April 7, 2021 10:44

extract build.Artifact buidld.DependencyGraph and build.DependenciesF…

5747fb2

…orArtifact to a separate package to avoid cyclic dependecies problem

changing GenerteTag contract to accept leatest.Artifact instead of ju…

0406004

…st image name.

implementing input digest tagger

d933074

implementing input digest tagger, adding tests

86aea0d

change name of the build/dep package to graph. Replace taking a hash …

a3ad708

…over the file name with the full relative path. Remove mode from the hash. Fix some typos

rebase and fix merge conflicts

9a3dc38

remove workdir from GenerateTag and pass artifact by value; code revi…

209d5a7

…ew comments

changing typ on inputTagger to InputDigest, some review comments

a299122

fix schemas

0ab3245

nkubala force-pushed the InputDigestTagger branch from 2388896 to 7099959 Compare April 7, 2021 18:34

fix merge conflicts and rewrite test

4a6b4b2

nkubala force-pushed the InputDigestTagger branch from 7099959 to 4a6b4b2 Compare April 7, 2021 20:32

nkubala merged commit b496731 into GoogleContainerTools:master Apr 7, 2021

This was referenced Apr 9, 2021

InputDigest is not supported by CustomTemplateTagger #5660

Closed

Add InputDigest support to CustomTemplateTagger #5661

Merged

nkubala mentioned this pull request Apr 13, 2021

Add back 'tag' digest source #5672

Merged

briandealwis mentioned this pull request May 13, 2021

Document inputDigest tagger and move sha256 tagger to end #5842

Closed

nkubala mentioned this pull request Jun 4, 2021

Custom tagging based on contributing files - eg digest-tagger #5103

Closed

aaron-prindle mentioned this pull request Dec 7, 2022

Add support for unique-per-image tag (was DIGEST_HEX) #2162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Input digest tagger #5192

Input digest tagger #5192

piotr-szybicki commented Dec 28, 2020 •

edited

Loading

codecov bot commented Dec 28, 2020 •

edited

Loading

briandealwis commented Jan 14, 2021

briandealwis left a comment

briandealwis Feb 17, 2021

piotr-szybicki Mar 6, 2021 •

edited

Loading

briandealwis Feb 17, 2021

piotr-szybicki Mar 6, 2021

piotr-szybicki Mar 6, 2021

piotr-szybicki left a comment •

edited

Loading

piotr-szybicki Dec 28, 2020

briandealwis left a comment

briandealwis Mar 8, 2021

nkubala Mar 16, 2021

piotr-szybicki Mar 16, 2021 •

edited

Loading

nkubala commented Mar 16, 2021

piotr-szybicki commented Mar 16, 2021

piotr-szybicki commented Mar 16, 2021

briandealwis left a comment

piotr-szybicki commented Mar 18, 2021

tejal29 commented Apr 6, 2021

nkubala commented Apr 6, 2021

nkubala commented Apr 7, 2021

mikekamornikov commented Apr 9, 2021

piotr-szybicki commented Apr 9, 2021

Input digest tagger #5192

Input digest tagger #5192

Conversation

piotr-szybicki commented Dec 28, 2020 • edited Loading

codecov bot commented Dec 28, 2020 • edited Loading

Codecov Report

briandealwis commented Jan 14, 2021

briandealwis left a comment

Choose a reason for hiding this comment

briandealwis Feb 17, 2021

Choose a reason for hiding this comment

piotr-szybicki Mar 6, 2021 • edited Loading

Choose a reason for hiding this comment

briandealwis Feb 17, 2021

Choose a reason for hiding this comment

piotr-szybicki Mar 6, 2021

Choose a reason for hiding this comment

piotr-szybicki Mar 6, 2021

Choose a reason for hiding this comment

piotr-szybicki left a comment • edited Loading

Choose a reason for hiding this comment

piotr-szybicki Dec 28, 2020

Choose a reason for hiding this comment

briandealwis left a comment

Choose a reason for hiding this comment

briandealwis Mar 8, 2021

Choose a reason for hiding this comment

nkubala Mar 16, 2021

Choose a reason for hiding this comment

piotr-szybicki Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

nkubala commented Mar 16, 2021

piotr-szybicki commented Mar 16, 2021

piotr-szybicki commented Mar 16, 2021

briandealwis left a comment

Choose a reason for hiding this comment

piotr-szybicki commented Mar 18, 2021

tejal29 commented Apr 6, 2021

nkubala commented Apr 6, 2021

nkubala commented Apr 7, 2021

mikekamornikov commented Apr 9, 2021

piotr-szybicki commented Apr 9, 2021

piotr-szybicki commented Dec 28, 2020 •

edited

Loading

codecov bot commented Dec 28, 2020 •

edited

Loading

piotr-szybicki Mar 6, 2021 •

edited

Loading

piotr-szybicki left a comment •

edited

Loading

piotr-szybicki Mar 16, 2021 •

edited

Loading