feat: Allow changing emptyDir sizeLimit #311
Conversation
Signed-off-by: Christian Kreuzberger <christian.kreuzberger@dynatrace.com>
christian-kreuzberger-dtx
force-pushed
the
patch/emptyDirSize
branch
from
78e4a50
to
974f14c
Compare
Signed-off-by: Christian Kreuzberger <christian.kreuzberger@dynatrace.com>
Signed-off-by: Christian Kreuzberger <christian.kreuzberger@dynatrace.com>
|
The following Docker Images have been built:
|
There was a problem hiding this comment.
Not sure from the code if it's allowed to not specify an emptyDirSize in order to use a non-predetermined amount of ephmeral space on the node (in which case we are missing at least a test for it). Aside from that we just have a couple of typos to be fixed and maybe consolidate default values
| @@ -4,6 +4,7 @@ metadata: | |||
| name: job-service-config | |||
| data: | |||
| job_namespace: "{{ .Release.Namespace }}" | |||
| job_emptydir_sizelimit: "{{ .Values.jobConfig.emptyDirSizeLimit | default "20Mi" }}" | |||
There was a problem hiding this comment.
this means that it won't be possible not to specify a size and leave the management to the k8s host. I would suggest to leave the default in the values.yaml and remove it from here to allow users to specify "" as value already when installing.
Current solution would force users to manually modify the configmap to have no limit for emptyDir volumes...
| @@ -38,6 +38,8 @@ type envConfig struct { | |||
| ConfigurationServiceURL string `envconfig:"CONFIGURATION_SERVICE" default:""` | |||
| // The k8s namespace the job will run in | |||
| JobNamespace string `envconfig:"JOB_NAMESPACE" required:"true"` | |||
| // Kubernetes Empty Dir Size Limit for every job (default: "20Mi") | |||
| JobEmtpyDirVolumeSizeLimit string `envconfig:"JOB_EMPTYDIR_SIZELIMIT" default:"20Mi"` | |||
There was a problem hiding this comment.
Another default value layer (so we have 3: values.yaml, default helm function in the configmap template and finally here in EnvConfig), it would get confusing as which value would be used.
For example: what is the value of the emptyDir size if I explicitly specify --set jobConfig.emptyDirSizeLimit="" during helm install ?
| @@ -38,6 +38,8 @@ type envConfig struct { | |||
| ConfigurationServiceURL string `envconfig:"CONFIGURATION_SERVICE" default:""` | |||
| // The k8s namespace the job will run in | |||
| JobNamespace string `envconfig:"JOB_NAMESPACE" required:"true"` | |||
| // Kubernetes Empty Dir Size Limit for every job (default: "20Mi") | |||
| JobEmtpyDirVolumeSizeLimit string `envconfig:"JOB_EMPTYDIR_SIZELIMIT" default:"20Mi"` | |||
There was a problem hiding this comment.
| JobEmtpyDirVolumeSizeLimit string `envconfig:"JOB_EMPTYDIR_SIZELIMIT" default:"20Mi"` | |
| JobEmptyDirVolumeSizeLimit string `envconfig:"JOB_EMPTYDIR_SIZELIMIT" default:"20Mi"` |
| @@ -130,6 +132,7 @@ func processKeptnCloudEvent(ctx context.Context, event cloudevents.Event, allowL | |||
| DefaultPodSecurityContext: DefaultPodSecurityContext, | |||
| AllowPrivilegedJobs: env.AllowPrivilegedJobs, | |||
| JobLabels: JobLabels, | |||
| JobEmtpyDirVolumeSizeLimit: env.JobEmtpyDirVolumeSizeLimit, | |||
There was a problem hiding this comment.
| JobEmtpyDirVolumeSizeLimit: env.JobEmtpyDirVolumeSizeLimit, | |
| JobEmptyDirVolumeSizeLimit: env.JobEmtpyDirVolumeSizeLimit, |
| quantity := resource.MustParse("20Mi") | ||
| if jobSettings.JobEmtpyDirVolumeSizeLimit != "" { |
There was a problem hiding this comment.
with all the default values we have I am not sure that we can ever have an empty JobEmptyDirVolumeSizeLimit
| if jobSettings.JobEmtpyDirVolumeSizeLimit != "" { | |
| if jobSettings.JobEmptyDirVolumeSizeLimit != "" { |
| quantity := resource.MustParse("20Mi") | ||
| if jobSettings.JobEmtpyDirVolumeSizeLimit != "" { | ||
| quantity = resource.MustParse(jobSettings.JobEmtpyDirVolumeSizeLimit) |
There was a problem hiding this comment.
| quantity = resource.MustParse(jobSettings.JobEmtpyDirVolumeSizeLimit) | |
| quantity = resource.MustParse(jobSettings.JobEmptyDirVolumeSizeLimit) |
Fixes #170
Allows configuring emptyDir Size Limit
Note: I've chosen a helm value + env variable, as this approach is the least-breaking and gives most power the person that installs job-executor.