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

build(olm): move capability level to Seamless Upgrades #473

Merged
merged 4 commits into from
Oct 14, 2022

Conversation

ebaron
Copy link
Member

@ebaron ebaron commented Oct 12, 2022

After the fixes for #294 and #433, I believe we should qualify for the "Seamless Upgrades" operator capability level. This PR updates our CSV to use that instead of "Basic Install". This is presented to the user as part of the install process on OperatorHub.io and OpenShift's OperatorHub.

Screenshot 2022-10-12 at 11-19-44 OperatorHub · Red Hat OpenShift Container Platform

Depends on: #433
Fixes: #417

@ebaron ebaron added chore Refactor, rename, cleanup, etc. build labels Oct 12, 2022
@andrewazores
Copy link
Member

I think it would qualify for the base line of a Seamless Upgrade-capable Operator, but there's probably also more we can do to better support future Cryostat minor semver upgrades. The Cryostat database schema could change for example, or more of our persistent data could be moved from JSON filesystem storage into the database, both of which would entail some kind of database or data migration scripts/actions that ideally the Operator would run, rather than that code being embedded into Cryostat itself.

@tthvo
Copy link
Member

tthvo commented Oct 13, 2022

Just a question on this: This capability specification is likely self specified by the operator right? There is not really any test suite/checks for this?

@ebaron
Copy link
Member Author

ebaron commented Oct 13, 2022

I think it would qualify for the base line of a Seamless Upgrade-capable Operator, but there's probably also more we can do to better support future Cryostat minor semver upgrades. The Cryostat database schema could change for example, or more of our persistent data could be moved from JSON filesystem storage into the database, both of which would entail some kind of database or data migration scripts/actions that ideally the Operator would run, rather than that code being embedded into Cryostat itself.

Any ideas for how this would work? The operator could add an init-container to the Cryostat pod to run some in-line Bash script, although we would need a base image with whatever migration tools are necessary. I'm thinking this might be easier to do from Cryostat's entrypoint script though.

@ebaron
Copy link
Member Author

ebaron commented Oct 13, 2022

Just a question on this: This capability specification is likely self specified by the operator right? There is not really any test suite/checks for this?

Right, I'm not aware of any test for this yet. There is mention of adding this to Scorecard, but nothing yet: operator-framework/operator-sdk#5339

@andrewazores
Copy link
Member

I think it would qualify for the base line of a Seamless Upgrade-capable Operator, but there's probably also more we can do to better support future Cryostat minor semver upgrades. The Cryostat database schema could change for example, or more of our persistent data could be moved from JSON filesystem storage into the database, both of which would entail some kind of database or data migration scripts/actions that ideally the Operator would run, rather than that code being embedded into Cryostat itself.

Any ideas for how this would work? The operator could add an init-container to the Cryostat pod to run some in-line Bash script, although we would need a base image with whatever migration tools are necessary. I'm thinking this might be easier to do from Cryostat's entrypoint script though.

Yea, an init container was along what I was thinking, but using Cryostat's entrypoint.bash could also work. Easy enough to add a hook there to run any scripts found in some migrations.d before doing anything else. The operator could then add scripts and mount the migrations.d volume. Those same scripts would be published upstream so the Operator can just package them, and end users (including helm users?) can figure their own way to run them either directly or by hooking into the entrypoint.

@ebaron
Copy link
Member Author

ebaron commented Oct 13, 2022

Yea, an init container was along what I was thinking, but using Cryostat's entrypoint.bash could also work. Easy enough to add a hook there to run any scripts found in some migrations.d before doing anything else. The operator could then add scripts and mount the migrations.d volume. Those same scripts would be published upstream so the Operator can just package them, and end users (including helm users?) can figure their own way to run them either directly or by hooking into the entrypoint.

Do you expect that these scripts would vary based on the environment Cryostat is running in? I'm just having a hard time understanding why this wouldn't ship as part of Cryostat itself. I was thinking this would be something like checking if the database schema version is out of date, and if so, run the necessary SQL commands to update it. Cryostat would need to do this no matter where it's running wouldn't it? Maybe I'm not thinking about it the right way.

@andrewazores
Copy link
Member

They could vary, yes, for example if the user is deploying Cryostat with Postgres vs H2 then a migration script would probably have to look a little different. The entrypoint script or the migration script itself could do some preconditions checking to determine if it should run or not. But this means that Cryostat would be shipping with both of those migration scripts packaged, and both of them checking if they should run every time the Cryostat container spins up. Eventually there might be some noticeable startup time impact. It also means that the time between the container "starting" and the Cryostat JVM actually getting up and responding to health/liveness probes is a lot longer, so OpenShift/Kubernetes might just kill and restart the Cryostat container because the one-time migration scripts are taking too long to complete.

@andrewazores
Copy link
Member

Also, more specifically to Postgres in the future, that Postgres instance that the user has configured Cryostat to use may not be used exclusively by Cryostat - it could be a shared database that the user's own workloads are already using. In that case we shouldn't try to run any manual migrations and should leave that up to the user, so now the entrypoint or the migration script need some configuration to prevent themselves from running even if the entrypoint does detect that Cryostat is configured to talk to a Postgres instance.

And in that scenario, the user should really have Postgres deployed with an admin account for themselves, and separate DB user accounts for their workloads and for Cryostat - and Cryostat's account probably shouldn't even have permissions to modify the DB schema. It would be a migration for the admin user to run separately before doing the Cryostat version upgrade.

@ebaron
Copy link
Member Author

ebaron commented Oct 14, 2022

These are fair points. We'll have to discuss in more detail when planning the next release. Considering there are no database migrations required yet, is it acceptable to upgrade our capability level yet?

@andrewazores
Copy link
Member

I think so. It does mean we're committing to having that automatic migration ability built in to Cryostat and/or the Operator by the time we have a release that might need to use such migrations, but I think that's fair. Having some automatic system for migrations to be applied on upgrade should be part of the planning process for any changes that would require such a migration.

@ebaron
Copy link
Member Author

ebaron commented Oct 14, 2022

I think so. It does mean we're committing to having that automatic migration ability built in to Cryostat and/or the Operator by the time we have a release that might need to use such migrations, but I think that's fair. Having some automatic system for migrations to be applied on upgrade should be part of the planning process for any changes that would require such a migration.

Makes sense. Even if it slows down feature progression, I think it's worthwhile to ensure that users can upgrade.

@ebaron ebaron merged commit bfbcdaf into cryostatio:main Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build chore Refactor, rename, cleanup, etc.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Increase Operator Capability Level
3 participants