-
Notifications
You must be signed in to change notification settings - Fork 20
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
Added support for s390x and ppc64le via catalog source #214
Conversation
@guicassolato @jasonmadigan @willthames Please review the changes for supporting authorino-operator-catalog image for s390x & pp64cle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @R3hankhan123.
First and foremost, thank you for this PR! Indeed we haven't been building the catalog images for s390x and ppc64le arches – only the operator and bundle images.
I do have a few concerns with the proposed approach nonetheless, starting with the substitution of buildah-build for a custom script. This would make Authorino the only component of Kuadrant not using buildah-build for any of its container images in CI.
Provided Buildah supports s390x and ppc64le, any particular reason why not to keep it, maybe by adding the two arches here? Did you have any issues with the current approach and supporting these arches?
I've left other comments as well. Hopefully we'll be able iterate over those together and continue with the work on this PR because it is indeed very much needed.
script/build_catalog.sh
Outdated
for tag in "${tags[@]}" | ||
do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will detach the tags into separate builds (separate manifests). Other container images (operator and olm bundle) are built just once with multiple tags. Breaking with this pattern may affect automation that depend on the link from single manifest build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opm index add
doesnt support build for multiple tags. Thats we have to iterate. buildah has the capability to build once with multiple tags which is not the case with opm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand. As I mentioned before, I'd rather not break the link between tags built based on the exact same version of the code and build args, but to keep them all linked as part of the same manifest instead.
This not being possible here, I guess we'll have to wait until we get confirmation no automation currently depends on linked tags.
In the meantime, maybe we could build the catalogs for s390x and ppc64le separately from amd64 and arm64? We keep the current ones as they are based on buildah and add another step for s390x and ppc64le? WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Deploy authorino operator using operator-sdk | ||
1. Install operator-sdk bin | ||
```sh | ||
make operator-sdk | ||
``` | ||
2. Run operator-sdk bundle command | ||
``` | ||
./bin/operator-sdk run bundle quay.io/kuadrant/authorino-operator-bundle:latest | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why removing these instructions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially catalog image support was not there for s390x and ppc64le, so this instruction was added for user to install operator using operator sdk. Now since we have added catalog support, so user can follow the generic approach.
.github/workflows/build-images.yaml
Outdated
- name: Run make catalog (main) | ||
if: ${{ github.ref_name == env.MAIN_BRANCH_NAME }} | ||
run: | | ||
make catalog \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This make target has been recently refactored to replace the deprecated Sqlite-based generation of the catalog with file-based builds (#201).
Also, I'd rather stay with single way of doing it between local dev env and CI, if possible. So I wish there's a way for us to keep relying on the make target here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opm index add
generate Operator index container images from pre-existing Operator bundles and builds as well. Since it does both the jobs so thats why we have removed make catalog
options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we can rollback to using opm index add
for generating the catalog. I'm gonna have to ask people smarter than me on this one, I'm afraid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if that means going back to sqlite based catalogs, I'd say it's not a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@didierofrivia opm index add
doesn't mean it will fall back to sqlite based catalogs. Below snapshot has been taken from OpenShift Docs. It says that default Red Hat provided Operator Catalog releases in the file based catalog format from 4.11 and we have used 4.14.4 version of OPM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it's worth, inspecting one of the pushed images (for example quay.io/r3hankhan/authorino-operator-catalog:latest-s390x), I'm pretty sure they're sqlite based and not file-based catalogs (looking at the last layer)
script/build_catalog.sh
Outdated
# * `docker` | ||
# * `opm` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's OK to assume docker, but OPM should be installed to the ./bin directory if the process depends on it. The make targets currently used to build the catalog images take care of that. we should favour that approach, if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently scripts is being invoked only from github workflow , which handles opm binary creation automatically. Incase user wants to use scripts by chance then , he has to run make target for opm as a pre-requisites. If you want we can add that in the comments.
By adding s390x and ppc64le in buildah argument catalog image is generated with x86 opm binary. Thats the reason we are using opm binary for each architecture to generate catalog image in the scripts. |
Hi @guicassolato, can you provide further comments based on the reply |
@guicassolato is there any slack channel where we can discuss your concerns regarding the PR as its being dragged on |
@R3hankhan123, you can find us all at the #kuadrant channel in the Kubernetes Slack workspace. I think the 2 main concerns at this point are:
|
Hi @guicassolato, I have made the catalog source filed based and now also the tags are linked |
Co-authored-by: Rehan Khan <[email protected]> Co-authored-by: Deepali Kushwah <[email protected]> Signed-off-by: Rehan Khan <[email protected]>
Both the points have been addressed in the latest changes |
Signed-off-by: Rehan Khan <[email protected]>
Signed-off-by: Rehan Khan <[email protected]>
… executible" This reverts commit 4900e43a8acd90e126ef0290d412598436d9e4e0. Signed-off-by: Rehan Khan <[email protected]>
Signed-off-by: Rehan Khan <[email protected]>
Signed-off-by: Rehan Khan <[email protected]>
Hi @guicassolato any more changes to be made? |
@R3hankhan123 Hey thanks for your time and the interest in our project! I'm a bit confused in your approach, you say you can't use buildah because the catalog image is generated with x86 opm binary...
The resulting docker image created with opm is referencing the In the case one need to directly specify the base opm image in the catalog dockerfile, I would choose to craft the catalog passing the Probably something like this : #222 , would that be Ok for your usecase? |
Hi @didierofrivia the PR you have raised, when I ran the workflow and noticed that the images were being overwritten ie amd64's image was being overwritten by arm64 and so on
|
Hi @R3hankhan123, have you tried specifying the podman pull quay.io/r3hankhan/authorino-operator-catalog:latest --arch=linux/arm64 OR docker pull quay.io/r3hankhan/authorino-operator-catalog:latest --platform linux/arm64 Let me know how it goes. |
@didierofrivia I am getting this
|
@R3hankhan123 Could you try first Cheers! |
@didierofrivia whn I am trying to pull image corresponding to #224 I am getting the following
But I am able to pull the image corresponding to #222 |
Signed-off-by: Rehan Khan <[email protected]>
@didierofrivia @guicassolato, now the workflows will build catalog images using buildah The workflow will be something like this |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #214 +/- ##
=======================================
Coverage 61.78% 61.78%
=======================================
Files 2 2
Lines 785 785
=======================================
Hits 485 485
Misses 249 249
Partials 51 51
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@R3hankhan123 Can you confirm that #222 solves your needs then? |
@didierofrivia PR #222 meets our needs just that the common manifests are not being created, but i have resolved that in my latest code change |
After discussing with @didierofrivia , PR #222 will work for our use case. |
Updated build image workflow to prepare Multi-arch for authorino-operator-catalog. Tested locally and deployed via the steps given in the readme
The pr was co authored by @Deepali1999 and @R3hankhan123