-
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Rehan Khan <Rehan.Khan7@ibm.com> Co-authored-by: Deepali Kushwah <Deepalikushwah@ibm.com> Signed-off-by: Rehan Khan <Rehan.Khan7@ibm.com>
@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.
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.
- 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)
# * `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:
|
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