Skip to content

Conversation

jlpedrosa
Copy link
Contributor

@jlpedrosa jlpedrosa commented Feb 19, 2021

Description of the change:
Upgrade kuttl to version 0.9.0

Motivation for the change:
New version contains the capability to run assertion based on commands/scripts, that enable more complete testing of the objects being managed

@estroz
Copy link
Member

estroz commented Feb 22, 2021

Unfortunately this kuttl version contains a breaking change (kudo.dev group removal, kudobuilder/kuttl#263) so upgrading isn't as easy as just changing scorecard-test-kuttl's base image version.

My suggestion is to start versioning the scorecard-test-kuttl image with kuttl version thrown into the matrix, ex:

quay.io/operator-framework/scorecard-test-kuttl-<kuttl version>:<operator-sdk version>

In the above scheme, a kuttl version can be upgraded to the next version that doesn't have a breaking change; once a break occurs, that kuttl version will be fixed and a new matrix entry created for the breaking kuttl version.

/ping @joelanford @jmccormick2001 @jmrodri

@jlpedrosa
Copy link
Contributor Author

As far as I can see, the change would impact XX-assert.yaml and kuttl-test.yaml that can be managed by a simple replace in the files with filenames following that patter. In my opinion is simple enough to include it in the migration guide to the next of operator sdk docs, rather than creating a matrix of versions, specially taking into account that they don't have planned to have an 0.8.1 and a 0.9.1, so it won't be two independent versions with support.

Also I'd like to point out that docs are referring tag image :dev which hasn't been updated since few months ago and make the test fail, so in my opinion correcting that tag and the namespace of the yaml for the new operator sdk version makes sense rather than having a matrix of versions (which won't have a long live). Users that don't want to upgrade can use an older version of the tag of operator sdk kuttl test.

@estroz
Copy link
Member

estroz commented Feb 23, 2021

As far as I can see, the change would impact XX-assert.yaml and kuttl-test.yaml that can be managed by a simple replace in the files with filenames following that patter.

The change is semantically broader than that: kuttl has deprecated an entire group, which is a pretty big breaking change. While I agree the fix is simple, operator-sdk has committed to no breaking changes within the 1.0 major version. Anyone should be able to upgrade between two minor versions, including upgrading image versions, without changing their project.

Two alternatives to the approach I suggested:

  • sed -i s/kudo.dev/kuttl.dev/g on kuttl config files before passing them to kuttl. This would be the easiest, and I don't see any issue with this as long as the version matches a version supported by kuttl.dev. If it doesn't support that version, then the breaking change problem comes up again.
  • Version the image with kuttl version only, ex. quay.io/operator-framework/scorecard-test-kuttl:<kuttl version>. This would be fine if an --output flag is supported by the image, which determines what scorecard TestStatus version to output, ex v1alpha3, for backwards and forwards compatibility (since scorecard version no longer affects image tag).

Also I'd like to point out that docs are referring tag image :dev which hasn't been updated since few months ago and make the test fail

This is a docs "bug", dev -> latest.

@jlpedrosa
Copy link
Contributor Author

@estroz I've been doing some testing with sed, unfortunately the path in which operator-sdk is mouting the yamls is read only.
sed: couldn't open temporary file /bundle/tests/scorecard/kuttl/sedFah9X9: Read-only file system

If you think it'd be appropriate I can move them to /tmp and run kuttl over there.

The option mentioned here:

Version the image with kuttl version only, ex. quay.io/operator-framework/scorecard-test-kuttl:. This would be fine if an --output flag is supported by the image, which determines what scorecard TestStatus version to output, ex v1alpha3, for backwards and forwards compatibility (since scorecard version no longer affects image tag).

Does not seem viable for me, as there is operator-sdk code (to convert the output of kuttle to scorecard), that gets injected into the container too, so if we would use kuttl version only, we would have no way to reflect changes in the binary we are using or any changes in the script for that matter.

Not certain what would be your preferred course.

@estroz
Copy link
Member

estroz commented Mar 1, 2021

Does not seem viable for me, as there is operator-sdk code (to convert the output of kuttle to scorecard), that gets injected into the container too, so if we would use kuttl version only, we would have no way to reflect changes in the binary we are using or any changes in the script for that matter.

While breakage is technically possible, the only things the operator-sdk wrapper does is run kuttl with some flags needed to generate a JSON report, decode that report, then encoding it in a scorecard output format. If a bug needs to be fixed/change made in the wrapper, new images can be built for existing kuttl version tags, such that docker pull updated-kuttl-image:<kuttl version> will refresh that image.

This solution isn't foolproof, and can cause some confusion because images tagged with a version are being refreshed in the background. You could use image sha256 tags to overcome these challenges.

I am open to other suggestions and so will post this issue in the working group meeting happening on wednesday (03/03/2021).

@jlpedrosa
Copy link
Contributor Author

@estroz I've added a new commit that solves the issue by copying the test files to a temp folder in case you want to pursue that route, for me seems like an acceptable compromise. I tested it with our project and it worked just fine, as well as the tests here are passing. In any case I'd update the docs to have the default with the new version so in the future the workaround can me removed.
As you asked for my opinion: I think that rewriting/repushing the image with the same tag can lead to all sort of problems as different behavior can occur in different clusters (cached vs pulled) and I would not go that way under no circumstances.
Let me know if you'd like me to attend the meeting.

Copy link
Member

@kensipe kensipe 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-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2021
@jlpedrosa
Copy link
Contributor Author

The CI for ansible failed, I am assuming that it's not related to my change. But I can't find a way to re-run it without pushing more changes (I don't want to wipe the review).

@estroz
Copy link
Member

estroz commented Mar 3, 2021

/ok-to-test

@openshift-ci-robot openshift-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Mar 3, 2021
@estroz
Copy link
Member

estroz commented Mar 3, 2021

@jlpedrosa you can run /retest now.

@jlpedrosa
Copy link
Contributor Author

@estroz thanks!
/retest

@jlpedrosa
Copy link
Contributor Author

/retest

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2021
@jlpedrosa
Copy link
Contributor Author

@kensipe Sorry, my commit wasn't signed and I had to push force, could you re approve this PR? Thanks

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

copying the test files to a temp folder in case you want to pursue that route

While this may resolve this particular breakage, it (1) hides complexity from users and isn't foolproof and (2) doesn't solve the general problem of resolving future breaking changes (pre kuttl 1.0).

A compromise that was brought up in the community meeting this morning: tag scorecard-test-kuttl images with an operator-sdk commit or a combination of kuttl version and operator-sdk commit, i.e. do not use operator-sdk versions in tags. This scheme gives you all version information within the image tag without exploding the build matrix.

Another, brought up afterwards, is to start versioning the scorecard-test-kuttl image independently of operator-sdk version entirely, so breaking changes between kuttl versions can be captured as major version bumps. The first, including this change, would be scorecard-test-kuttl:v2.0.0.

The second is probably the better solution, but I figured I'd field both. Do either of those fit your needs/sound better @jlpedrosa?

Either way, string substitution isn't the right solution.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2021
New version contains the capability to run assertion based on commands/scripts, that enable more complete testing of the objects being managed

Signed-off-by: Jose Luis Pedrosa <jlpedrosa@gmail.com>
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2021
@jlpedrosa jlpedrosa temporarily deployed to deploy March 8, 2021 11:56 Inactive
@jlpedrosa jlpedrosa temporarily deployed to deploy March 8, 2021 11:56 Inactive
@jlpedrosa jlpedrosa temporarily deployed to deploy March 8, 2021 11:57 Inactive
@jlpedrosa jlpedrosa temporarily deployed to deploy March 8, 2021 11:57 Inactive
@jlpedrosa jlpedrosa temporarily deployed to deploy March 8, 2021 11:57 Inactive
@jlpedrosa jlpedrosa temporarily deployed to deploy March 8, 2021 11:57 Inactive
@jlpedrosa
Copy link
Contributor Author

@estroz Any of both approaches meets my reqs, I only need to have >= 0.9 Kuttl version to be able to execute assertions.
Due to the fact that the solution you incline to is just tagging the image, I am not sure what would be the nexts steps in this PR (already rebased), could you shed some light about what would you like me to implement?

@estroz
Copy link
Member

estroz commented Mar 8, 2021

could you shed some light about what would you like me to implement

Nothing on your end yet. I'll need to set up a separate release job for scorecard-test-kuttl and get that merged first. Then you can rebase this PR onto master, and once merged I can build a v2.0.0 release.

@estroz
Copy link
Member

estroz commented Mar 12, 2021

/retest

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

Now that #4633 has merged, this is unblocked. After this merges I'll cut a v2.0.0 release! Thanks for the contribution @jlpedrosa

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2021
@estroz estroz removed the blocked label Mar 12, 2021
@estroz estroz merged commit 8757f9f into operator-framework:master Mar 12, 2021
@estroz
Copy link
Member

estroz commented Mar 12, 2021

@jlpedrosa quay.io/operator-framework/scorecard-test-kuttl:v2.0.0 has been released!

@jlpedrosa
Copy link
Contributor Author

Thanks @estroz awesome!

@jlpedrosa jlpedrosa deleted the upgrade_kuttl_090 branch March 12, 2021 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants