Add per-service scheduling config support

[michaellzc]

This PR introduces three partial templates for tolerations, affinity, and nodeSelector.

Usage

trim may seem weird, but this is the only way to avoid introducing the extra \n which is bugging me a lot...

{{- include "sourcegraph.nodeSelector" (list . "serviceName" ) | trim | nindent 6 }}
{{- include "sourcegraph.affinity" (list . "serviceName" ) | trim | nindent 6 }}
{{- include "sourcegraph.tolerations" (list . "serviceName" ) | trim | nindent 6 }}

Most changes are just copying & pasting, but please help me double-check!

TODO

not blocking

• Document usage of our per-service magic values, i.e., stuff that are referenced from templates but not defined in values.yaml
  • maybe we should include them explicitly in values.yaml? Related -> Autogen doc for helm charts sourcegraph#31016
• Considering adding snapshot (golden files) testing to our helm chart to ensure future changes won't accidentally cause breakage?

Test plan

override.yaml

sourcegraph:
  nodeSelector:
    node-type: memory-optimized
  tolerations:
  - key: node-type
    operator: Equal
    value: memory-optimized
    effect: NoSchedule
  affinity:
    nodeAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
        nodeSelectorTerms:
        - matchExpressions:
          - key: kubernetes.io/zone
            operator: In
            values:
            - zone-1
            - zone-

frontend:
  nodeSelector:
    node-type: general

Inspect the output

helm template -n sourcegraph -f override.yaml sourcegraph charts/sourcegraph/. > bundle.yaml

[michaellzc]

Current dependencies on/for this PR:

• main
  • PR Add per-service scheduling config support #46 (this PR)

This comment was auto-generated by Graphite.

[caugustus-sourcegraph]

Very nice!

[caugustus-sourcegraph]

I was on the fence about whether this "if service, else if global" pattern was the right choice. It could mean a lot of duplication if there's a globally shared setting + service-specific settings. But I can't think of a nice way to support both situations (have the option to include global or exclude global, in addition to service-specific).

This scenario is pretty similar to the problem faced with the image tags, and I'm not very happy about the solution for that.

Your approach here means it is possible to get the right outcome for each service, and that's what's most important!

[michaellzc]

I was on the fence about whether this "if service, else if global" pattern was the right choice. It could mean a lot of duplication if there's a globally shared setting + service-specific settings. But I can't think of a nice way to support both situations (have the option to include global or exclude global, in addition to service-specific).

This scenario is pretty similar to the problem faced with the image tags, and I'm not very happy about the solution for that.

Your approach here means it is possible to get the right outcome for each service, and that's what's most important!

Are you talking about something like

global:
  nodeSelector:
    zone: zone-1

frontend:
  nodeSelector:
    zone: zone-1
    node-purpose: generic

the users have to duplicate zone: zone-1 in the per-service override, but we want to avoid the duplicatoin

Maybe we can merge global and per-service nodeSelector? so users can just do

global:
  nodeSelector:
    zone: zone-1

frontend:
  nodeSelector:
    node-purpose: generic

The rendered frontend-deployment would have both zone=zone-1 and node-purpose: generic in the nodeSelector?

This could potentially be put behind a flag, such as global.mergeNodeSelector=true? Or we can introduce a new value like global.sharedNodeSelector, then merge it with the per-service selector.

tbh, I am not sure if this is a good idea. It's possible users accidentally enable this flag and keep scratching their head wondering what's going on.

Another approach is educating user about alias and anchor in yaml, they can technically avoid the duplication by doing

commonNodeSelectorLabels: &commonNodeSelectorLabels
  zone: zone-1

global:
  nodeSelector: *commonNodeSelectorLabels

frontend:
  nodeSelector:
    <<: *commonNodeSelectorLabels
    node-purpose: generic

[caugustus-sourcegraph]

educating user about alias and anchor in yaml

Great point! I always forget about that feature (every time I wanted to use it, there were reasons it didn't work out). That's much better than trying to build in any fancy functionality.

[michaellzc]

educating user about alias and anchor in yaml

Great point! I always forget about that feature (every time I wanted to use it, there were reasons it didn't work out). That's much better than trying to build in any fancy functionality.

LOL, I am never a fan of alias and anchor anyway. Also, the fact you can't merge sequences/list/array with it also feels off to me.

This would be a great candidate for docs improvement! sourcegraph/sourcegraph#31821