Add PullIfNotPresent to multiple container test

[nak3]

Proposed Changes

When e2e test creates ksvc, it explicitly sets corev1.PullIfNotPresent in ksvc(ConfigurationSpec).

serving/test/v1/configuration.go

Lines 122 to 133 in 45b119b

	if !test.ServingFlags.DisableOptionalAPI {
	// Container.imagePullPolicy is not required by Knative
	// Serving API Specification.
	//
	// Kubernetes default pull policy is IfNotPresent unless
	// the :latest tag (== no tag) is used, in which case it
	// is Always. To support e2e testing on KinD, we want to
	// explicitly disable image pulls when present because we
	// side-load the test images onto all nodes and never push
	// them to a registry.
	c.Template.Spec.PodSpec.Containers[0].ImagePullPolicy = corev1.PullIfNotPresent
	}

However, multiple containers test overrides the container spec and missed to set the corev1.PullIfNotPresent.
Due to that, e2e test on KinD or minikube, which uses local image fails due to ImagePullBackOff.

Release Note

NONE

[nak3]

/hold
/cc @KaranJagtiani

@KaranJagtiani reported an issue (thank you!) about his e2e test and I suspect that it was caused by this.

[knative-prow]

@nak3: GitHub didn't allow me to request PR reviews from the following users: KaranJagtiani.

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

In response to this:

/hold
/cc @KaranJagtiani

@KaranJagtiani reported an issue (thank you!) about his e2e test and I suspect that it was caused by this.

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.

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: +0.02% 🎉

Comparison is base (442c13d) 86.04% compared to head (66ecfa4) 86.06%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14317      +/-   ##
==========================================
+ Coverage   86.04%   86.06%   +0.02%     
==========================================
  Files         196      196              
  Lines       14781    14781              
==========================================
+ Hits        12718    12721       +3     
+ Misses       1754     1753       -1     
+ Partials      309      307       -2     

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[KaranJagtiani]

@nak3 I can confirm that this fixes the issue that I was facing. Thank you for the quick resolution!

Issue

While running e2e tests that created container(s) using the K8s API (example) on my local setup (minikube with kourier), minikube was not able to set the container image from the local docker registry even though the image was already present.

[nak3]

/hold cancel

Thank you, Karan!

[dprotaso]

@KaranJagtiani curious if the mini-kube registry addon works?

https://minikube.sigs.k8s.io/docs/handbook/registry/#docker-on-macos

If so I'm inclined to drop this workaround in our tests and require a registry since kind has instructions how to set that up now - https://kind.sigs.k8s.io/docs/user/local-registry/

[ReToCode]

@dprotaso Hm might work for minikube, but the kind thing seems like a bit more work for folks to make tests pass locally (as long as it is not a built-in thing, like the say in the banner). I usually just use the kind.local registry with ko and also experienced that error.

[KaranJagtiani]

@dprotaso I just tested the minikube registry addon and it works like a charm - it totally fixes the issue!
Haven't tried the kind setup yet for local development, but based on the setup process provided in the link you shared, I would agree with @ReToCode that it seems like a little more work to run certain tests.

Just a thought - we can maybe provide bash scripts for enabling minikube and kind registries for folks who want to run e2e tests on local, and then document that?

[dprotaso]

/lgtm
/approve

We should probably revisit this when kind adds a registry add on

Just a thought - we can maybe provide bash scripts for enabling minikube and kind registries for folks who want to run e2e tests on local, and then document that?

@KaranJagtiani yeah I've been thinking about this. It's hard to figure out what to do to make things maintainable across serving's N repos and reduce duplicate work with maintaining our CI scripts.

eg. we use this action for CI - https://github.com/chainguard-dev/actions/tree/main/setup-kind - creating an equivalent bash script would be easy - but then i don't think it makes sense to copy and paste it across net-istio/net-contour etc.

There's been discussions in the past about using something like https://magefile.org/ and some projects already use makefiles. But neither these tools are great for running tasks in my mind (maybe mage is ok). There's a lot of gotcha's with running shell commands in a makefile

i guess tl;dr - I'm suffering from analysis paralysis - i sorta hope our productivity working group would tackle this problem with buy in from all the WG group leads

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, nak3

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/OWNERS [dprotaso,nak3]

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

[KaranJagtiani]

@dprotaso, totally get the analysis paralysis—maintaining something scalable and easy to manage across all of serving's N repos is a real challenge. Your point about the potential issues with duplicating bash scripts is very valid.

Hadn't heard of Mage before, seems interesting - seems like it can easily integrate with the codebase as well if needed.

I like the idea of getting a standardized solution from the productivity working group, as I can understand such problems can use a org-wide common solution. For the short term, do you think it'd be worth it to document this particular issue and the solution for it somewhere?

Looking forward to what the productivity working group comes up with!

[dprotaso]

Looks like (v1.25.x, kourier-tls, e2e) is broken for some reason

[dprotaso]

@KaranJagtiani feel free to update https://github.com/knative/serving/blob/main/DEVELOPMENT.md#create-a-cluster-and-a-repo

[nak3]

Looks like (v1.25.x, kourier-tls, e2e) is broken for some reason

It was fixed and passed all tests.

[ReToCode]

/lgtm