feat: remove condition that checks if an image was built from Skaffold

[renzodavid9]

Fixes: #8768

Description
This PR is to improve the skaffold verify command with K3D and Kind clusters:

• Now the images built by Skaffold will be loaded to the cluster
• Now, when the cluster doesn't find the image for the pod it will fail, before it was stuck

[codecov]

Codecov Report

Merging #8935 (9353c54) into main (290280e) will decrease coverage by 6.86%.
The diff coverage is 48.91%.

@@            Coverage Diff             @@
##             main    #8935      +/-   ##
==========================================
- Coverage   70.48%   63.62%   -6.86%     
==========================================
  Files         515      624     +109     
  Lines       23150    31923    +8773     
==========================================
+ Hits        16317    20311    +3994     
- Misses       5776    10085    +4309     
- Partials     1057     1527     +470     

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/completion.go	13.04% <0.00%> (-1.25%)	⬇️
cmd/skaffold/app/cmd/config/list.go	65.21% <ø> (ø)
cmd/skaffold/app/cmd/config/set.go	88.72% <ø> (ø)
cmd/skaffold/app/cmd/config/util.go	54.28% <ø> (ø)
cmd/skaffold/app/cmd/credits.go	100.00% <ø> (ø)
cmd/skaffold/app/cmd/credits/export.go	0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go	40.90% <0.00%> (-12.94%)	⬇️
cmd/skaffold/app/cmd/generate_pipeline.go	60.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_modules.go	65.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_profiles.go	66.66% <ø> (ø)
... and 40 more

... and 416 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ericzzzzzzz]

While testing the fix, I realize that we have something really wrong regarding how we use
loader.loadImage method

skaffold/pkg/skaffold/kubernetes/loader/load.go

Line 88 in 88e1c17

func (i *ImageLoader) LoadImages(ctx context.Context, out io.Writer, localImages, deployerImages, images []graph.Artifact) error {

let's take this specific implementation for example, LoadImages(ctx context.Context, out io.Writer, localImages, deployerImages, images []graph.Artifact) , The deployimages seem to suggest we pass images that the deployer going to deploy to this method, and we did this in v1 by passing d.originalImages, that images were parsed out from deployer' manifests. Here is a kubectl deployer example:

skaffold/pkg/skaffold/deploy/kubectl/kubectl.go

Lines 443 to 452 in 652a425

	originalManifests := append(localManifests, remoteManifests...)
	if len(k.originalImages) == 0 {
	// TODO(aaron-prindle) maybe use different resoureselector?
	k.originalImages, err = originalManifests.GetImages(manifest.NewResourceSelectorImages(k.transformableAllowlist, k.transformableDenylist))
	// k.originalImages, err = originalManifests.GetImages(k.transformableAllowlist, k.transformableDenylist)
	if err != nil {
	return nil, err
	}
	}

, and when we moved to v2, we missed setting original images part which causes skaffold dev fail to pull images when using kind or k3d cluster, as images were not loaded to the cluster. The attempt to fix 'images-not-loading' issue #8007 created some problems and led to some chain-effect for later works. The attempt set originalImages with artifacts from all pipeline

skaffold/pkg/skaffold/deploy/kubectl/kubectl.go

Lines 116 to 122 in a3ee2cc

	var ogImages []graph.Artifact
	for _, artifact := range artifacts {
	ogImages = append(ogImages, graph.Artifact{
	ImageName: artifact.ImageName,
	RuntimeType: artifact.RuntimeType,
	})
	}

, this does fix the original issue, but downsides are:

• for multi-modules project with multiple local clusters, we will load all build artifacts to different clusters even though some cluster may not need it, this doesn't actually matter too much considering the chance people will have multiple local kind or k3d clusters .
• this is technically wrong thing to do and may lead more problems in the future, the originalImages for verifier should not gain from artifacts under build stanza from all pipeline, instead images defined under verify stanza may be a good candidate for the source of verifier's original images, using artifacts from Build Stanza could lead to image pull error due to images not load, even if images are actually available in local and user pass build-output that contains the images required by verify stanza if they run a skaffold profile for verify without that build stanza. Take a step further, do we have to use the current load Image method for loading images for verify, this method also has logic around other stanzas like build, deploy for local images, do we even need to know which images are local images for verify? Should we just assume that the intersection between images provided through build-output and images defined under verify stanza are those need to load to the cluster?

[ericzzzzzzz]

let's focus on what is the best for verify command, and create an issue to track how to set deployer's original images, that may need some work for helm deployer.

[renzodavid9]

Hey @ericzzzzzzz, thanks for going deeper wit the load images logic. From our offline conversation:

• I created a new issue Improve/fix load Image logic in deployers, related with Kind and K3D #8946 to follow-up with the improvement for the load image logic, mentioning the points you added in the previous comment; feel free to add/modify any more details if need it
• Currently, for the verify command, the artifacts variable that is used to calculate the images to load in the clusters, is populated with the resulting values from the intersection between:
  • the images found in all the verify test cases (filtered by profile), and
  • the images received in the build-artifacts flag,
  • we don't take into account the values from the build stanza.
• As discussed, I added back the logic that calculates the images to load in the clusters, and instead now we are turning on the --load-images flag, which will allow users to force the loading of the images in the cluster

Please let me know if you have more comments :)

[ericzzzzzzz]

Hey @renzodavid9 ,

• the verify command uses imageLoader to load the image under the hood, the imageLoader take the intersection of localImage, originalImage, allBuilds
• The local image slice passed by k8s verifier is from https://github.com/GoogleContainerTools/skaffold/blob/73a798a280e46d9ddaddc26d9caf05c268759d33/pkg/skaffold/runner/verify.go#L92C1-L100
• The source of original images is from Build Stanza in skaffold yaml file.
• allBuilds is the one you mentioned, -- the intersection of imaged defined in verify stanza and build-output from command line
• You can test with your branch to see if verify command if taking build stanza into account when loading image by the following steps:
  • create a temp folder
  • create a Dockerfile with following content

    FROM ubuntu as starter

  • create skaffold yaml file

  apiVersion: skaffold/v4beta6
  kind: Config
  build:
   artifacts:
     - image: verify-starter
       context: .
  verify:
   - name: vvv
     container:
       name: vvvaaa
       image: verify-starter
       command: [ "/bin/sh" ]
       args: [ "-c", "echo 123; sleep 1; echo bye" ]
     executionMode:
       kubernetesCluster: {}

  • run skaffold build -q > build_result.json
  • delete build stanza from the skaffold.yaml file
  • run kind delete cluster && kind create cluster to make sure we use a brand new cluster to test.
  • run skaffold verify -a build_result.json --load-images, this will fail due to image doesn't exit on cluster
• I'm thinking we just change here

  skaffold/pkg/skaffold/verify/k8sjob/verify.go

  Line 138 in e50818a

  if err := v.imageLoader.LoadImages(childCtx, out, v.localImages, v.originalImages, allbuilds); err != nil {

  to v.imageLoader.LoadImages(childCtx, out, allbuilds, allbuilds, allbuilds);, this way we don't need to change the flag, also the intersection of image under verify and from cli will be loaded. WDYT?

[renzodavid9]

Oh, I see, good catch. I modified the call to imageLoader in

skaffold/pkg/skaffold/verify/k8sjob/verify.go

Line 138 in e50818a

if err := v.imageLoader.LoadImages(childCtx, out, v.localImages, v.originalImages, allbuilds); err != nil {

to v.imageLoader.LoadImages(childCtx, out, v.localImages, v.localImages, v.localImages); I'm using here v.localImages instead of allbuilds to be able to control if the images should be loaded into Kind/K3D using Skaffold's global configuration file, setting the k3d-disable-load and kind-disable-load properties. Let me know what do you think. Thanks!

[ericzzzzzzz]

LGTM