Make TestKanikoTaskRun more portable #1516

vdemeester · 2019-11-04T10:50:15Z

Changes

This test could, previously, only run on specific environment, and
would depend on GCP on the CI. This is not more the case with this
changes as it is now able to run on any kubernetes.

We start a registry in the same namespace the test happens, and we
push build/push the image on this local registry. To validate the
digest, we need to run a new pod using skopeo and jq to get the
remote digest.

Marking it as wip for now as it needs a little bit of refactoring 👼

Signed-off-by: Vincent Demeester [email protected]

/cc @afrittoli @bobcatfish @imjasonh

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

vdemeester · 2019-11-04T13:00:20Z

test/kaniko_task_test.go

+				Name:    "skopeo",
+				Image:   "quay.io/rhpipeline/skopeo:alpine",
+				Command: []string{"/bin/sh", "-c"},
+				Args:    []string{"apk add --no-cache jq 1>/dev/null; skopeo inspect --tls-verify=false docker://" + image + ":latest| jq '.Digest'"},


I'm gonna push some images to plumbing to use it instead of this hack 👼

gcr.io/tekton-releases/dogfooding/skopeo is now available

vdemeester · 2019-11-04T13:01:04Z

test/kaniko_task_test.go

+	if _, err := c.KubeClient.Kube.AppsV1().Deployments(namespace).Create(getRegistryDeployment(namespace)); err != nil {
+		t.Fatalf("Failed to create the local registry deployment: %v", err)
+	}
+	service := getRegistryService(namespace)
+	if _, err := c.KubeClient.Kube.CoreV1().Services(namespace).Create(service); err != nil {
+		t.Fatalf("Failed to create the local registry service: %v", err)
 	}
+	set := labels.Set(service.Spec.Selector)
+	if pods, err := c.KubeClient.Kube.CoreV1().Pods(namespace).List(metav1.ListOptions{LabelSelector: set.AsSelector().String()}); err != nil {
+		t.Fatalf("Failed to list Pods of service[%s] error:%v", service.GetName(), err)
+	} else {
+		if len(pods.Items) != 1 {
+			t.Fatalf("Only 1 pod for service %s should be running: %v", service, pods.Items)
+		}
+
+		if err := WaitForPodState(c, pods.Items[0].Name, namespace, func(pod *corev1.Pod) (bool, error) {
+			return pod.Status.Phase == "Running", nil
+		}, "PodContainersRunning"); err != nil {
+			t.Fatalf("Error waiting for Pod %q to run: %v", pods.Items[0].Name, err)
+		}
+	}


This part could be done as an option to setup(t) 👼

This test could, previously, only run on specific environment, and would depend on GCP on the CI. This is not more the case with this changes as it is now able to run on any kubernetes. We start a registry in the same namespace the test happens, and we push build/push the image on this local registry. To validate the digest, we need to run a new pod using `skopeo` and `jq` to get the remote digest. Signed-off-by: Vincent Demeester <[email protected]>

afrittoli

Thank you for this, I think it's a great direction for us to go.
I have a couple of comments - mainly I'm concerned with cleanup and isolation across tests.

afrittoli · 2019-11-05T10:39:00Z

test/kaniko_task_test.go

+			RestartPolicy: corev1.RestartPolicyNever,
+		},
+	}); err != nil {
+		t.Fatalf("Failed to create the local registry service: %v", err)


The log message doesn't look right here?

oh yeah, my bad =)

test/kaniko_task_test.go

test/init_test.go

afrittoli

/lgtm

And use this to extract the "local" registry part of TestKanikoTaskRun into a separate function, reusable in other tests. Signed-off-by: Vincent Demeester <[email protected]>

chmouel

/lgtm
/approve

went back and forth with vincent offline on this and explained how it works, nicely done 👍

dlorenc · 2019-11-08T14:33:48Z

/approve

tekton-robot · 2019-11-08T14:33:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chmouel, dlorenc

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~~ [dlorenc]

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

bobcatfish

This is super cool!!! I think there are a few places where I feel like future explorers might benefit from some additional docstrings + comments but otherwise this is beautiful!!! 😍

bobcatfish · 2019-11-08T14:57:52Z

test/registry_test.go

+	set := labels.Set(service.Spec.Selector)
+
+	// Give it a little bit of time to at least create the pod
+	time.Sleep(5 * time.Second)


could we use wait for to poll the pod instead?

We are actually waiting for the pod below, this is to give a little bit of time for the kube api-server to actually create the pod… (didn't find a good way on the service status to get that information for some reason). We may refine that though 😉

hm shouldn't we be able to query the kube api for the status before the pod is actually created?

i just find that generally arbitrarily sleeping in tests tends to cause pain over time (5 seconds can be a really long time sometimes) - even (or maybe especially) when the test is already pretty slow

i just find that generally arbitrarily sleeping in tests tends to cause pain over time (5 seconds can be a really long time sometimes) - even (or maybe especially) when the test is already pretty slow

I cannot agree more with " generally arbitrarily sleeping in tests tends to cause pain over time" 😛 The main problem with service is that the status is pretty "bare", there is only loadBalancer in there 🤦‍♂️. That said, I think I should be able to watch the deployment instead 👼 I'll include that in my follow-up 😉

bobcatfish · 2019-11-08T14:57:59Z

test/registry_test.go

+	"k8s.io/apimachinery/pkg/labels"
+)
+
+func withRegistry(t *testing.T, c *clients, namespace string) {


bobcatfish · 2019-11-08T14:58:30Z

test/registry_test.go

+			},
+		},
+	}
+}


i feel like these functions are super cool and reusable - what do you think about putting them in their own package and having docstrings?

@bobcatfish this is doable indeed 👼 I wanted to port more tests (like the helm one, …) before to see if I need to refine them before extracting them 👼

bobcatfish · 2019-11-08T14:59:54Z

test/kaniko_task_test.go

+				Name:    "skopeo",
+				Image:   "gcr.io/tekton-releases/dogfooding/skopeo:latest",
+				Command: []string{"/bin/sh", "-c"},
+				Args:    []string{"skopeo inspect --tls-verify=false docker://" + image + ":latest| jq '.Digest'"},


i feel like this and/or the function could use some comments to explain what skopeo is, what the function is all about and how exactly it determines the digest (looks like specifically looking for image with tag latest - which would mean if you don't tag latest, it wont work)

Indeed, I'll follow-up on this to add the comment on the function 👼
Also I feel the function name could be more explicit (like getRemoteDigestFromTheInside or something like that 😹)

tekton-robot requested review from afrittoli, bobcatfish and imjasonh November 4, 2019 10:50

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2019

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Nov 4, 2019

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 4, 2019

vdemeester force-pushed the test-e2e-portable-kaniko branch from 7494f85 to 3138ad3 Compare November 4, 2019 12:39

vdemeester commented Nov 4, 2019

View reviewed changes

vdemeester force-pushed the test-e2e-portable-kaniko branch 2 times, most recently from 189e76e to c5559f4 Compare November 4, 2019 13:59

vdemeester changed the title ~~wip: Make TestKanikoTaskRun more portable~~ Make TestKanikoTaskRun more portable Nov 4, 2019

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2019

vdemeester mentioned this pull request Nov 4, 2019

Some integration tests failed on local kube environment #746

Closed

vdemeester force-pushed the test-e2e-portable-kaniko branch from ca91307 to 74bff77 Compare November 5, 2019 10:21

afrittoli reviewed Nov 5, 2019

View reviewed changes

tekton-robot assigned afrittoli Nov 5, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2019

Refactor setup to take extra setup functions ⛽

5dfe457

And use this to extract the "local" registry part of TestKanikoTaskRun into a separate function, reusable in other tests. Signed-off-by: Vincent Demeester <[email protected]>

vdemeester force-pushed the test-e2e-portable-kaniko branch from 74bff77 to 5dfe457 Compare November 5, 2019 13:58

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2019

chmouel approved these changes Nov 8, 2019

View reviewed changes

tekton-robot assigned chmouel Nov 8, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2019

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2019

bobcatfish reviewed Nov 8, 2019

View reviewed changes

tekton-robot merged commit 50cf4bc into tektoncd:master Nov 8, 2019

vdemeester deleted the test-e2e-portable-kaniko branch November 8, 2019 15:05

vdemeester mentioned this pull request Nov 12, 2019

Kaniko test follow up #1558

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make TestKanikoTaskRun more portable #1516

Make TestKanikoTaskRun more portable #1516

vdemeester commented Nov 4, 2019

vdemeester Nov 4, 2019

afrittoli Nov 4, 2019

vdemeester Nov 4, 2019

afrittoli left a comment

afrittoli Nov 5, 2019

vdemeester Nov 5, 2019

afrittoli left a comment

chmouel left a comment •

edited

Loading

dlorenc commented Nov 8, 2019

tekton-robot commented Nov 8, 2019

bobcatfish left a comment

bobcatfish Nov 8, 2019

vdemeester Nov 8, 2019

bobcatfish Nov 8, 2019

vdemeester Nov 8, 2019

bobcatfish Nov 8, 2019

bobcatfish Nov 8, 2019

vdemeester Nov 8, 2019

bobcatfish Nov 8, 2019

vdemeester Nov 8, 2019

Make TestKanikoTaskRun more portable #1516

Make TestKanikoTaskRun more portable #1516

Conversation

vdemeester commented Nov 4, 2019

Changes

Submitter Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

chmouel left a comment • edited Loading

Choose a reason for hiding this comment

dlorenc commented Nov 8, 2019

tekton-robot commented Nov 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chmouel left a comment •

edited

Loading