Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] 🌱 cleanup/e2e-fixtures #3310

Conversation

perdasilva
Copy link
Collaborator

@perdasilva perdasilva commented Jun 7, 2024

Description of the change:
Our e2e tests cover the case of serving a catalog whose content was extracted (i.e. not using the bundled opm + being able to ship just content). The problem is that we were using the official operatorhub io image. This image is not versioned (it only has the latest tag). Because this registry's cache is built with a newer version of opm, it is breaking tests in older versions of the product.

In this PR:

  • Refactors the never used communityCatalogImage (or something like that) into a opmVersion and refactors the tests to reflect the changes
  • Refactors the e2e job to detect changes to either the operator-registry dependency version of changes to the fixture files and rebuild the fixtures and kind load them for the e2e tests
  • OLM is tested with the newly built fixture images
  • If on master and e2e succeeds, the new fixture images are published by a new "fixture" GHA
  • Updates tests to use the opm version compatible fixture
  • I also had to update the catalog operator to treat test images (images coming from the quay.io/olmtest) as a digest image (basically PullIfNotPresent) - this is needed to avoid having to push temporary images but rather just kind load them instead

The new e2e flow is:

build:

  • rebuild fixture images if necessary
  • build controller image
  • upload artifacts

e2e:

  • download artifacts
  • load images
  • deploy test clusters with images (kind load docker-image)
  • run e2e

publish-e2e-fixtures:

  • only executes if on master and there have been changes
  • pushes new fixtures up to the olmtest registry

Copy link
Member

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to understand this, so posted a bunch of questions.

Makefile Show resolved Hide resolved
scripts/build_test_images.sh Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
m1kola
m1kola previously approved these changes Jun 10, 2024
Copy link
Member

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2024
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2024
Copy link

openshift-ci bot commented Jun 10, 2024

New changes are detected. LGTM label has been removed.

@perdasilva perdasilva force-pushed the perdasilva/fix/catalog-image branch from 04e2e3d to 741dbc9 Compare June 10, 2024 11:39
@perdasilva perdasilva changed the title 🌱 Patch e2es to use a test-catalog instead of operatorhubio [WIP] 🌱 cleanup/e2e-fixtures Jun 10, 2024
@perdasilva perdasilva force-pushed the perdasilva/fix/catalog-image branch 7 times, most recently from d95676f to 3dd1ff6 Compare June 10, 2024 13:45
@perdasilva perdasilva force-pushed the perdasilva/fix/catalog-image branch 3 times, most recently from c226fcd to 3cbb56a Compare June 10, 2024 14:16
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 10, 2024
@perdasilva perdasilva force-pushed the perdasilva/fix/catalog-image branch 6 times, most recently from b9112b8 to ca39bec Compare June 11, 2024 07:10
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2024
Signed-off-by: Per Goncalves da Silva <[email protected]>
@perdasilva perdasilva force-pushed the perdasilva/fix/catalog-image branch from e21f2d7 to 09fd438 Compare June 11, 2024 08:28
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2024
@perdasilva perdasilva force-pushed the perdasilva/fix/catalog-image branch from 09fd438 to a46e149 Compare June 11, 2024 08:52
@perdasilva perdasilva changed the title [WIP] 🌱 cleanup/e2e-fixtures 🌱 cleanup/e2e-fixtures Jun 11, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 11, 2024
@perdasilva perdasilva force-pushed the perdasilva/fix/catalog-image branch from a46e149 to a346cb9 Compare June 11, 2024 09:24
Per Goncalves da Silva added 2 commits June 11, 2024 11:26
Signed-off-by: Per Goncalves da Silva <[email protected]>
@perdasilva perdasilva force-pushed the perdasilva/fix/catalog-image branch from a346cb9 to d6b61ca Compare June 11, 2024 09:26
Copy link
Member

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall all looks good to me, but this made me wondering - can we do something so we are not dependent on quay.io for tests? Maybe something like we do with OLMv1 where we have a local registry? Just to clarify: not asking to do this as part of this PR.

Comment on lines 17 to 22
// Ensure test registry images are pulled only if needed
// These will normally be loaded through kind load docker-image
// This is also helps support the e2e fixture image re-build flow (see e2e GHA)
if strings.HasPrefix(image, "quay.io/olmtest/") {
return corev1.PullIfNotPresent
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of having a special cases for tests. Is there a way to avoid this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same - the only other way I could think of is by threading another command line arg down the stack - idwtdt

I figured this isn't so bad, since the pattern should only ever match our registry. So, there shouldn't be any hidden surprises.

But, yeah, I don't like it either. But anything else that I could think of felt like too many changes T_T

🤔 let me see if I can programmatically grab the sha from the image:tag - I think we already have a dependency on the docker client anyway

@perdasilva
Copy link
Collaborator Author

Overall all looks good to me, but this made me wondering - can we do something so we are not dependent on quay.io for tests? Maybe something like we do with OLMv1 where we have a local registry? Just to clarify: not asking to do this as part of this PR.

Yeah, that'd be heaps better! The current approach relies on kind loading. Maybe the right way is to just as v1. Get rid of quay.io/olmtest and just rebuild against a local registry for the tests. I've created an issue to track this: #3315

@perdasilva
Copy link
Collaborator Author

No luck. Doesn't seem to find the image on the node by its sha - so kind load didn't work. Maybe we'll have to look at a local registry...=S

@perdasilva
Copy link
Collaborator Author

/hold exploring registry option

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2024
Signed-off-by: Per Goncalves da Silva <[email protected]>
@perdasilva perdasilva force-pushed the perdasilva/fix/catalog-image branch from d6b61ca to 442c42d Compare June 14, 2024 14:23
@perdasilva perdasilva changed the title 🌱 cleanup/e2e-fixtures [WIP] 🌱 cleanup/e2e-fixtures Jun 14, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 14, 2024
@perdasilva perdasilva force-pushed the perdasilva/fix/catalog-image branch 4 times, most recently from 94e5712 to 279ff33 Compare June 17, 2024 12:27
Signed-off-by: Per Goncalves da Silva <[email protected]>
@perdasilva perdasilva force-pushed the perdasilva/fix/catalog-image branch 2 times, most recently from 93d971b to 195720d Compare June 17, 2024 17:03
Signed-off-by: Per Goncalves da Silva <[email protected]>
@perdasilva perdasilva force-pushed the perdasilva/fix/catalog-image branch from 195720d to 17d51c3 Compare June 17, 2024 17:39
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2024
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

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-sigs/prow repository.

@perdasilva perdasilva closed this Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants