feat(helm): Support for flower and websocket containers

[Yann-J]

SUMMARY

This PR brings a number of updates to the Helm chart:

• Support running an optional flower sidecar container in the celery beat pod deployment, to visualize the tasks (but NOT exposed through the ingress, since most users probably don't want to expose this publicly - either a new ingress needs to be deployed, or a port-forward to the flower service is needed)
• Support the optional deployment of a superset-websocket server in a separate pod, together with associated services and ingress path within the same host(s)
• Reworked the probes to make them fully customizable through the values.yaml, added startupProbes, as well as a livenessProbe for the celery workers (that runs a celery inspect ping command)
• Added an auto-generated README, using helm-docs, to document the contents of values.yaml (and should be visible from artifacthub)
• Some minor cleanups, in particular switched all the indent to nindent so that we can apply indentation and improve readability

The more controversial change is also that I've removed the schema definition... I'm sorry but I've really find it too hard to work with, and was making iterative developments of the chart quite painfully slow, while not necessarily helping catch too many issues...

I guess this is debatable, and if that's a problem I can work on restoring it, but I personally suggest to drop it since I think it creates unnecessary work - unless there's a tool to generate it that I'm not aware of?

TESTING INSTRUCTIONS

• Deploy a new release of the chart. To enable the new features:
  • set supersetCeleryBeatFlower.enabled=true to see the flower sidecar container in the celerybeat pod, and access the UI by port-forwarding its port 5555. WARNING this requires a custom Superset image that has flower<1.0.0 installed (recent versions are causing a dependency conflict)
  • set supersetWebsockets.enabled=true to see the superset-websocket pod (by default a custom image I've built from our fork, since there's no official one). It assumes that both pods are exposed through an ingress on the same domain (the built-in ingress will do this automatically) and requires various things:
    • JWT_SECRET set as a secret env variable - it will be passed to both Superset and websocket servers
    • in superset_config.py:
      • GLOBAL_ASYNC_QUERIES_TRANSPORT=ws
      • A proper value for GLOBAL_ASYNC_QUERIES_WEBSOCKET_URL (I use f"{re.sub('^http','ws',env('BASE_URL'))}/ws")
    • GLOBAL_ASYNC_QUERIES_JWT_SECRET=env("JWT_SECRET")

This was tested successfully on Superset 1.5.2

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[craig-rueda]

Thanks for this PR -- looking great. Any reason why you went for a flower sidecar instead of a standalone deployment? It's my understanding that flower will show ALL tasks/workers and not just the ones that are being executed by the local instance.

[Yann-J]

Yes, it will show tasks for all the workers, regardless of where it runs, because it connects to the broker.
I just wanted to keep things simple, since this server is unlikely to ever require any scaling (and since celery beat has to be a singleton, I thought it was good enough). A separate pod would also work of course but was just more boilerplate...

[craig-rueda]

Yes, it will show tasks for all the workers, regardless of where it runs, because it connects to the broker. I just wanted to keep things simple, since this server is unlikely to ever require any scaling (and since celery beat has to be a singleton, I thought it was good enough). A separate pod would also work of course but was just more boilerplate...

Ok, understood. Can we pls make it another deployment? 🙏 This pod can scale depending on usage quite a bit (especially when running reports or generating thumbnails, for example)

[Yann-J]

Yes, sure, I can do this if you think it's useful.
I'm not sure about the need for scaling, since (if I understand Celery correctly), the celery workers will need scaling, but the celery beat acts as a controller and needs to be a singleton.
Flower, being a management UI, is also unlikely to require any scaling.
That beind said, there's no harm in having it as a separate deployment, that's probably the right way of doing this indeed...

[Yann-J]

OK, this is done, flower is now in its own separate Deployment.

One more change I propose is to switch the initContainers that wait for dependencies from a shell script wait to dockerize which is more robust and versatile (e.g. can wait for multiple dependencies, in our case both postgres and redis).

[Yann-J]

Another suggestion (not implemented here) is to add helm-docs as a pre-commit hooks to make sure the chart's README is always up-to-date with the values.yaml.
Not sure if that's OK...
Also, I think it would be nice to add an icon to the chart manifest, but I couldn't find an official square logo for Superset to link to...

[craig-rueda]

Sounds interesting, however pre-commit probably isn't the best spot, as folks can pretty easily skip them. What would be better would be a lint step in CI that checks the README against a generated version.

[Yann-J]

OK, I've been convinced and switched back to using the shared defaults from initImage.* for the init containers. They can still be overridden separately for each Deployment if required.

There's a very low risk of regression, but it will presumably be very quick to identify and fix, for anyone who has overridden the previous busybox default...

[Yann-J]

I would still suggest to add helm-docs as a pre-commit hook - of course CI needs double-check it, but the local hook is a very useful reminder to run it before pushing, saving CI time to catch such a trivial issue...

[craig-rueda]

Looks like there's some issues resolving bitnami?

[Yann-J]

Yes, I've seen those CI failures, and assumed this was some kind of known problem, since this is unlikely to be related to any of the changes in this PR.

I don't know very well how the CI was originally set up, but I can see that the superset-helm-lint.yaml workflow is set up to run chart-testing, however it doesn't specify any chart repos to install.

I think the fix would be to pass the list of dependent repos to install prior to running chart-testing. The ct.yaml configuration basically needs to include the following config (or passed as a CT_CHART_REPOS env var):

chart-repos:
  - bitnami=https://charts.bitnami.com/bitnami

I'm sending some commits to try a fix, but it might take a while since this might needs some trial-and-error...

[Yann-J]

OK, so I've unblocked the repository dependency issue.

The chart linter now runs successfully, but was raising a tricky error, complaining about whitespaces inside braces in values.yaml... the problem is that this linting rule is conflicting with the prettier rules (which is also used to lint YAML) - in fact these spaces were actually introduced by prettier in my VS Code automatically (I have "format on save" enabled).

I've made this linting rule a bit more lax by allowing up to one space, but you may have other opinions on how to fix it.

[Yann-J]

OK, we now have a passing linting and chart-testing! Just pushed one final polish to tweak some docs, and I think we're finally all good!

[Yann-J]

Looks like we have passing checks across the board now!

FYI I'm preparing a separate change to add helm-docs as both a pre-commit check and a CI check, but I prefer to keep this as a separate PR and not mix things too much.

[craig-rueda]

LGTM, Thanks!