Connect: set meaningful (fallback) defaults for job resources

[pat-s]

Without these, packrat-restore jobs will fail on clusters

• that do not allow empty resources
• inject a (too) low default resources: settings instead

This is often the case on Openshift.

Defining these fallback values safeguards packrat-restore jobs for new apps which don't have any resource definitions set and hence run into the condition described above.

[dbkegley]

This would apply the default values globally which I think is too heavy handed. This setting would apply to all environments restores, content execution, and document renders.
Rather than setting new global defaults in the launcher template, I think my preference would be to fall back on Connect's scheduler settings which are more granular. Unfortunately, scheduler resource settings are not applied to the environment restore jobs today so that's an improvement we would have to make in Connect first.

In the meantime I think a better approach would be to modify the job.tpl to only set resource requests/limits if the configurations are present in the helm chart's values.yaml. This way we are not changing the global defaults but user's are still free to configure the values globally if needed.

launcher.templateValues section of the values file are all examples of where we have allowed users to modify the job.tpl without changing the global defaults.

helm/charts/rstudio-connect/values.yaml

Lines 290 to 311 in 2cdb401

	templateValues:
	service:
	type: ClusterIP
	annotations: {}
	labels: {}
	pod:
	annotations: {}
	labels: {}
	serviceAccountName: ""
	volumes: []
	volumeMounts: []
	env: []
	imagePullPolicy: ""
	imagePullSecrets: []
	initContainers: []
	extraContainers: []
	containerSecurityContext: {}
	defaultSecurityContext: {}
	securityContext: {}
	tolerations: []
	affinity: {}
	nodeSelector: {}

We'll also need to make sure that we always use the launcher Job's resource requests/limits if they are set, so we'll have to merge $templateData.pod.resources with .Job.resourceLimits inside the template.

helm/charts/rstudio-connect/files/job.tpl

Lines 247 to 284 in 2cdb401

	{{- $limits := dict }}
	{{- $requests := dict }}
	{{- range .Job.resourceLimits }}
	{{- if eq .type "cpuCount" }}
	{{- $_ := set $limits "cpu" .value }}
	{{- else if eq .type "CPU Request" }}
	{{- $_ := set $requests "cpu" .value }}
	{{- else if eq .type "memory" }}
	{{- $_ := set $limits "memory" (printf "%s%s" .value "M") }}
	{{- else if eq .type "Memory Request" }}
	{{- $_ := set $requests "memory" (printf "%s%s" .value "M") }}
	{{- else if eq .type "NVIDIA GPUs" }}
	{{- $val := float64 .value }}
	{{- if ne $val 0.0 }}
	{{- $_ := set $limits "nvidia.com/gpu" $val }}
	{{- end }}
	{{- else if eq .type "AMD GPUs" }}
	{{- $val := float64 .value }}
	{{- if ne $val 0.0 }}
	{{- $_ := set $limits "amd.com/gpu" $val }}
	{{- end }}
	{{- end }}
	{{- end }}
	{{- if any (ne (len $requests) 0) (ne (len $limits) 0) }}
	resources:
	{{- if ne (len $requests) 0 }}
	requests:
	{{- range $key, $val := $requests }}
	{{ $key }}: {{ toYaml $val }}
	{{- end }}
	{{- end }}
	{{- if ne (len $limits) 0 }}
	limits:
	{{- range $key, $val := $limits }}
	{{ $key }}: {{ toYaml $val }}
	{{- end }}
	{{- end }}
	{{- end }}

cc @bschwedler - is this something that the workbench chart would benefit from as well if we modify the job.tpl to support it?

[pat-s]

This would apply the default values globally which I think is too heavy handed. This setting would apply to all environments restores, content execution, and document renders.

I was aware of this when proposing the change. Right now, there are no defaults set which causes global Openshift defaults to jump in (of course global resource defaults are also possible on vanilla k8s). These will be too low in many scenarios, causing restore jobs to fail and giving admins a hard time.
To me it don't understand why one would not set meaningful resource defaults for "resource-heavy" jobs in a chart but rather wait for users/admins deploying to chart to find out the hard way.
I know that settings resources: by default is generally not best practice, however IMO this applies to low-resource jobs only, where these values fall into the "optimization" category but don't cause functional failures in the first place.

While a user configurable default is surely welcome, I still think that sensible defaults should be defined in the chart that prevent these cases from happening. Admins can still override these values then in the chart themselves if desired (and once exposed).

The limits proposed in this PR are not particularly optimized and lower values might also suffice, however I had no motivation to benchmark the best lowest value that would still work in most cases.

[dbkegley]

As you've already mentioned a namespace default using a LimitRange with extraObjects could be a workaround for some users. This may not be suitable for all users though if there are other pods besides Connect running within a namespace.

extraObjects:
- apiVersion: v1
  kind: LimitRange
  metadata:
    name: default-limit-range
  spec:
    limits:
    - default:
        cpu: 500m
        memory: 1Gi
      defaultRequest:
        cpu: 500m
        memory: 1Gi
      type: Container

Setting the default cpu/memory requests/limits aside for now, I still think your PR needs to merge $templateData.pod.resources with .Job.resourceLimits inside the template. Otherwise a content job that sets a GPU resource but not cpu/mem will ignore the default cpu/mem fallbacks defined in the job.tpl.

[pat-s]

This may not be suitable for all users though if there are other pods besides Connect running within a namespace.

The workaround I mentioned was done by patching a local copy of the chart and changing the value of the job template. This shouldn't apply to all jobs in the namespace, should it?

Besides, I think the vast majority of users are not running connect in a mixed namespace. But sure, there might be some that don't care about namespacing.

I still think your PR needs to merge $templateData.pod.resources with .Job.resourceLimits inside the template

Agreed, good point.

I also noticed that the implementation needs to be more granular for CPU and mem, e.g if users set one of the two in the app resources, the else condition will be skipped and the namespace default will kick in for the missing one, leading to potentially the same issue as when omitting both.

[pat-s]

@dbkegley I've made the conditinoal checks more granular. This should now also work if only gpu is defined in .Job.resourceLimits which then creates limits or requests as the individual sub-components are checked individually and the fallback is applied in case they are missing.

Untested though, needs a proper review.

[dbkegley]

The workaround I mentioned was done by patching a local copy of the chart and changing the value of the job template. This shouldn't apply to all jobs in the namespace, should it?

For sure, that's right. I just wanted to give another option for anyone following along who wants to make this change without waiting for this PR to merge or modifying the job.tpl

also noticed that the implementation needs to be more granular for CPU and mem, e.g if users set one of the two in the app resources, the else condition will be skipped and the namespace default will kick in for the missing one, leading to potentially the same issue as when omitting both.

Thanks for making those changes! I'll take another look this afternoon

[dbkegley]

I think my preference would be for these defaults to be defined in the values.yaml rather than here in the template. We'll want to add a new config section under launcher.templateValues.pod.resources so that the configured values are available inside of $templateData.pod.resources, then we can merge those values with the values provided on the .Job.resourceLimits - this would be more consistent with how the other job overrides work.

Take VolumeMounts for example:

      {{- if or (ne (len .Job.volumes) 0) (ne (len $templateData.pod.volumeMounts) 0) }}
          volumeMounts:
            {{- range .Job.volumeMounts }}
            - {{ nindent 14 (toYaml .) | trim -}}
            {{- end }}
            {{- range $templateData.pod.volumeMounts }}
            - {{ nindent 14 (toYaml .) | trim -}}
            {{- end }}
      {{- end }}

Except in this case we'll want to prefer values from the .Job and fall back to values in $templateData rather than appending.

[pat-s]

@dbkegley Feel free to make changes as needed and restructure the way you consider it optimal. My current workaround does the job for the instances at hand, happy to migrate to a proper upstream fix in an upcoming release.

[dbkegley]

close this in favor of #634