Add Helm chart

[dunn]

This builds on previous work:

• @johnschultz https://github.com/johnschultz/helmadon
• @Ladicle https://github.com/Ladicle/mastodon-chart

Closes #11197 cc @pchico83 @jdbohrman

---

known issues/future work:

• SSO is unsupported

• S3/Minio/GCS is unsupported

• Swift is unsupported

• WEB_DOMAIN is unsupported

• Tor is unsupported

• due to how Helm manages upgrades, there's no way (that I'm aware of) to
  guarantee that the web and sidekiq pods are rolled after the post-upgrade
  migration job completes; this means pods may sometimes have to be manually
  rolled after upgrades that include database migrations I believe changing the migrate job to pre-upgrade fixes this

[orangewolf]

I saw @Gargron request for review and wanted to help out and I’ve been living at the intersection between Rails applications and Deployment for a long time. However, 100% acknowledge that this would be my first contribution to this community or code base and as such would find it absolutely reasonably if all or some of my comments or suggests where ignored.

Overall I think this is a solid chart with lots of good things going for it. It validated and deployed in to a test cluster for me with minimal work. My only real concerns before merging would be the config vars being hand mapped (lots of non-DRY code to maintain there) and the PVC read write once volume / rails affinity issue. The later seems in my reading to drive everything on to a single node. Which seems anti-cloud. Anything else I would tend to stick in a ticket for future cleanup.
Big shout out to @dunn for the contribution. Helm charts really do make k8s distribution a breeze.

[orangewolf]

Making the job a sidecar instead for web or adding a sidecar to both web and sidekiq that wait for migrations to be latest before starting the main pod illuminate this problem. Having said that I’d lean toward opening this as a follow on ticket

[dunn]

I'm not familiar with sidecars in k8s; is that this future feature? kubernetes/enhancements#753

The alternative approaches I considered were initContainers on both pods, or simply running db:migrate as part of the pod command. But they both seemed a bit clunky, and I worried that if an instance scaled up with many replicas, either of those approaches might cause problems with a bunch of simultaneous attempted db writes.

[orangewolf]

I was thinking initContainers (this role was played by sidecars in Rancher 1 and I get them mixed up). Keep the job as is, but add an init container that waits for the database migration, something like this:

      initContainers:
        - name: check-migrations
          image: {{ .Values.rails.image.repository }}:{{ .Values.rails.image.tag }}
          command: ["/usr/bin/env ruby"]
          args:
            - "-e"
            - "loop { x = `rails db:migrate:status`; puts x; break if(x.match(/Status/) && !x.match(/down\s+20/)); sleep 5; }"
          envFrom:
            - configMapRef:
                name: {{ template "app.rails-env.name" . }}

(note the ruby is a little dense in here, it says loop until the result of db:migrate:status succeeds and does not contain the word "down" followed by one or more spaces and the number 20). The number 20 thing is so we match down 20200621104105 Change id column name but not up 20200621104105 Touchdown!)

[dunn]

I think switching the migrate job to pre-upgrade fixes this; let me know if you still see migration errors.

[orangewolf]

Instead of hand mapping (and sometime hard coding) these values, I would recommend that any that are not derived by mapped directly (so STREAMING_CLUSTER_NUM here is set by a STREAMING_CLUSTER_NUM over in values. This really simplicities both the conceptual model and the upkeep as new variables are added or needed.

I have template code for this I can drop in Monday if that sounds interesting.

[dunn]

I'd like to see your template for that, thanks!

I initially did allow adding arbitrary env values, and injected them all into the configmap with something like:

  {{- range .Values.env_vars }}
  {{ .name }}: {{ .value | quote }}
  {{- end }}

But I changed to hand mapping because I was afraid people would add all the values they thought they needed, without checking if they were already defined in the configmap.

[orangewolf]

I'm suggesting removing everywhere that takes a structure in values.template and simply remaps in in to ENV vars for the containers and move it in to a more 1-1 structure with code like this:

{{- range $key, $value := .Values.env.configmap }}
  {{ $key }}: "{{ $value }}"
{{- end }}

In the values file you'd have things like

env:
  configmap: 
    SMTP_AUTH_METHOD: 
    SMTP_CA_FILE:
    STREAMING_CLUSTER_NUM:

etc. So you are still showing all the important ones in the template, but if you miss one they don't need a new chart version to add it. Also adding one to the chart example is as simple as updating the values template in one spot.

Note - all the derived values I would keep as is.

[dunn]

By derived values I assume you mean ones like

  DB_HOST: {{ template "mastodon.postgresql.fullname" . }}
  DB_NAME: {{ .Values.postgresql.postgresqlDatabase }}

since golang templating doesn't work in values.yaml and throws errors like

Error: cannot load values.yaml: error converting YAML to JSON: yaml: invalid map key: map[interface {}]interface {}{".Values.postgresql.postgresqlDatabase":interface {}(nil)}

But if we keep all those values where they are in the configmap and move the rest to values.yaml as their ENV names, then we get this: dunn@33e2c10

Which IMO isn't great, since anyone deploying the chart has to know to look in the configmap to avoid clobbering already-defined variables. That's what I was trying to avoid with the hand-mapping approach.

[dunn]

A minor thing, but we also lose the ability to flag required values in secrets.yaml, since we can't interpolate values into the error message; this

  {{- if (empty $value) }}
  {{ $key }}: {{ required "$key is required" $value }}
  {{- else }}

doesn't print a useful warning when the user fails to set a value for a given variable:

Error: UPGRADE FAILED: execution error at (mastodon/templates/secrets.yaml:11:17): $key is required

[orangewolf]

Does this mean that all sidekiq instances have to live together on one node? Doesn’t that severely limit scaling?

[dunn]

Is there any way around that, without ReadWriteMany PVCs? (The cluster I tested this on is on Linode, which doesn't yet offer rwx storage.)

[dunn]

One way to mitigate it is something like 514c145, although that means you can only specify one access mode for the PVCs; I'm not sure how much of a drawback that is.

[orangewolf]

This sort of shared write space really requires ReadWriteMany to get any kind of scaling and since a lot of providers have this out of the box or allow you to configure NFS easily, I would think supporting it is worth while and that forcing ReadWriteOnce and the affinity is going to cause confusion.

[dunn]

In addition to 514c145, does 4612f39 clarify things enough?

[dunn]

Rebased those commits as b97babf.

[orangewolf]

I suggest that this should be settable via a variable. It has implications not just in how much work each container can do but also because sidekiq can be rather aggressive how much load you are putting on various other services like fs and db. It can make since for example if doing a lot of image / video processing to have more nodes doing fewer threads to keep io in check

[dunn]

Agreed; I haven't tested this at scale (and the only Mastodon instance I currently run is very small), so I just kept the values from the stock systemd service files. I'll make it configurable, and if you have suggestions about reasonable defaults that'd be great!

[pchico83]

@dunn this is great!
I will test the chart later this week, but it LGTM :-)

[dunn]

Added a job in 2556584 to create an initial admin.

[Gargron]

So, is this good to merge now?

[dunn]

@Gargron I think we're in good shape, though I wouldn't mind hearing back from @orangewolf about the environment variable handling, since changing that later would be the Helm equivalent of a breaking change.

[orangewolf]

sorry for the vanish, stopped getting updates for some reason. @dunn - just wanted to come back and say this looks awesome!