First prow integration test: sinker #20451

chaodaiG · 2021-01-12T16:08:18Z

This is the first integration test added for prow, as designed in https://docs.google.com/document/d/1hIHIoApoR4OUs_esBDE7A778wi-jUEZcr2-a0zVTqW0/edit.

The integration test deploys prow components in KIND cluster and test prow functions inside the cluster.

/assign @cjwagner @alvaroaleman @fejta

chaodaiG · 2021-01-12T16:08:36Z

/test pull-test-infra-integration

chaodaiG · 2021-01-12T16:48:33Z

/retest

chaodaiG · 2021-01-12T16:59:25Z

The integration test took 6 min 40 sec: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/test-infra/20451/pull-test-infra-integration/1349025501456371712

matthyx · 2021-01-12T17:08:07Z

awesome
/lgtm
/hold

petr-muller · 2021-01-13T12:55:16Z

prow/test/integration/setup-cluster.sh

+
+# Install nginx and wait for it ready
+echo "Install nginx on kind cluster"
+kubectl --context=${CONTEXT} apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/master/deploy/static/provider/kind/deploy.yaml


Can we rely on this URL to stay this way? How likely is this to break?

Yeah agreed, we should refer a concrete revision here rather than just master

Good point, pinned to a revision rather than master

petr-muller · 2021-01-13T12:55:50Z

prow/test/integration/setup-cluster.sh

+    help: "https://kind.sigs.k8s.io/docs/user/local-registry/"
+EOF
+
+# Install nginx and wait for it ready


We don't seem to wait for it here though.

Good catch, the wait was deferred to later step to make this run faster, removed the comment

petr-muller · 2021-01-13T12:56:27Z

prow/test/integration/setup-prow.sh

+echo "Push test image to registry"
+docker pull busybox
+docker tag busybox:latest localhost:5000/busybox:latest
+docker push localhost:5000/busybox:latest


Won't we hit the dockerhub rate limits?

Good point. What do you think about craning this image to gcr?

We already publish an alpine image with Prow. I think that is suitable for this. gcr.io/k8s-prow/alpine

Great, done

Looks like we're still tagging the image as busybox and referencing that in the pod. That works, but it'd be better to name it accurately.

petr-muller · 2021-01-13T12:59:08Z

prow/test/integration/test/sinker_test.go

+			// if err != nil {
+			// 	t.Fatalf("Failed stat %q: %v", defaultKubeconfig, err)
+			// }
+			// t.Logf("Stat of %q: %v\n\n%v\n\n%v", defaultKubeconfig, stat.Mode(), stat.Sys(), stat)


debug code? do we need to keep this?

yeah, deleted

alvaroaleman

Awesome to see some progress here :)

alvaroaleman · 2021-01-13T15:12:32Z

prow/test/integration/prow/cluster/100_namespace.yaml

+data:
+  oauth: ZmFrZW9hdXRodG9rZW4K # From 'fakeoauthtoken'
+---
+apiVersion: apiextensions.k8s.io/v1beta1


Would it be possible to use the manifests from the config directory (maybe the starter-s3.yaml) to make sure they are correct?

That would also be what I prefer, since majority of the manifests are identical. But there will be slight difference in deployment config, such as the image path, and future github-endpoint for github related integration tests, as well as other services that need mock(s). These can be achieved though, by various different method, such as kustomize, but might need some maintenance. What do you think?

alvaroaleman · 2021-01-13T15:13:10Z

prow/test/integration/prow/config.yaml

+
+prowjob_namespace: default
+pod_namespace: test-pods
+log_level: debug


You can just omit the job config file, it isn't mandatory (I can not comment on an empty file)

alvaroaleman · 2021-01-13T15:14:34Z

prow/test/integration/setup-cluster.sh

+
+# Install nginx and wait for it ready
+echo "Install nginx on kind cluster"
+kubectl --context=${CONTEXT} apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/master/deploy/static/provider/kind/deploy.yaml


Yeah agreed, we should refer a concrete revision here rather than just master

alvaroaleman · 2021-01-13T15:15:47Z

prow/test/integration/test.sh

+
+CURRENT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd -P )"
+
+if [[ -n "${CI:-}" ]]; then


I would just check for the presence of the kind binary rather than making assumptions about where it is and isn't present

alvaroaleman · 2021-01-13T15:16:16Z

prow/test/integration/test.sh

+  chmod +x /usr/bin/kind
+
+  # TODO(chaodaiG): remove this once bazel is installed in test image
+  echo "Install bazel for prow"


Same here, checking for thhe presence of bazel rather than for running in CI makes IMHO more sense

which bazel would be more robust by avoiding assuming the installation path. If you want to ensure a specific bazel version check with bazel --version. This installation doesn't create a binary called bazel or add anything to the path so I wouldn't expect the bazel command below to work.

I still don't think this works? If I have bazel installed, but don't have the correct bazel version, this will download the correct version, but then continue to use the version I originally had installed.
What is the need for requiring such a specific bazel version?

This solves the problem of the test image has bazel installed but not at the version required, it felt to me that bazel is smart enough to figure out which version to use?

alvaroaleman · 2021-01-13T15:17:21Z

prow/test/integration/test/sinker_test.go

+
+	for _, tt := range tests {
+		tt := tt
+		name := tt.name


this isn't needed, since you already capture tt

alvaroaleman · 2021-01-13T15:18:32Z

prow/test/integration/test/setup.go

+	return defaultKubeconfig
+}
+
+func NewClients(configPath, clusterName string) (*kubernetes.Clientset, *prow.Clientset, error) {


I can only recommend to use the controller-runtime client for this, as it is one client that allows you to interact with all object kinds

alvaroaleman · 2021-01-13T15:20:07Z

prow/test/integration/test/sinker_test.go

+			}
+			t.Logf("Pod is running: %s", name)
+
+			// Make sure pod is deleted, it'll take roughly 2 minutes


This takes two minutes with a five second resync period, are you sure?

The deletion action starts pretty fast, but completion of the deletion can take more than 1 minute

alvaroaleman · 2021-01-13T15:21:43Z

prow/test/integration/test/sinker_test.go

+					Delete(ctx, name, v1.DeleteOptions{})
+			})
+
+			if tt.hasCRD {


err, please rename to hasCR, all tests have the CRD.

alvaroaleman · 2021-01-13T15:25:27Z

prow/test/integration/test/sinker_test.go

+				}
+				return !exist, nil
+			})
+			pods, err := kubeClient.CoreV1().Pods(testpodNamespace).List(ctx, v1.ListOptions{})


Can we just capture the exists variable in this scope and avoid the second list, pod iteration etc and end the test right here?

chaodaiG · 2021-01-13T18:39:01Z

/test pull-test-infra-integration

chaodaiG · 2021-01-13T19:55:58Z

/test pull-test-infra-integration

chaodaiG · 2021-01-13T20:18:40Z

/test pull-test-infra-integration

cjwagner

Thanks for working on this Chao, very exciting!

cjwagner · 2021-01-13T23:59:12Z

prow/test/integration/test.sh

+  chmod +x /usr/bin/kind
+
+  # TODO(chaodaiG): remove this once bazel is installed in test image
+  echo "Install bazel for prow"


which bazel would be more robust by avoiding assuming the installation path. If you want to ensure a specific bazel version check with bazel --version. This installation doesn't create a binary called bazel or add anything to the path so I wouldn't expect the bazel command below to work.

cjwagner · 2021-01-14T00:03:36Z

hack/bazel.sh

@@ -20,7 +20,7 @@ set -o errexit
 set -o pipefail

 code=0
-(set -o xtrace && bazel "$@") || code=$?
+(set -o xtrace && bazel "$@" --test_tag_filters=-e2e) || code=$?


Was this change included accidentally? This script isn't used and this would probably be a breaking change for existing uses of the script.

Oh it looks like you're trying to prevent the integration tests from running unless specifically requested. Can we achieve that a better way? This prevents hack/bazel.sh from being used with the --test_tag_filters flag since it will already be specified. Also this assumes that bazel is always invoked via this script which is not the case.

Makes sense. I can use an env var or some sort to skip integration test if it's not specified, what do you think?

That could work. A flag would be a bit more explicit. It wouldn't be ideal for the test to noop and produce successful junit results when skipped, but IIRC Go's testing package provides a way to explicitly mark tests as skipped.

cjwagner · 2021-01-14T00:07:27Z

prow/test/integration/prow/cluster/100_namespace.yaml

@@ -0,0 +1,99 @@
+apiVersion: v1


nit: Please rename this file, it has more than just the namespace.

cjwagner · 2021-01-14T01:11:53Z

prow/test/integration/setup-prow.sh

+
+set -o errexit
+
+CURRENT_REPO="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd -P )"


Nit: This variable name is misleading, this is not the repo root, but rather the bash source dir (prow/test/integration).

cjwagner · 2021-01-14T01:22:32Z

prow/test/integration/test/setup.go

+}
+
+func NewClientsFromConfig(cfg *rest.Config) (*kubernetes.Clientset, error) {
+	kubeClient, err := kubernetes.NewForConfig(cfg)


nit: We could just use kubernetes.NewForConfig(cfg) directly. This function isn't really needed.

This function is obsolete as well, deleted

cjwagner · 2021-01-14T01:59:46Z

prow/test/integration/test/sinker_test.go

+				if err := kubeClient.Create(ctx, &prowjob); err != nil {
+					t.Fatalf("Failed creating prowjob: %v", err)
+				}
+				t.Logf("Finished creating CRD: %s", tt.name)


nit: In a couple places we say CRD rather than CR or PJ.

cjwagner · 2021-01-14T02:08:41Z

prow/test/integration/test/sinker_test.go

+			"orphaned-pod",
+			false,
+			true,
+		},


We'll also want to test some more scenarios like the following:

completed, non-orphaned pods are deleted after the terminatedPodTTL expires.

pods not created by prow are not deleted.

prowjobs (not pods) are deleted after maxProwJobAge has passed.

I figure this PR is more an initial prototype for integration testing though so we don't need to add these just yet if you'd rather focus on just validating this integration testing pattern.

Agreed that the above scenarios all need to be tested, not added in this PR as this is more for validating the pattern as you mentioned.

cjwagner · 2021-01-14T02:16:53Z

prow/test/integration/test/sinker_test.go

+			}
+			t.Logf("Finished creating pod: %s", tt.name)
+
+			// Make sure pod is running


This races with sinker deleting the pod. To safely test the orphaned pod case, I'd expect a PJ to be created before the pod is created, wait for the pod to start, then delete the PJ to orphan the pod. That should prevent sinker from seeing an orphaned pod until after we've confirmed the pod was successfully created.

cjwagner · 2021-01-14T02:19:01Z

prow/test/integration/setup-prow.sh

+echo "Push test image to registry"
+docker pull busybox
+docker tag busybox:latest localhost:5000/busybox:latest
+docker push localhost:5000/busybox:latest


We already publish an alpine image with Prow. I think that is suitable for this. gcr.io/k8s-prow/alpine

cjwagner · 2021-01-14T02:23:23Z

prow/test/integration/prow/config.yaml

+
+prowjob_namespace: default
+pod_namespace: test-pods
+log_level: debug


It would be handy to allow dumping the Prow component logs into an output dir ($ARTIFACTS in CI) so that we can more easily debug integration test failures.

I assume this needs to be done manually, something like k get logs svc/sinker -f > $ARTIFACTS/prowlogs/sinker &, what do you think?

It can also be done with client-go, but it might be easier with kubectl. We don't need to stream it though, we can just as some kind of a post step dump the log of all pods in a file that has the pod name or sth like that

Yes I'd expect logs to be dumped with kubectl at the end if $ARTIFACTS is populated (or better yet use a CLI arg/flag).

And have verified that dumped logs are under artifacts: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/test-infra/20451/pull-test-infra-integration/1349801426494164992

chaodaiG · 2021-01-14T17:20:48Z

/test pull-test-infra-integration

alvaroaleman · 2021-01-14T18:20:40Z

prow/test/integration/prow/config.yaml

+
+prowjob_namespace: default
+pod_namespace: test-pods
+log_level: debug


It can also be done with client-go, but it might be easier with kubectl. We don't need to stream it though, we can just as some kind of a post step dump the log of all pods in a file that has the pod name or sth like that

alvaroaleman · 2021-01-14T18:22:28Z

prow/test/integration/test/setup.go

+	return *clusterContext
+}
+
+func getDefaultKubeconfig(cfg string) string {


all of this is something the clientcfg.ConfigLoader already does with its default ruleset

Good to learn, done

chaodaiG · 2021-01-14T19:31:51Z

/test pull-test-infra-integration

chaodaiG · 2021-01-14T21:13:13Z

/test pull-test-infra-integration

chaodaiG · 2021-01-14T21:18:15Z

@cjwagner , instead of using build tag for integration test, a test flag --run-integration-test was added for the test suite, if not provided the tests won't run.

chaodaiG · 2021-01-14T21:31:47Z

/test pull-test-infra-integration

chaodaiG · 2021-01-15T16:54:51Z

@petr-muller , @alvaroaleman , @cjwagner , I believe I have addressed all comments, could you take another look?

alvaroaleman

/hold

It would be nice to enable reporting for the new presubmit though, so that it appears below PRs where ppl explicitly triggered it

The integration test introduced in kubernetes#20451 works as expected, make it reporting to github before make it required for presubmit

chaodaiG · 2021-01-15T17:54:24Z

/test pull-test-infra-integration

cjwagner

/hold cancel

cjwagner · 2021-01-15T18:46:33Z

prow/test/integration/test/setup.go

+	overrides := clientcmd.ConfigOverrides{}
+	// Override the cluster name if provided.
+	if clusterName != "" {
+		overrides.Context.Cluster = clusterName


I'm pretty sure overwriting the Context.Cluster would be problematic if the values actually differed since the cluster needs to be associated with the correct user (AuthInfo). That being said I don't know if the values will ever differ in practice so this might be fine anyways.

k8s-ci-robot · 2021-01-15T18:47:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, chaodaiG, cjwagner

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:

~~prow/OWNERS~~ [alvaroaleman,chaodaiG,cjwagner]

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

chaodaiG added 4 commits January 12, 2021 07:31

Write sinker test in go

4917a58

Changes for prow

f9cf4c5

Add some debugging

4812f55

Try bazel tags for no remote exec

d7e7229

k8s-ci-robot assigned alvaroaleman, cjwagner and fejta Jan 12, 2021

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/prow Issues or PRs related to prow sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jan 12, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2021

k8s-ci-robot requested review from petr-muller and spiffxp January 12, 2021 16:08

chaodaiG mentioned this pull request Jan 12, 2021

Write sinker test in go #20262

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2021

k8s-ci-robot assigned matthyx Jan 12, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2021

petr-muller reviewed Jan 13, 2021

View reviewed changes

alvaroaleman reviewed Jan 13, 2021

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2021

chaodaiG force-pushed the first-integration-test-sinker branch from edd9f28 to 713a6c5 Compare January 13, 2021 18:36

chaodaiG force-pushed the first-integration-test-sinker branch from 713a6c5 to 29d2c2d Compare January 13, 2021 19:55

Update based on code review feedback

043da32

chaodaiG force-pushed the first-integration-test-sinker branch from 29d2c2d to 043da32 Compare January 13, 2021 20:18

cjwagner reviewed Jan 14, 2021

View reviewed changes

Code review updates kubernetes#2

322da3b

alvaroaleman reviewed Jan 14, 2021

View reviewed changes

Code review comments kubernetes#3

53be2ca

Code review comments kubernetes#4

f8003df

chaodaiG force-pushed the first-integration-test-sinker branch from 576433a to f8003df Compare January 14, 2021 21:31

alvaroaleman approved these changes Jan 15, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 15, 2021

chaodaiG added a commit to chaodaiG/test-infra that referenced this pull request Jan 15, 2021

Prow integration test job report to github

10c3c6c

The integration test introduced in kubernetes#20451 works as expected, make it reporting to github before make it required for presubmit

chaodaiG mentioned this pull request Jan 15, 2021

Prow integration test job report to github #20498

Merged

cjwagner approved these changes Jan 15, 2021

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2021

k8s-ci-robot merged commit 603a3a0 into kubernetes:master Jan 15, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 15, 2021

This was referenced Jan 15, 2021

Make prow integration test as required presubmit test #20501

Merged

Prow integration test: add more scenarios for sinker test #20520

Merged

chaodaiG deleted the first-integration-test-sinker branch January 21, 2021 15:34


		CURRENT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd -P )"

		if [[ -n "${CI:-}" ]]; then


		set -o errexit

		CURRENT_REPO="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd -P )"

First prow integration test: sinker #20451

First prow integration test: sinker #20451

Conversation

chaodaiG commented Jan 12, 2021

chaodaiG commented Jan 12, 2021

chaodaiG commented Jan 12, 2021

chaodaiG commented Jan 12, 2021

matthyx commented Jan 12, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman 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

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

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

chaodaiG commented Jan 13, 2021

chaodaiG commented Jan 13, 2021

chaodaiG commented Jan 13, 2021

cjwagner 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

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

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

chaodaiG commented Jan 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaodaiG commented Jan 14, 2021

chaodaiG commented Jan 14, 2021