Inconsistent unit test result

[wilsonehusin]

What steps did you take and what happened:
Unit tests under pkg/image deliver inconsistent result

What did you expect to happen:
Unit tests should be consistent

Anything else you would like to add:
My initial reaction is probably improper mock or race conditions. This occurred on CircleCI on 2b8d1eabc96070954a3fad5c482973fffefd2506 as part of #1179.

Failed on first run, but without anything being changed, it passed on second run.

[vladimirvivien]

@wilsonehusin yes, the CI is flaky. The workaround is to kick off the entire job after a failure. We will need to address this issue and possibly re-write the CI jobs.

[wilsonehusin]

Investigation

• seems like it was surfaced here (line 178)

  sonobuoy/pkg/image/manifest_test.go

  Lines 170 to 179 in 7419e1d

  	// Check one of the returned image pairs to ensure correct format
  	imageTagPair := imageTagPairs[0]
  	if strings.HasPrefix(imageTagPair.Src, customRegistry) {
  	t.Errorf("Src image should not have custom registry prefix: %q", imageTagPair.Src)
  	}
  	imageComponents := strings.SplitAfter(imageTagPair.Src, "/")
  	if !strings.HasPrefix(imageTagPair.Dst, customRegistry) {
  	t.Errorf("Expected Dst image to have prefix %q, got %q", customRegistry, imageTagPair.Dst)
  	}

• inspecting the value of imageTagPairs, turns out not all registries were overwritten (I've sorted the output here to show the first section where the paths were not replaced vs the second section where they were properly replaced)

  example output

  {docker.io/gluster/glusterdynamic-provisioner:v1.0 docker.io/gluster/glusterdynamic-provisioner:v1.0}
  {gcr.io/authenticated-image-pulling/alpine:3.7 gcr.io/authenticated-image-pulling/alpine:3.7}
  {gcr.io/k8s-authenticated-test/agnhost:2.6 gcr.io/k8s-authenticated-test/agnhost:2.6}
  {gcr.io/google-containers/startup-script:v1 gcr.io/google-containers/startup-script:v1}
  {invalid.com/invalid/alpine:3.1 invalid.com/invalid/alpine:3.1}
  {gcr.io/authenticated-image-pulling/windows-nanoserver:v1 gcr.io/authenticated-image-pulling/windows-nanoserver:v1}
  {quay.io/kubernetes_incubator/nfs-provisioner:v2.2.2 quay.io/kubernetes_incubator/nfs-provisioner:v2.2.2}
  

  {gcr.io/kubernetes-e2e-test-images/apparmor-loader:1.0 my-custom/registry/apparmor-loader:1.0}
  {gcr.io/kubernetes-e2e-test-images/cuda-vector-add:1.0 my-custom/registry/cuda-vector-add:1.0}
  {k8s.gcr.io/pause:3.1 my-custom/registry/pause:3.1}
  {gcr.io/kubernetes-e2e-test-images/test-webserver:1.0 my-custom/registry/test-webserver:1.0}
  {k8s.gcr.io/etcd:3.4.3 my-custom/registry/etcd:3.4.3}
  {docker.io/library/httpd:2.4.38-alpine my-custom/registry/httpd:2.4.38-alpine}
  {docker.io/library/httpd:2.4.39-alpine my-custom/registry/httpd:2.4.39-alpine}
  {gcr.io/kubernetes-e2e-test-images/nonroot:1.0 my-custom/registry/nonroot:1.0}
  {gcr.io/kubernetes-e2e-test-images/resource-consumer-controller:1.0 my-custom/registry/resource-consumer-controller:1.0}
  {gcr.io/kubernetes-e2e-test-images/metadata-concealment:1.2 my-custom/registry/metadata-concealment:1.2}
  {gcr.io/kubernetes-e2e-test-images/nonewprivs:1.0 my-custom/registry/nonewprivs:1.0}
  {k8s.gcr.io/prometheus-dummy-exporter:v0.1.0 my-custom/registry/prometheus-dummy-exporter:v0.1.0}
  {gcr.io/kubernetes-e2e-test-images/volume/nfs:1.0 my-custom/registry/volume/nfs:1.0}
  {gcr.io/kubernetes-e2e-test-images/cuda-vector-add:2.0 my-custom/registry/cuda-vector-add:2.0}
  {gcr.io/kubernetes-e2e-test-images/dnsutils:1.1 my-custom/registry/dnsutils:1.1}
  {gcr.io/kubernetes-e2e-test-images/ipc-utils:1.0 my-custom/registry/ipc-utils:1.0}
  {docker.io/library/nginx:1.14-alpine my-custom/registry/nginx:1.14-alpine}
  {k8s.gcr.io/sd-dummy-exporter:v0.2.0 my-custom/registry/sd-dummy-exporter:v0.2.0}
  {gcr.io/kubernetes-e2e-test-images/volume/iscsi:2.0 my-custom/registry/volume/iscsi:2.0}
  {gcr.io/kubernetes-e2e-test-images/sample-apiserver:1.10 my-custom/registry/sample-apiserver:1.10}
  {gcr.io/kubernetes-e2e-test-images/mounttest:1.0 my-custom/registry/mounttest:1.0}
  {docker.io/library/perl:5.26 my-custom/registry/perl:5.26}
  {docker.io/library/redis:5.0.5-alpine my-custom/registry/redis:5.0.5-alpine}
  {gcr.io/kubernetes-e2e-test-images/regression-issue-74839-amd64:1.0 my-custom/registry/regression-issue-74839-amd64:1.0}
  {gcr.io/kubernetes-e2e-test-images/resource-consumer:1.5 my-custom/registry/resource-consumer:1.5}
  {gcr.io/kubernetes-e2e-test-images/volume/gluster:1.0 my-custom/registry/volume/gluster:1.0}
  {gcr.io/kubernetes-e2e-test-images/agnhost:2.8 my-custom/registry/agnhost:2.8}
  {docker.io/library/busybox:1.29 my-custom/registry/busybox:1.29}
  {gcr.io/kubernetes-e2e-test-images/echoserver:2.2 my-custom/registry/echoserver:2.2}
  {gcr.io/kubernetes-e2e-test-images/jessie-dnsutils:1.0 my-custom/registry/jessie-dnsutils:1.0}
  {gcr.io/kubernetes-e2e-test-images/nautilus:1.0 my-custom/registry/nautilus:1.0}
  {docker.io/library/nginx:1.15-alpine my-custom/registry/nginx:1.15-alpine}
  {gcr.io/kubernetes-e2e-test-images/kitten:1.0 my-custom/registry/kitten:1.0}
  {gcr.io/kubernetes-e2e-test-images/mounttest-user:1.0 my-custom/registry/mounttest-user:1.0}
  {k8s.gcr.io/prometheus-to-sd:v0.5.0 my-custom/registry/prometheus-to-sd:v0.5.0}
  {gcr.io/kubernetes-e2e-test-images/volume/rbd:1.0.1 my-custom/registry/volume/rbd:1.0.1}
  

• seems like this might be origin of drift, where only some of the registries were modified (lines 127-130)

  sonobuoy/pkg/image/manifest_test.go

  Lines 121 to 146 in 7419e1d

  	func createTestRegistryConfig(customRegistry, version string) (string, error) {
  	registries, err := GetDefaultImageRegistries(version)
  	if err != nil {
  	return "", err
  	}
  	registries.E2eRegistry = customRegistry
  	registries.DockerLibraryRegistry = customRegistry
  	registries.GcRegistry = customRegistry
  	registries.SampleRegistry = customRegistry
  	tmpfile, err := ioutil.TempFile("", "config.*.yaml")
  	if err != nil {
  	return "", err
  	}
  	defer tmpfile.Close()
  	d, err := yaml.Marshal(&registries)
  	if err != nil {
  	return "", err
  	}
  	if _, err := tmpfile.Write(d); err != nil {
  	return "", err
  	}
  	return tmpfile.Name(), nil
  	}

  
  fortunately (unfortunately?) the test above writes to a config file in /tmp (Mac might have a different directory due to TempFile implementation, I'm on Linux) which was not deleted at the end of test run. here's an example value which I found on my computer:

  buildImageRegistry: ""
  dockerGluster: docker.io/gluster
  dockerLibraryRegistry: my-custom/registry
  e2eRegistry: my-custom/registry
  e2eVolumeRegistry: ""
  gcRegistry: my-custom/registry
  googleContainerRegistry: gcr.io/google-containers
  promoterE2eRegistry: ""
  quayIncubator: quay.io/kubernetes_incubator
  sampleRegistry: my-custom/registry
  sigStorageRegistry: ""

• while I'm tempted to straight up modify the code above to add the rest of registries, I've found the following lines as well:
  

  sonobuoy/pkg/image/manifest.go

  Lines 302 to 316 in 7419e1d

  	return &RegistryList{
  	E2eRegistry: e2eRegistry,
  	DockerLibraryRegistry: dockerLibraryRegistry,
  	GcRegistry: gcRegistry,
  	GoogleContainerRegistry: googleContainerRegistry,
  	DockerGluster: dockerGluster,
  	QuayIncubator: quayIncubator,
  	// The following keys are used in the v1.17 registry list however their images
  	// cannot be pulled as they are used as part of tests for checking image pull
  	// behavior. They are omitted from the resulting config.
  	// InvalidRegistry: invalidRegistry,
  	// GcAuthenticatedRegistry: gcAuthenticatedRegistry,
  	// PrivateRegistry: privateRegistry,
  	}, nil

Proposal solution

I think it is correct for invalidRegistry, gcAuthenticatedRegistry, and privateRegistry to remain as is, for they are used to validate image pulling behavior. Therefore, I think they should remain untouched.

With that being said, imageTagPairs in our test will always have some of its members to not be prefixed with my-custom/registry.

I think we should only assert the values where we specifically modify the registries, i.e. choosing one/any/all of e2eRegistry, dockerLibraryRegistry, or gcRegistry deterministically.

[wilsonehusin]

@vladimirvivien do you have thoughts on the solution?

[vladimirvivien]

@wilsonehusin you know how I feel about this part of the code. For now, any fix that would make things better is welcome. By the way thanks for looking into this with such insight.