Added digest cache, store digest instead of tags, added test #396

Zyqsempai · 2019-01-16T08:36:05Z

Added new cache instans for image -> digest step caching.
Moved common functionality for fetching remote image to dedicated function
Added test for tag -> digest functionality

Fixed: #333

Open questions:

The Digest pattern are the next sha256:bla-bla-bla, but we have NewDigest function inside this step https://github.com/knative/build-pipeline/blob/master/pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint.go#L123 which one wait for next pattern: image@sha256:bla-bla-bla, so who is right here?
In the case of cold cache we will be forced to warmup two caches instead of one, this can affect on perfomance.

Zyqsempai · 2019-01-16T09:21:19Z

cc @imjasonh @bobcatfish

bobcatfish · 2019-01-16T14:24:00Z

/ok-to-test

bobcatfish

Looking great to me! Just a few minor items to address - also a question for my own understanding about httptest :D

/approve

bobcatfish · 2019-01-16T14:24:38Z

pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint.go

@@ -115,27 +115,45 @@ func getEnvVar(cmd, args []string) (string, error) {
 // GetRemoteEntrypoint accepts a cache of image lookups, as well as the image
 // to look for. If the cache does not contain the image, it will lookup the
 // metadata from the images registry, and then commit that to the cache


could you update this comment to say digest instead of image? (the word image above i think mostly is referring literally to the image variable`)

bobcatfish · 2019-01-16T14:24:53Z

pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint.go

+// GetImageDigest tries to find and return image digest in cache, if
+// cache doesn't exists it will lookup the digest in remote image manifest
+// and then cache it
+func GetImageDigest(cache *Cache, image string) (string, error) {


i like how you broke this into a separate function!

bobcatfish · 2019-01-16T14:26:28Z

pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint.go

+// and then cache it
+func GetImageDigest(cache *Cache, image string) (string, error) {
+	if ep, ok := cache.get(image); ok {
+		return ep[0], nil


what is ep[0]? what does ep have in it? could it be an empty list?

Yep, sorry, my bad, fixed.

bobcatfish · 2019-01-16T14:26:44Z

pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint.go

+	if err != nil {
+		return "", fmt.Errorf("failed to fetch remote image %s: %v", image, err)
+	}
+	// Get Digest Hash struct


i think this comment kinda just repeats the line below it so im not sure we need it

bobcatfish · 2019-01-16T14:27:21Z

pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint.go

+	// Get Digest Hash struct
+	digestHash, err := img.Digest()
+	if err != nil {
+		return "", fmt.Errorf("couldn't get config for image %s: %v", image, err)


should this be "couldn't get digest hash for image"?

bobcatfish · 2019-01-16T14:28:00Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

 	bSpec := bs.DeepCopy()
 	for i := range bSpec.Steps {
 		step := &bSpec.Steps[i]
 		if len(step.Command) == 0 {
-			ep, err := entrypoint.GetRemoteEntrypoint(cache, step.Image)
+			digest, err := entrypoint.GetImageDigest(digestCache, step.Image)
+			ep, err := entrypoint.GetRemoteEntrypoint(entrypointCache, digest)


nice, im glad we do this in two steps 👍

@Zyqsempai Actually shouldn't entrypoint.GetRemoteEntrypoint take step.Image and digest ?
The digested image should be in the form of step.Image@digest otherwise the ref is wrong 😅

bobcatfish · 2019-01-16T14:30:55Z

pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint_test.go

+	digestHash, _ := img.Digest()
+	expectedDigetsSha := digestHash.String()
+	expectedRepo := "image"
+	manifestPath := fmt.Sprintf("/v2/%s/manifests/latest", expectedRepo)


might be nice to have a function (or functions!) to return digestHash and expectedRepo especially since this seems to be repeated b/w the two tests

Added only function to get digest like Str, since expetedRepo doesn't necessarily should be same for tests, and add function just to return on string seems like small overhead to me;)

kk makes sense! :D

bobcatfish · 2019-01-16T14:38:26Z

pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint_test.go

+	expectedRepo := "image"
+	manifestPath := fmt.Sprintf("/v2/%s/manifests/latest", expectedRepo)
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {


this is for my own understanding: how does httptest intercept requests?? i dont see anywhere where it is configured to listen on a particular port, or anywhere that we tell remote.Image to make requests to this server 🤔 (obviously it works but I don't understand how!)

httptest.NewServer creates a thing (listening to a port or a socket I don't remember) and to target it, server.URL will point to it. (See https://golang.org/src/net/http/httptest/server.go?s=2302:2346#L70, it runs an http.Server behind the scene 👼 )

func newLocalListener() net.Listener { if *serve != "" { l, err := net.Listen("tcp", *serve) if err != nil { panic(fmt.Sprintf("httptest: failed to listen on %v: %v", *serve, err)) } return l } l, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { if l, err = net.Listen("tcp6", "[::1]:0"); err != nil { panic(fmt.Sprintf("httptest: failed to listen on a port: %v", err)) } } return l }

Oooooooooh and then we use server.URL in image := path.Join(strings.TrimPrefix(server.URL, "http://"), "image:latest") ! Got it, thanks for explaining @vdemeester :D

vdemeester

sgtm 👍

vdemeester · 2019-01-16T15:15:24Z

pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint_test.go

+	expectedRepo := "image"
+	manifestPath := fmt.Sprintf("/v2/%s/manifests/latest", expectedRepo)
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {


httptest.NewServer creates a thing (listening to a port or a socket I don't remember) and to target it, server.URL will point to it. (See https://golang.org/src/net/http/httptest/server.go?s=2302:2346#L70, it runs an http.Server behind the scene 👼 )

func newLocalListener() net.Listener { if *serve != "" { l, err := net.Listen("tcp", *serve) if err != nil { panic(fmt.Sprintf("httptest: failed to listen on %v: %v", *serve, err)) } return l } l, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { if l, err = net.Listen("tcp6", "[::1]:0"); err != nil { panic(fmt.Sprintf("httptest: failed to listen on a port: %v", err)) } } return l }

Zyqsempai · 2019-01-16T15:34:49Z

Fixed comments. But I can't reach the test server, to check why the tests are failing.

vdemeester · 2019-01-16T16:03:12Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

 	bSpec := bs.DeepCopy()
 	for i := range bSpec.Steps {
 		step := &bSpec.Steps[i]
 		if len(step.Command) == 0 {
-			ep, err := entrypoint.GetRemoteEntrypoint(cache, step.Image)
+			digest, err := entrypoint.GetImageDigest(digestCache, step.Image)
+			ep, err := entrypoint.GetRemoteEntrypoint(entrypointCache, digest)


@Zyqsempai Actually shouldn't entrypoint.GetRemoteEntrypoint take step.Image and digest ?
The digested image should be in the form of step.Image@digest otherwise the ref is wrong 😅

Zyqsempai · 2019-01-16T16:03:22Z

@vdemeester Could you please post the failed test stucktrace, or another info, coz seems like i just doesn't have access to gubernator.knative.dev

vdemeester · 2019-01-16T16:05:09Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

 	bSpec := bs.DeepCopy()
 	for i := range bSpec.Steps {
 		step := &bSpec.Steps[i]
 		if len(step.Command) == 0 {
-			ep, err := entrypoint.GetRemoteEntrypoint(cache, step.Image)
+			digest, err := entrypoint.GetImageDigest(digestCache, step.Image)


this err need to be checked too before getting to the next line

vdemeester · 2019-01-16T16:07:10Z

@Zyqsempai the errors indicates the image reference seems to be wrong

status:
  conditions:
  - lastTransitionTime: "2019-01-16T15:42:04Z"
    message: 'References a Task arendelle-uvyhvmae/helm-reset-task that doesn''t exist:  could
      not get entrypoint from registry for alpine/helm: couldn''t get config for image
      sha256:1cc169daeea16a0011753c5727001f35142327c8f4dcb181558fc2ed04ad17f6: UNAUTHORIZED:
      "authentication required"'
    reason: CouldntGetTask
    status: "False"
    type: Succeeded
  podName: ""

See my comment above ^^

Zyqsempai · 2019-01-16T16:12:33Z

@vdemeester Seems like it's related to the problem with Digest you mentioned above.

Fix PR comments Fix PR comments 2

knative-metrics-robot · 2019-01-16T16:19:58Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint.go	83.8%	80.8%	-3.0
pkg/reconciler/v1alpha1/taskrun/taskrun.go	72.7%	71.3%	-1.3

bobcatfish · 2019-01-16T18:09:34Z

@vdemeester Could you please post the failed test stucktrace, or another info, coz seems like i just doesn't have access to gubernator.knative.dev

Is this to be expected @adrcunha ? Docs don't seem to indicate any requirements to see these logs: https://github.com/knative/docs/blob/9f8fe54d315f6a2ba32d3dadfc39312b44922f53/community/CONTRIBUTING.md#pull-requests (maybe you need to be a member of ~~knative-dev@~~ the knative org?)

Zyqsempai · 2019-01-16T20:39:03Z

Seems, like tests pathed can we merge now?

vdemeester

/lgtm

knative-prow-robot · 2019-01-17T12:53:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, vdemeester, Zyqsempai

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bobcatfish]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Fix image replacement

knative-prow-robot requested review from aaron-prindle and bobcatfish January 16, 2019 08:36

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 16, 2019

knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 16, 2019

bobcatfish reviewed Jan 16, 2019

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2019

vdemeester approved these changes Jan 16, 2019

View reviewed changes

Zyqsempai force-pushed the cache-image-digest branch from a6b62f8 to dd56fc3 Compare January 16, 2019 15:31

vdemeester requested changes Jan 16, 2019

View reviewed changes

vdemeester reviewed Jan 16, 2019

View reviewed changes

Added digest cache, store digest instead of tags, added test

c2fd4e5

Fix PR comments Fix PR comments 2

Zyqsempai force-pushed the cache-image-digest branch from dd56fc3 to c2fd4e5 Compare January 16, 2019 16:17

knative-prow-robot assigned vdemeester Jan 17, 2019

vdemeester approved these changes Jan 17, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2019

knative-prow-robot merged commit f2c5037 into tektoncd:master Jan 17, 2019

mchmarny unassigned vdemeester Mar 7, 2019

piyush-garg pushed a commit to piyush-garg/pipeline that referenced this pull request May 20, 2020

Merge pull request tektoncd#396 from piyush-garg/fix_replacement

fb881c1

Fix image replacement

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added digest cache, store digest instead of tags, added test #396

Added digest cache, store digest instead of tags, added test #396

Zyqsempai commented Jan 16, 2019 •

edited

Loading

Zyqsempai commented Jan 16, 2019

bobcatfish commented Jan 16, 2019

bobcatfish left a comment

bobcatfish Jan 16, 2019

bobcatfish Jan 16, 2019

bobcatfish Jan 16, 2019

Zyqsempai Jan 16, 2019

bobcatfish Jan 16, 2019

bobcatfish Jan 16, 2019

bobcatfish Jan 16, 2019

vdemeester Jan 16, 2019 •

edited

Loading

bobcatfish Jan 16, 2019

Zyqsempai Jan 16, 2019

bobcatfish Jan 16, 2019

bobcatfish Jan 16, 2019

vdemeester Jan 16, 2019

bobcatfish Jan 16, 2019

vdemeester left a comment

vdemeester Jan 16, 2019

Zyqsempai commented Jan 16, 2019

vdemeester Jan 16, 2019 •

edited

Loading

Zyqsempai commented Jan 16, 2019

vdemeester Jan 16, 2019

vdemeester commented Jan 16, 2019 •

edited

Loading

Zyqsempai commented Jan 16, 2019

knative-metrics-robot commented Jan 16, 2019

bobcatfish commented Jan 16, 2019 •

edited

Loading

Zyqsempai commented Jan 16, 2019

vdemeester left a comment

knative-prow-robot commented Jan 17, 2019

Added digest cache, store digest instead of tags, added test #396

Added digest cache, store digest instead of tags, added test #396

Conversation

Zyqsempai commented Jan 16, 2019 • edited Loading

Zyqsempai commented Jan 16, 2019

bobcatfish commented Jan 16, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zyqsempai commented Jan 16, 2019

vdemeester Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

Zyqsempai commented Jan 16, 2019

Choose a reason for hiding this comment

vdemeester commented Jan 16, 2019 • edited Loading

Zyqsempai commented Jan 16, 2019

knative-metrics-robot commented Jan 16, 2019

bobcatfish commented Jan 16, 2019 • edited Loading

Zyqsempai commented Jan 16, 2019

vdemeester left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jan 17, 2019

Zyqsempai commented Jan 16, 2019 •

edited

Loading

vdemeester Jan 16, 2019 •

edited

Loading

vdemeester Jan 16, 2019 •

edited

Loading

vdemeester commented Jan 16, 2019 •

edited

Loading

bobcatfish commented Jan 16, 2019 •

edited

Loading