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.

Sign up for GitHub

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

Jump to bottom

feat: Helm for DB config, MariaDB configurable storage #531

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

feat: Helm for DB config, MariaDB configurable storage #531

wants to merge 12 commits into from

Conversation

skrobul
Copy link
Collaborator

@skrobul skrobul commented Dec 3, 2024 •
edited
Loading

Caution

Do note merge without reading the section below!

Description

This PR changes how the MariaDB component for OpenStack is configured.

By moving to a dedicated Helm chart, we allow better customisation of options delivered to the mariadb-operator.
The main reason behind this change was to have the ability to change the storageClass used to back the database PVCs, as we previously relied on a default storage class being configured correctly.

Please review, but DO NOT MERGE because ArgoCD's autosync may wipe the database and other resources if the changes are not applied in correct order.
I am planning to integrate this change as outlined in the plan below (please provide feedback on it too if you have any):

Depends on https://github.com/RSS-Engineering/undercloud-deploy/pull/239
Closes PUC-521

Migration Plan to switchover clusters to UnderstackDB

  • merge the PR for deploy repo
  • make sure that both dev and cluster follow HEAD
  • create a new branch (migration-X)that only disables autosync for operators and openstack components.
  • set BOTH clusters to follow the migration-X branch
  • verify that autosync is disabled for both openstack and operators appsets through CLI or UI, refresh as necessary
  • merge the understack PR into the migration-X branch
    • refresh the bravo-uc-iad3-understackdb app, review the changes. This should mostly be annotation and name changes like these: 
      ❯ argocd app diff argocd/bravo-uc-iad3-dev-understackdb 

===== k8s.mariadb.com/Database openstack/glance ======
8c8
<     argocd.argoproj.io/instance: bravo-uc-iad3-dev-glance
---
>     argocd.argoproj.io/instance: bravo-uc-iad3-dev-understackdb
53a54
>   name: glance

===== k8s.mariadb.com/Database openstack/horizon ======
8c8
<     argocd.argoproj.io/instance: bravo-uc-iad3-dev-horizon
---
>     argocd.argoproj.io/instance: bravo-uc-iad3-dev-understackdb
53a54
>   name: horizon

  [...]
  • Sync the bravo-uc-iad3-understackdb if you are happy with those changes.
  • Switchover to the openstack appset and verify that the User,Database,Grant resources have moved away from their relevant apps.
  • Repeat the same for charlie/staging cluster.
  • Verify that everything is in sync and there is no outstanding diffs
  • merge the PR to main branch.
  • Switchover the ArgoCD instances to follow HEAD/HEAD or their equivalent refs again.

@skrobul skrobul force-pushed the mariadb-configurable-storage branch 30 times, most recently from 8732afb to 12d4cd5 Compare December 5, 2024 10:09
@skrobul skrobul force-pushed the mariadb-configurable-storage branch 8 times, most recently from 6df0326 to daee5f5 Compare December 5, 2024 17:47
@skrobul skrobul changed the title WIP - Helm for DB config, Mariadb configurable storage feat: Helm for DB config, MariaDB configurable storage Dec 5, 2024
@skrobul skrobul marked this pull request as ready for review December 5, 2024 18:07
@skrobul skrobul requested a review from a team December 5, 2024 18:08
skrobul added 12 commits December 5, 2024 18:55
@skrobul
extract MariaDB instance config into a chart
4dfc08c
@skrobul
understackdb: create templates
19fdaf6 
This creates helm templates for grants, databases and users based on the
current setup.
@skrobul
components: remove old way of creating db,grants,users
587c9cd
@skrobul
helm-charts: add CI/CD, pre-commit, remove duplicates
3036fb2 
- add chart linting with helm-lint and chart-testing
- remove duplicate YAML linter: we are already doing yamllint as part of the pre-commit run.

The helm-charts are excluded anyways because they would produce incorrect results until they are rendered.
@skrobul
understackdb: add tests
0700c77
@skrobul
understackdb: add support storageClassName and volumeClaimTemplate
d0bfee1
@skrobul
understackdb: add documentation
8a4e377
@skrobul
helm-charts: add tooling for documentation
516f367
@skrobul
helm-charts: switch release to OCI registry
16d64eb 
We cannot use the traditional HTTP repository hosted on the Github
Pages, because the understack repo usees github actions to publish the
website through mkdocs. The hard requirement for the
`actions/deploy-pages` action is that the artifcat that is upload is
created inside the same workflow. Currently the artifact is produced by
other mkdocs workflow so using traditional Helm Chart Releaser would
overwrite the page. We do not want that.

The potential workaround would be to:
- have the mkdocs push to a temporary branch
- have the mkdocs workflow stop pushing to Github Pages
- have the helm-chart-releaser not publish `index.yaml` to the
  `gh-pages`, but to the temporary branch instead
- additional workflow would need to be setup to react on pushes to the
  temporary branch. That workflow would need to merge outputs of mkdocs
  and index.yaml from helm chart releaser, then upload and publish to GH
  pages.

This workaround was never implemented or tested, because going down the
route of publishing the Helm chart to OCI index sounds easier as the
OCI method does not need the index.yaml.
@skrobul
argocd: add understackdb component
7faa392
@skrobul
understackdb: handle db name discrepancies
856977d 
In our current setup, the Kubernetes resource is called 'nova-api', but
the database name is 'nova_api'.
@skrobul
gitops-secrets-gen: pre-create directory for understackdb deploy
6c86ed8
@skrobul skrobul force-pushed the mariadb-configurable-storage branch from daee5f5 to 6c86ed8 Compare December 5, 2024 18:55
@cardoe
Copy link
Contributor

cardoe commented Dec 5, 2024

So I don't understand why the Database, User and Grant objects are being moved. Cause certain pieces will be deployed in global, regional, and fabric levels. With this setup you're bringing everything to one scope which works for AIO but not in a large scale.

I thought we would put the pieces in components/openstack into a helm chart and that's it.

@skrobul
Copy link
Collaborator Author

skrobul commented Dec 5, 2024 •
edited
Loading

So I don't understand why the Database, User and Grant objects are being moved.

Few reasons:

  1. Mostly because of this comment, if that's not what you meant, can you please elaborate on what is the actual requirement here?

  2. This removes clearly repetitive, error prone, hardcoded yaml which can now be generated by providing only what matters (the metadata).

  3. Logically it makes sense to me keep the database configuration in a single place rather than spread over several files. It is also more consistent and now takes total of 51 lines instead of 376 lines.

Cause certain pieces will be deployed in global, regional, and fabric levels.

We would still deploy those releases on global, regional and fabric levels - just with different values. All with auditable history, versioning and other benefits of using charts. Happy to discuss this further if it's not premature optimization.

I thought we would put the pieces in components/openstack into a helm chart and that's it.

Ok, now I don't understand why this was said in the JIRA about "configuration of storageclass for MariaDB" which did not mention openstack anywhere...

Now, ignoring that for a minute - let's look into the contents of this folder - we can certainly work towards moving it, but in it's current form it may not be the best idea to just throw it into a chart. It's looking like dumping ground for random pieces - it has:

  • part of the mariadb cluster config (this PR takes care of that part)
  • value file for memcached chart
  • YAML manifest for rabbitmq cluster
  • 4 manifests for External Secrets
  • service account for argo workflows
  • the README file which states that these are pre-req components for the OpenStack.

This README is not accurate, as you cannot deploy OpenStack without having Databases, Users and Grants deployed.

@cardoe
Copy link
Contributor

cardoe commented Dec 6, 2024 •
edited
Loading

So I don't understand why the Database, User and Grant objects are being moved.

Few reasons:

  1. Mostly because of this comment, if that's not what you meant, can you please elaborate on what is the actual requirement here?

I was thinking the creation of the DB instance / management of it as well as the RabbitMQ instance. There are however some shared pieces that can exist. Like the DB instance and the RabbitMQ instance that other components will depend upon. What we really need to start doing is breaking stuff up along the boundaries laid out on https://rackerlabs.github.io/understack/deploy-guide/

So 100% a lot of that contents needs to get broken up. What I was suggesting to you prior to PTO was that we needed to make that piece a helm chart and then have a helm chart for each OpenStack piece that had the OpenStack Helm chart as a dependency. We could use a library chart to prevent duplication. Creating the DB for a service seemed to pair with the actual service itself in my mind but if you want to centralize it I won't protest.

I don't think we need to be releasing a helm chart. That's only useful if something external is going to be installing it. This is an internal helm chart and by doing releases it'll make testing things a lot harder. You'll have to release the chart and then change the repo to consume the new release. Just use the chart straight from git at the tag/commit that you're using the rest of the repo.

@skrobul
Copy link
Collaborator Author

skrobul commented Dec 9, 2024 •
edited
Loading

Thanks. That sounds like a bigger restructuring that will impact more than just a database related features. At first thought sticking RabbitMQ on top of that feels like it's random addition that currently has just 4 lines of code and zero customization. If we draw the line at "shared" bits specifically for openstack deployment then we probably need to include the memcached and ES too.

What I was suggesting to you prior to PTO was that we needed to make that piece a helm chart and then have a helm chart for each OpenStack piece that had the OpenStack Helm chart as a dependency. We could use a library chart to prevent duplication. Creating the DB for a service seemed to pair with the actual service itself in my mind but if you want to centralize it I won't protest.

That approach makes more sense to me and it was kinda what I was trying to achieve here by creating another virtual "component" that sets up everything related to the database. If you'd rather have the instance setup as part of the "shared" chart, but Database,Grants and Users configured with their respective components, I'm also fine with that as long as it's DRY, flexible and consistent. Individual YAML files in folders aren't, but if we create per-component charts that are partially fed helpers from the shared chart it may be a good choice.

Before I make any changes here I would like to make sure that we are on the same page so let's chat about this in person when you have a minute.

@skrobul skrobul marked this pull request as draft December 11, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@skrobul @cardoe