e2e: support CSI images in -list-images #108458

pohly · 2022-03-02T16:58:30Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

It was possible to patch images in the YAML files via KUBE_TEST_REPO, but it
was not possible to know in advance which images might be needed. Now
-list-images also includes all images referenced
by the test-manifest/storage-csi YAML files.

Special notes for your reviewer:

This is an alternative for #108261

Does this PR introduce a user-facing change?

When invoked with `-list-images`, the e2e.test binary now also lists the images that might be needed for storage tests.

/cc @jsafrane @derek-pryor

k8s-ci-robot · 2022-03-02T16:58:32Z

@pohly: GitHub didn't allow me to request PR reviews from the following users: derek-pryor.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?

/kind feature

What this PR does / why we need it:

It was possible to patch images in the YAML files via KUBE_TEST_REPO, but it
was not possible to know in advance which images might be needed. Now
-list-images also includes the k8s.gcr.io/sig-storage images referenced
by the test-manifest/storage-csi YAML files.

Special notes for your reviewer:

This is an alternative for #108261

Does this PR introduce a user-facing change?
The e2e.test binary now also lists k8s.gcr.io/sig-storage images that might be needed for tests when invoked with `-list-images`.
/cc @jsafrane @derek-pryor

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2022-03-02T16:58:38Z

@pohly: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pohly · 2022-03-02T19:22:31Z

I was focusing on just on "our" (= k8s.io/sig-storage) images initially, but then thought some more and decided that list all images that are referenced by our YAML files is the better and probably right solution, because the additional images (like socat and busybox) might also be used.

pohly · 2022-03-02T19:26:03Z

New images (from go test -v ./test/e2e -list-images):

> alpine/socat:1.0.3
5a7
> k8s.gcr.io/cloud-provider-gcp/gcp-compute-persistent-disk-csi-driver:v1.2.2
7a10
> k8s.gcr.io/e2e-test-images/busybox:1.29-1
40a44,56
> k8s.gcr.io/sig-storage/csi-attacher:v3.1.0
> k8s.gcr.io/sig-storage/csi-attacher:v3.3.0
> k8s.gcr.io/sig-storage/csi-external-health-monitor-controller:v0.4.0
> k8s.gcr.io/sig-storage/csi-node-driver-registrar:v2.1.0
> k8s.gcr.io/sig-storage/csi-node-driver-registrar:v2.3.0
> k8s.gcr.io/sig-storage/csi-provisioner:v2.1.0
> k8s.gcr.io/sig-storage/csi-provisioner:v3.0.0
> k8s.gcr.io/sig-storage/csi-resizer:v1.1.0
> k8s.gcr.io/sig-storage/csi-resizer:v1.3.0
> k8s.gcr.io/sig-storage/csi-snapshotter:v3.0.3
> k8s.gcr.io/sig-storage/csi-snapshotter:v4.2.1
> k8s.gcr.io/sig-storage/hostpathplugin:v1.7.3
> k8s.gcr.io/sig-storage/livenessprobe:v2.4.0

pohly · 2022-03-02T20:41:45Z

/retest

derek-pryor · 2022-03-03T17:09:29Z

Manually patched openshift-tests with this change and verified that openshift-tests images works as we wanted.
/lgtm

k8s-ci-robot · 2022-03-03T17:09:47Z

@derek-pryor: changing LGTM is restricted to collaborators

In response to this:

Manually patched openshift-tests with this change and verified that openshift-tests images works as we wanted.
/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pohly · 2022-03-03T19:06:04Z

/assign @jsafrane

jsafrane · 2022-03-03T17:20:12Z

test/utils/image/csi_manifest.go

+		// with many dependencies.
+		items := bytes.Split(data, []byte("\n---"))
+		for i, item := range items {
+			// We don't care what the acsictual type is. We just


typo: acsictual

Urgh. Looks like a cat stepped on my keyboard. I don't have a cat. Fixed.

jsafrane · 2022-03-04T10:13:10Z

test/utils/image/csi_manifest.go

+			// We don't care what the acsictual type is. We just
+			// unmarshal into generic maps and then look up images.
+			var object interface{}
+			if err := yaml.Unmarshal(item, &object); err != nil {


Would it be better to use unstructured parse? Random example:
https://github.com/openshift/azure-disk-csi-driver-operator/blob/master/vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/unstructured.go#L13

In addition, there are some helpers that can handle interface{}, would NestedSlice / NestedString help? It would require this code to know what is a slice in "spec", "template", "spec", "containers", "image" below, not sure if it's worth the effort.

I'm aware of unstructured.Unstructured. I decided against using it here because it would add another dependency (not a big deal, though) and because I would still need to walk the fields myself. At least that is my understanding of the current helpers: NestedString returns the first match, but not all of them.

It was possible to patch images in the YAML files via KUBE_TEST_REPO, but it was not possible to know in advance which images might be needed. Now -list-images also includes all images referenced by the test-manifest/storage-csi YAML files.

jsafrane · 2022-03-08T12:51:28Z

/lgtm

pohly · 2022-03-08T13:22:04Z

/assign @mkumatag

For approval.

mkumatag

/lgtm
/approve

/hold @dims if you have any comments!

mkumatag · 2022-03-10T11:32:18Z

test/utils/image/csi_manifest.go

+	if err != nil {
+		panic(err)
+	}


this can be worrisome but UT will make sure this won't happen(especially when yaml.Unmarshal goes wrong)

We control the input, so we know that this cannot go wrong.

dims · 2022-03-10T11:41:25Z

/lgtm
/approve

/hold cancel

k8s-ci-robot · 2022-03-10T11:41:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, mkumatag, pohly

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:

~~test/utils/image/OWNERS~~ [dims,mkumatag]

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

dims · 2022-03-10T11:43:37Z

@pohly could you write up this (how to list the images and how to specify a yaml to override) somewhere please? @mkumatag you are using this trick too? if so please collaborate with @pohly ? thanks!

mkumatag · 2022-03-10T11:49:51Z

@pohly could you write up this (how to list the images and how to specify a yaml to override) somewhere please? @mkumatag you are using this trick too? if so please collaborate with @pohly ? thanks!

I see some information is located here: https://github.com/kubernetes/kubernetes/tree/master/test/images#testing-images but not sure that's good enough!

pohly · 2022-03-10T13:35:56Z

I am not familiar with how people use this and it's not a concept that I came up with. In other words, someone else needs to write better documentation if the current one is not good enough.

dims · 2022-03-10T15:30:58Z

/retest

pohly · 2022-03-10T19:39:32Z

/retest

k8s-triage-robot · 2022-03-10T22:38:23Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-triage-robot · 2022-03-11T03:32:22Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 2, 2022

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 2, 2022

k8s-ci-robot requested a review from jsafrane March 2, 2022 16:58

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Mar 2, 2022

pohly mentioned this pull request Mar 2, 2022

Csi test image mirroring #108261

Closed

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 2, 2022

pohly force-pushed the csi-test-image-mirroring branch from 47d6086 to fafcc92 Compare March 2, 2022 19:19

pohly force-pushed the csi-test-image-mirroring branch from fafcc92 to 39ec153 Compare March 2, 2022 19:37

k8s-ci-robot assigned jsafrane Mar 3, 2022

jsafrane reviewed Mar 7, 2022

View reviewed changes

e2e: support CSI images in -list-images

df1d1cc

It was possible to patch images in the YAML files via KUBE_TEST_REPO, but it was not possible to know in advance which images might be needed. Now -list-images also includes all images referenced by the test-manifest/storage-csi YAML files.

pohly force-pushed the csi-test-image-mirroring branch from 39ec153 to df1d1cc Compare March 7, 2022 14:03

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2022

k8s-ci-robot assigned mkumatag Mar 8, 2022

mkumatag approved these changes Mar 10, 2022

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 10, 2022

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

k8s-ci-robot assigned dims Mar 10, 2022

k8s-ci-robot merged commit 897f2f7 into kubernetes:master Mar 11, 2022

k8s-ci-robot added this to the v1.24 milestone Mar 11, 2022

github-actions bot mentioned this pull request Mar 22, 2022

Week Ending March 13, 2022 dev-obs/actus#391

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e: support CSI images in -list-images #108458

e2e: support CSI images in -list-images #108458

pohly commented Mar 2, 2022 •

edited

k8s-ci-robot commented Mar 2, 2022

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Mar 2, 2022

pohly commented Mar 2, 2022

pohly commented Mar 2, 2022

pohly commented Mar 2, 2022

derek-pryor commented Mar 3, 2022

k8s-ci-robot commented Mar 3, 2022

pohly commented Mar 3, 2022

jsafrane Mar 3, 2022

pohly Mar 7, 2022

jsafrane Mar 4, 2022

pohly Mar 7, 2022

jsafrane commented Mar 8, 2022

pohly commented Mar 8, 2022

mkumatag left a comment

mkumatag Mar 10, 2022

pohly Mar 10, 2022

dims commented Mar 10, 2022

k8s-ci-robot commented Mar 10, 2022

dims commented Mar 10, 2022

mkumatag commented Mar 10, 2022

pohly commented Mar 10, 2022

dims commented Mar 10, 2022

pohly commented Mar 10, 2022

k8s-triage-robot commented Mar 10, 2022

k8s-triage-robot commented Mar 11, 2022

e2e: support CSI images in -list-images #108458

e2e: support CSI images in -list-images #108458

Conversation

pohly commented Mar 2, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Mar 2, 2022

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Mar 2, 2022

pohly commented Mar 2, 2022

pohly commented Mar 2, 2022

pohly commented Mar 2, 2022

derek-pryor commented Mar 3, 2022

k8s-ci-robot commented Mar 3, 2022

pohly commented Mar 3, 2022

jsafrane Mar 3, 2022

Choose a reason for hiding this comment

pohly Mar 7, 2022

Choose a reason for hiding this comment

jsafrane Mar 4, 2022

Choose a reason for hiding this comment

pohly Mar 7, 2022

Choose a reason for hiding this comment

jsafrane commented Mar 8, 2022

pohly commented Mar 8, 2022

mkumatag left a comment

Choose a reason for hiding this comment

mkumatag Mar 10, 2022

Choose a reason for hiding this comment

pohly Mar 10, 2022

Choose a reason for hiding this comment

dims commented Mar 10, 2022

k8s-ci-robot commented Mar 10, 2022

dims commented Mar 10, 2022

mkumatag commented Mar 10, 2022

pohly commented Mar 10, 2022

dims commented Mar 10, 2022

pohly commented Mar 10, 2022

k8s-triage-robot commented Mar 10, 2022

k8s-triage-robot commented Mar 11, 2022

pohly commented Mar 2, 2022 •

edited