adutra commented on code in PR #4755:
URL: https://github.com/apache/polaris/pull/4755#discussion_r3418977024


##########
helm/polaris/values.yaml:
##########
@@ -1167,3 +1167,104 @@ tasks:
   # -- The maximum number of tasks that can be queued up for execution. If 
unspecified or zero, defaults to Integer.MAX_VALUE.
   # @section -- Tasks
   maxQueuedTasks: 0  # 1000
+
+# @schema type: object
+# -- Configuration for maintenance tasks using as Kubernetes CronJob
+# @section -- Maintenance
+maintenance:
+  # @schema additionalProperties: {type: string}
+  # -- Annotations to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  annotations: {}
+
+  # @schema item: object
+  # -- Extra environment variables to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraEnv: []
+  #  - name: AWS_STORAGE_BUCKET
+  #    value: s3://xxxxx/
+  #  - name: AWS_ACCESS_KEY_ID
+  #    valueFrom:
+  #      secretKeyRef:
+  #        name: aws-secret
+  #        key: access_key_id
+  #  - name: AWS_SECRET_ACCESS_KEY
+  #    valueFrom:
+  #      secretKeyRef:
+  #        name: aws-secret
+  #        key: secret_access_key
+
+  # @schema item: object
+  # -- Bulk import environment variables from Secrets or ConfigMaps to every 
maintenance Cronjob object.
+  # @section -- Maintenance
+  envFrom: []
+  #  - secretRef:
+  #      name: polaris-env-secret
+  #  - configMapRef:
+  #      name: polaris-env-configmap
+
+  # @schema item: object
+  # -- Extra volumes to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraVolumes: []
+    # - name: extra-volume
+    #   emptyDir: {}
+
+  # @schema item: object
+  # -- Extra volume mounts to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraVolumeMounts: []
+    # - name: extra-volume
+    #   mountPath: /usr/share/extra-volume
+
+  # -- The container image used by every maintenance Cronjob object.
+  # @section -- Maintenance
+  image:
+    # -- The image repository to pull from for the Polaris admin tool.
+    # @section -- Maintenance
+    repository: apache/polaris-admin-tool
+    # @schema enum: [Always, IfNotPresent, Never]
+    # -- The image pull policy.
+    # @section -- Maintenance
+    pullPolicy: IfNotPresent
+    # -- The image tag.
+    # @section -- Maintenance
+    tag: "latest"  # This tag will be replaced with the chart version at 
release time.

Review Comment:
   You now need to update the `update_helm_version` in 
`./releasey/libs/_version.sh:107` and include a new replacement for this tag:
   
   
https://github.com/apache/polaris/blob/0c730504ee5bbfee6761823e4a914546f35074a9/releasey/libs/_version.sh#L107-L125



##########
helm/polaris/values.yaml:
##########
@@ -1167,3 +1167,104 @@ tasks:
   # -- The maximum number of tasks that can be queued up for execution. If 
unspecified or zero, defaults to Integer.MAX_VALUE.
   # @section -- Tasks
   maxQueuedTasks: 0  # 1000
+
+# @schema type: object
+# -- Configuration for maintenance tasks using as Kubernetes CronJob
+# @section -- Maintenance
+maintenance:
+  # @schema additionalProperties: {type: string}
+  # -- Annotations to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  annotations: {}
+
+  # @schema item: object
+  # -- Extra environment variables to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraEnv: []
+  #  - name: AWS_STORAGE_BUCKET
+  #    value: s3://xxxxx/
+  #  - name: AWS_ACCESS_KEY_ID
+  #    valueFrom:
+  #      secretKeyRef:
+  #        name: aws-secret
+  #        key: access_key_id
+  #  - name: AWS_SECRET_ACCESS_KEY
+  #    valueFrom:
+  #      secretKeyRef:
+  #        name: aws-secret
+  #        key: secret_access_key
+
+  # @schema item: object
+  # -- Bulk import environment variables from Secrets or ConfigMaps to every 
maintenance Cronjob object.
+  # @section -- Maintenance
+  envFrom: []
+  #  - secretRef:
+  #      name: polaris-env-secret
+  #  - configMapRef:
+  #      name: polaris-env-configmap
+
+  # @schema item: object
+  # -- Extra volumes to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraVolumes: []
+    # - name: extra-volume
+    #   emptyDir: {}
+
+  # @schema item: object
+  # -- Extra volume mounts to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraVolumeMounts: []
+    # - name: extra-volume
+    #   mountPath: /usr/share/extra-volume
+
+  # -- The container image used by every maintenance Cronjob object.
+  # @section -- Maintenance
+  image:
+    # -- The image repository to pull from for the Polaris admin tool.
+    # @section -- Maintenance
+    repository: apache/polaris-admin-tool
+    # @schema enum: [Always, IfNotPresent, Never]
+    # -- The image pull policy.
+    # @section -- Maintenance
+    pullPolicy: IfNotPresent
+    # -- The image tag.
+    # @section -- Maintenance
+    tag: "latest"  # This tag will be replaced with the chart version at 
release time.
+
+  # @schema additionalProperties: {type: object, required: [schedule]}
+  # -- Define maintenance CronJobs. The key is the name of the job.
+  # @section -- Maintenance
+  jobs:
+    nosql-maintenance:
+      # -- Enable this maintenance job.
+      # @section -- Maintenance
+      enabled: false
+      # -- The schedule in Cron format.
+      # @section -- Maintenance
+      schedule: "0 2 * * *"
+      # -- The arguments to pass to the admin tool.
+      # @section -- Maintenance
+      args: ["nosql", "maintenance-run"]
+      # @schema enum: [Allow, Forbid, Replace]
+      # -- The concurrency policy, Valid values are: Allow, Forbid, Replace.
+      # @section -- Maintenance
+      concurrencyPolicy: Forbid
+      # @schema type: object
+      # -- Configures the resources requests and limits for this job's 
container.
+      # @section -- Maintenance
+      resources:
+        {}
+        # limits:
+        #   cpu: 100m
+        #   memory: 128Mi
+        # requests:
+        #   cpu: 100m
+        #   memory: 128Mi
+      # Optional per-job knobs (uncomment to use):
+      # restartPolicy: Never

Review Comment:
   I think you should uncomment these so that they get properly documented / 
included in the values.schema.json file. 
   
   The tricky thing with auto-generated docs and schema is that we should avoid 
commented-out values as much as possible.



##########
helm/polaris/templates/_helpers.tpl:
##########
@@ -215,6 +232,16 @@ Prints an environment variable for a secret key reference.
 {{- end -}}
 {{- end -}}
 
+{{/*
+Prints database/persistence connection environment variables.
+*/}}
+{{- define "polaris.persistenceEnv" -}}
+{{- include "polaris.secretToEnv" (list 
.Values.persistence.relationalJdbc.secret "username" 
"quarkus.datasource.username") -}}

Review Comment:
   These secrets are now duplicated here:
   
   
https://github.com/apache/polaris/blob/11b713a6d8fb1c1efc9733ae4cd60af22a2f7155/helm/polaris/templates/deployment.yaml#L98-L101
   
   Can we call the new template there?



##########
helm/polaris/values.yaml:
##########
@@ -1167,3 +1167,104 @@ tasks:
   # -- The maximum number of tasks that can be queued up for execution. If 
unspecified or zero, defaults to Integer.MAX_VALUE.
   # @section -- Tasks
   maxQueuedTasks: 0  # 1000
+
+# @schema type: object
+# -- Configuration for maintenance tasks using as Kubernetes CronJob
+# @section -- Maintenance
+maintenance:
+  # @schema additionalProperties: {type: string}
+  # -- Annotations to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  annotations: {}
+
+  # @schema item: object
+  # -- Extra environment variables to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraEnv: []
+  #  - name: AWS_STORAGE_BUCKET
+  #    value: s3://xxxxx/
+  #  - name: AWS_ACCESS_KEY_ID
+  #    valueFrom:
+  #      secretKeyRef:
+  #        name: aws-secret
+  #        key: access_key_id
+  #  - name: AWS_SECRET_ACCESS_KEY
+  #    valueFrom:
+  #      secretKeyRef:
+  #        name: aws-secret
+  #        key: secret_access_key
+
+  # @schema item: object
+  # -- Bulk import environment variables from Secrets or ConfigMaps to every 
maintenance Cronjob object.
+  # @section -- Maintenance
+  envFrom: []
+  #  - secretRef:
+  #      name: polaris-env-secret
+  #  - configMapRef:
+  #      name: polaris-env-configmap
+
+  # @schema item: object
+  # -- Extra volumes to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraVolumes: []
+    # - name: extra-volume
+    #   emptyDir: {}
+
+  # @schema item: object
+  # -- Extra volume mounts to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraVolumeMounts: []
+    # - name: extra-volume
+    #   mountPath: /usr/share/extra-volume
+
+  # -- The container image used by every maintenance Cronjob object.
+  # @section -- Maintenance
+  image:
+    # -- The image repository to pull from for the Polaris admin tool.
+    # @section -- Maintenance
+    repository: apache/polaris-admin-tool
+    # @schema enum: [Always, IfNotPresent, Never]
+    # -- The image pull policy.
+    # @section -- Maintenance
+    pullPolicy: IfNotPresent
+    # -- The image tag.
+    # @section -- Maintenance
+    tag: "latest"  # This tag will be replaced with the chart version at 
release time.
+
+  # @schema additionalProperties: {type: object, required: [schedule]}
+  # -- Define maintenance CronJobs. The key is the name of the job.

Review Comment:
   ```suggestion
     # -- Define maintenance CronJobs. By default, only a `nosql-maintenance` 
job is included, but other jobs can be defined by adding more fields to the 
`jobs` object. The field name becomes the job name.
   ```



##########
helm/polaris/values.yaml:
##########
@@ -1167,3 +1167,104 @@ tasks:
   # -- The maximum number of tasks that can be queued up for execution. If 
unspecified or zero, defaults to Integer.MAX_VALUE.
   # @section -- Tasks
   maxQueuedTasks: 0  # 1000
+
+# @schema type: object
+# -- Configuration for maintenance tasks using as Kubernetes CronJob
+# @section -- Maintenance
+maintenance:
+  # @schema additionalProperties: {type: string}
+  # -- Annotations to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  annotations: {}
+
+  # @schema item: object
+  # -- Extra environment variables to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraEnv: []
+  #  - name: AWS_STORAGE_BUCKET
+  #    value: s3://xxxxx/
+  #  - name: AWS_ACCESS_KEY_ID
+  #    valueFrom:
+  #      secretKeyRef:
+  #        name: aws-secret
+  #        key: access_key_id
+  #  - name: AWS_SECRET_ACCESS_KEY
+  #    valueFrom:
+  #      secretKeyRef:
+  #        name: aws-secret
+  #        key: secret_access_key
+
+  # @schema item: object
+  # -- Bulk import environment variables from Secrets or ConfigMaps to every 
maintenance Cronjob object.
+  # @section -- Maintenance
+  envFrom: []
+  #  - secretRef:
+  #      name: polaris-env-secret
+  #  - configMapRef:
+  #      name: polaris-env-configmap
+
+  # @schema item: object
+  # -- Extra volumes to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraVolumes: []
+    # - name: extra-volume
+    #   emptyDir: {}
+
+  # @schema item: object
+  # -- Extra volume mounts to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraVolumeMounts: []
+    # - name: extra-volume
+    #   mountPath: /usr/share/extra-volume
+
+  # -- The container image used by every maintenance Cronjob object.
+  # @section -- Maintenance
+  image:
+    # -- The image repository to pull from for the Polaris admin tool.
+    # @section -- Maintenance
+    repository: apache/polaris-admin-tool
+    # @schema enum: [Always, IfNotPresent, Never]
+    # -- The image pull policy.
+    # @section -- Maintenance
+    pullPolicy: IfNotPresent
+    # -- The image tag.
+    # @section -- Maintenance
+    tag: "latest"  # This tag will be replaced with the chart version at 
release time.
+
+  # @schema additionalProperties: {type: object, required: [schedule]}
+  # -- Define maintenance CronJobs. The key is the name of the job.
+  # @section -- Maintenance
+  jobs:
+    nosql-maintenance:
+      # -- Enable this maintenance job.
+      # @section -- Maintenance
+      enabled: false
+      # -- The schedule in Cron format.
+      # @section -- Maintenance
+      schedule: "0 2 * * *"
+      # -- The arguments to pass to the admin tool.
+      # @section -- Maintenance
+      args: ["nosql", "maintenance-run"]

Review Comment:
   Should we add support for `command` as well ? If the image has no 
entrypoint, `args` will fail. And since we allow users to define their own 
jobs, we cannot know which image they will use.



##########
helm/polaris/templates/cronjob-maintenance.yaml:
##########
@@ -0,0 +1,149 @@
+{{/*
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+   http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing,
+  software distributed under the License is distributed on an
+  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+  KIND, either express or implied.  See the License for the
+  specific language governing permissions and limitations
+  under the License.
+*/}}
+
+{{- range $jobName, $jobSpec := .Values.maintenance.jobs }}
+{{- if $jobSpec.enabled }}
+{{- $cronName := printf "%s-maint-%s" (include "polaris.fullname" $) $jobName 
}}
+{{- if gt (len $cronName) 52 }}
+  {{- fail (printf "Maintenance CronJob name %q is %d chars; must be <= 52 
(K8s appends a timestamp+random suffix to generated Pod names, which are capped 
at 63). Shorten release name, nameOverride/fullnameOverride, or the job key 
%q." $cronName (len $cronName) $jobName) }}
+{{- end }}
+---
+apiVersion: batch/v1
+kind: CronJob
+metadata:
+  name: {{ $cronName }}
+  namespace: {{ $.Release.Namespace }}
+  labels:
+    {{- include "polaris.labels" $ | nindent 4 }}
+  {{- with $.Values.maintenance.annotations }}
+  annotations:
+    {{- tpl (toYaml .) $ | nindent 4 }}
+  {{- end }}
+spec:
+  schedule: {{ $jobSpec.schedule | quote }}
+  concurrencyPolicy: {{ default "Forbid" $jobSpec.concurrencyPolicy }}
+  {{- if hasKey $jobSpec "successfulJobsHistoryLimit" }}
+  successfulJobsHistoryLimit: {{ $jobSpec.successfulJobsHistoryLimit }}
+  {{- end }}
+  {{- if hasKey $jobSpec "failedJobsHistoryLimit" }}
+  failedJobsHistoryLimit: {{ $jobSpec.failedJobsHistoryLimit }}
+  {{- end }}
+  jobTemplate:
+    spec:
+      {{- if hasKey $jobSpec "backoffLimit" }}
+      backoffLimit: {{ $jobSpec.backoffLimit }}
+      {{- end }}
+      {{- with $jobSpec.activeDeadlineSeconds }}
+      activeDeadlineSeconds: {{ . }}
+      {{- end }}
+      template:
+        metadata:
+          labels:
+            helm.sh/chart: {{ include "polaris.chart" $ }}
+            app.kubernetes.io/name: {{ include "polaris.name" $ }}-maintenance
+            app.kubernetes.io/instance: {{ $.Release.Name }}
+            app.kubernetes.io/component: maintenance
+            app.kubernetes.io/managed-by: {{ $.Release.Service }}
+            {{- if $.Chart.AppVersion }}
+            app.kubernetes.io/version: {{ $.Chart.AppVersion | quote }}
+            {{- end }}
+          {{- with $jobSpec.podAnnotations }}
+          annotations:
+            {{- tpl (toYaml .) $ | nindent 12 }}
+          {{- end }}
+        spec:
+          {{- if $.Values.imagePullSecrets }}
+          imagePullSecrets:
+          {{- range $.Values.imagePullSecrets }}
+            - name: {{ . | quote }}
+          {{- end }}
+          {{- end }}
+          restartPolicy: {{ default "Never" $jobSpec.restartPolicy }}
+          {{- with $.Values.priorityClassName }}
+          priorityClassName: {{ tpl . $ | quote }}
+          {{- end }}
+          serviceAccountName: {{ include "polaris.serviceAccountName" $ }}

Review Comment:
   I think indeed we should give users the ability to use a different SA, 
defaulting to the global SA if not overridden at job level. Wdyt?



##########
helm/polaris/values.yaml:
##########
@@ -1167,3 +1167,104 @@ tasks:
   # -- The maximum number of tasks that can be queued up for execution. If 
unspecified or zero, defaults to Integer.MAX_VALUE.
   # @section -- Tasks
   maxQueuedTasks: 0  # 1000
+
+# @schema type: object
+# -- Configuration for maintenance tasks using as Kubernetes CronJob
+# @section -- Maintenance
+maintenance:
+  # @schema additionalProperties: {type: string}
+  # -- Annotations to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  annotations: {}
+
+  # @schema item: object
+  # -- Extra environment variables to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraEnv: []

Review Comment:
   Shouldn't we merge `maintenant.extraEnv` with `extraEnv`? Same for 
`envFrom`, `extraVolumes` and `extraVolumeMounts`?
   
   I'm fine either way, but I think this should be documented since users will 
likely wonder, since some things are inherited from the global level (e.g. 
`podSecurityContext`).



##########
helm/polaris/values.yaml:
##########
@@ -1167,3 +1167,104 @@ tasks:
   # -- The maximum number of tasks that can be queued up for execution. If 
unspecified or zero, defaults to Integer.MAX_VALUE.
   # @section -- Tasks
   maxQueuedTasks: 0  # 1000
+
+# @schema type: object
+# -- Configuration for maintenance tasks using as Kubernetes CronJob
+# @section -- Maintenance
+maintenance:
+  # @schema additionalProperties: {type: string}
+  # -- Annotations to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  annotations: {}
+
+  # @schema item: object
+  # -- Extra environment variables to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraEnv: []
+  #  - name: AWS_STORAGE_BUCKET
+  #    value: s3://xxxxx/
+  #  - name: AWS_ACCESS_KEY_ID
+  #    valueFrom:
+  #      secretKeyRef:
+  #        name: aws-secret
+  #        key: access_key_id
+  #  - name: AWS_SECRET_ACCESS_KEY
+  #    valueFrom:
+  #      secretKeyRef:
+  #        name: aws-secret
+  #        key: secret_access_key
+
+  # @schema item: object
+  # -- Bulk import environment variables from Secrets or ConfigMaps to every 
maintenance Cronjob object.
+  # @section -- Maintenance
+  envFrom: []
+  #  - secretRef:
+  #      name: polaris-env-secret
+  #  - configMapRef:
+  #      name: polaris-env-configmap
+
+  # @schema item: object
+  # -- Extra volumes to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraVolumes: []
+    # - name: extra-volume
+    #   emptyDir: {}
+
+  # @schema item: object
+  # -- Extra volume mounts to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraVolumeMounts: []
+    # - name: extra-volume
+    #   mountPath: /usr/share/extra-volume
+
+  # -- The container image used by every maintenance Cronjob object.
+  # @section -- Maintenance
+  image:
+    # -- The image repository to pull from for the Polaris admin tool.
+    # @section -- Maintenance
+    repository: apache/polaris-admin-tool
+    # @schema enum: [Always, IfNotPresent, Never]
+    # -- The image pull policy.
+    # @section -- Maintenance
+    pullPolicy: IfNotPresent
+    # -- The image tag.
+    # @section -- Maintenance
+    tag: "latest"  # This tag will be replaced with the chart version at 
release time.
+
+  # @schema additionalProperties: {type: object, required: [schedule]}

Review Comment:
   ```suggestion
     # @schema additionalProperties: {type: object, required: 
[enabled,schedule]}
   ```



##########
helm/polaris/values.yaml:
##########
@@ -1167,3 +1167,104 @@ tasks:
   # -- The maximum number of tasks that can be queued up for execution. If 
unspecified or zero, defaults to Integer.MAX_VALUE.
   # @section -- Tasks
   maxQueuedTasks: 0  # 1000
+
+# @schema type: object
+# -- Configuration for maintenance tasks using as Kubernetes CronJob
+# @section -- Maintenance
+maintenance:
+  # @schema additionalProperties: {type: string}
+  # -- Annotations to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  annotations: {}
+
+  # @schema item: object
+  # -- Extra environment variables to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraEnv: []
+  #  - name: AWS_STORAGE_BUCKET
+  #    value: s3://xxxxx/
+  #  - name: AWS_ACCESS_KEY_ID
+  #    valueFrom:
+  #      secretKeyRef:
+  #        name: aws-secret
+  #        key: access_key_id
+  #  - name: AWS_SECRET_ACCESS_KEY
+  #    valueFrom:
+  #      secretKeyRef:
+  #        name: aws-secret
+  #        key: secret_access_key
+
+  # @schema item: object
+  # -- Bulk import environment variables from Secrets or ConfigMaps to every 
maintenance Cronjob object.
+  # @section -- Maintenance
+  envFrom: []
+  #  - secretRef:
+  #      name: polaris-env-secret
+  #  - configMapRef:
+  #      name: polaris-env-configmap
+
+  # @schema item: object
+  # -- Extra volumes to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraVolumes: []
+    # - name: extra-volume
+    #   emptyDir: {}
+
+  # @schema item: object
+  # -- Extra volume mounts to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraVolumeMounts: []
+    # - name: extra-volume
+    #   mountPath: /usr/share/extra-volume
+
+  # -- The container image used by every maintenance Cronjob object.
+  # @section -- Maintenance
+  image:
+    # -- The image repository to pull from for the Polaris admin tool.
+    # @section -- Maintenance
+    repository: apache/polaris-admin-tool
+    # @schema enum: [Always, IfNotPresent, Never]
+    # -- The image pull policy.
+    # @section -- Maintenance
+    pullPolicy: IfNotPresent
+    # -- The image tag.
+    # @section -- Maintenance
+    tag: "latest"  # This tag will be replaced with the chart version at 
release time.
+
+  # @schema additionalProperties: {type: object, required: [schedule]}
+  # -- Define maintenance CronJobs. The key is the name of the job.
+  # @section -- Maintenance
+  jobs:
+    nosql-maintenance:
+      # -- Enable this maintenance job.
+      # @section -- Maintenance
+      enabled: false
+      # -- The schedule in Cron format.
+      # @section -- Maintenance
+      schedule: "0 2 * * *"
+      # -- The arguments to pass to the admin tool.
+      # @section -- Maintenance
+      args: ["nosql", "maintenance-run"]
+      # @schema enum: [Allow, Forbid, Replace]
+      # -- The concurrency policy, Valid values are: Allow, Forbid, Replace.
+      # @section -- Maintenance
+      concurrencyPolicy: Forbid
+      # @schema type: object
+      # -- Configures the resources requests and limits for this job's 
container.
+      # @section -- Maintenance
+      resources:

Review Comment:
   Same here, I think users will wonder whether the global `resources` will be 
merged with this or not. Let's document this.



##########
helm/polaris/templates/cronjob-maintenance.yaml:
##########
@@ -0,0 +1,149 @@
+{{/*
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+   http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing,
+  software distributed under the License is distributed on an
+  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+  KIND, either express or implied.  See the License for the
+  specific language governing permissions and limitations
+  under the License.
+*/}}
+
+{{- range $jobName, $jobSpec := .Values.maintenance.jobs }}
+{{- if $jobSpec.enabled }}
+{{- $cronName := printf "%s-maint-%s" (include "polaris.fullname" $) $jobName 
}}

Review Comment:
   We already have a template called `polaris.fullnameWithSuffix`, maybe we can 
reuse that if we add a new parameter `maxLength` (by default 63, and here: 52)?
   
   Also: I wonder if we should drop the `-maint-` segment. Even a moderately 
long release name, e.g. `argocd-us-west2-release` would cross the limits: it 
would generate the name 
`argocd-us-west2-release-polaris-maint-nosql-maintenance` which is 55 chars.
   
   And lastly, I wonder if we should fail hard here when the limit is crossed, 
as opposed to trim. If we fail hard, this means that some pipelines may fail to 
deploy the chart with no simple way for operators to fix the issue, unless they 
can pick a shorter release name.



##########
helm/polaris/templates/cronjob-maintenance.yaml:
##########
@@ -0,0 +1,149 @@
+{{/*
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+   http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing,
+  software distributed under the License is distributed on an
+  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+  KIND, either express or implied.  See the License for the
+  specific language governing permissions and limitations
+  under the License.
+*/}}
+
+{{- range $jobName, $jobSpec := .Values.maintenance.jobs }}
+{{- if $jobSpec.enabled }}
+{{- $cronName := printf "%s-maint-%s" (include "polaris.fullname" $) $jobName 
}}
+{{- if gt (len $cronName) 52 }}
+  {{- fail (printf "Maintenance CronJob name %q is %d chars; must be <= 52 
(K8s appends a timestamp+random suffix to generated Pod names, which are capped 
at 63). Shorten release name, nameOverride/fullnameOverride, or the job key 
%q." $cronName (len $cronName) $jobName) }}
+{{- end }}
+---
+apiVersion: batch/v1
+kind: CronJob
+metadata:
+  name: {{ $cronName }}
+  namespace: {{ $.Release.Namespace }}
+  labels:
+    {{- include "polaris.labels" $ | nindent 4 }}
+  {{- with $.Values.maintenance.annotations }}
+  annotations:
+    {{- tpl (toYaml .) $ | nindent 4 }}
+  {{- end }}
+spec:
+  schedule: {{ $jobSpec.schedule | quote }}
+  concurrencyPolicy: {{ default "Forbid" $jobSpec.concurrencyPolicy }}
+  {{- if hasKey $jobSpec "successfulJobsHistoryLimit" }}
+  successfulJobsHistoryLimit: {{ $jobSpec.successfulJobsHistoryLimit }}
+  {{- end }}
+  {{- if hasKey $jobSpec "failedJobsHistoryLimit" }}
+  failedJobsHistoryLimit: {{ $jobSpec.failedJobsHistoryLimit }}
+  {{- end }}
+  jobTemplate:
+    spec:
+      {{- if hasKey $jobSpec "backoffLimit" }}
+      backoffLimit: {{ $jobSpec.backoffLimit }}
+      {{- end }}
+      {{- with $jobSpec.activeDeadlineSeconds }}
+      activeDeadlineSeconds: {{ . }}
+      {{- end }}
+      template:
+        metadata:
+          labels:
+            helm.sh/chart: {{ include "polaris.chart" $ }}

Review Comment:
   Should we call the `polaris.labels` template here instead? Why do you need 
to customize `app.kubernetes.io/name`?



##########
helm/polaris/values.yaml:
##########
@@ -1167,3 +1167,104 @@ tasks:
   # -- The maximum number of tasks that can be queued up for execution. If 
unspecified or zero, defaults to Integer.MAX_VALUE.
   # @section -- Tasks
   maxQueuedTasks: 0  # 1000
+
+# @schema type: object
+# -- Configuration for maintenance tasks using as Kubernetes CronJob
+# @section -- Maintenance
+maintenance:
+  # @schema additionalProperties: {type: string}
+  # -- Annotations to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  annotations: {}
+
+  # @schema item: object
+  # -- Extra environment variables to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraEnv: []
+  #  - name: AWS_STORAGE_BUCKET
+  #    value: s3://xxxxx/
+  #  - name: AWS_ACCESS_KEY_ID
+  #    valueFrom:
+  #      secretKeyRef:
+  #        name: aws-secret
+  #        key: access_key_id
+  #  - name: AWS_SECRET_ACCESS_KEY
+  #    valueFrom:
+  #      secretKeyRef:
+  #        name: aws-secret
+  #        key: secret_access_key
+
+  # @schema item: object
+  # -- Bulk import environment variables from Secrets or ConfigMaps to every 
maintenance Cronjob object.
+  # @section -- Maintenance
+  envFrom: []
+  #  - secretRef:
+  #      name: polaris-env-secret
+  #  - configMapRef:
+  #      name: polaris-env-configmap
+
+  # @schema item: object
+  # -- Extra volumes to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraVolumes: []
+    # - name: extra-volume
+    #   emptyDir: {}
+
+  # @schema item: object
+  # -- Extra volume mounts to add to every maintenance Cronjob object.
+  # @section -- Maintenance
+  extraVolumeMounts: []
+    # - name: extra-volume
+    #   mountPath: /usr/share/extra-volume
+
+  # -- The container image used by every maintenance Cronjob object.
+  # @section -- Maintenance
+  image:
+    # -- The image repository to pull from for the Polaris admin tool.
+    # @section -- Maintenance
+    repository: apache/polaris-admin-tool
+    # @schema enum: [Always, IfNotPresent, Never]
+    # -- The image pull policy.
+    # @section -- Maintenance
+    pullPolicy: IfNotPresent
+    # -- The image tag.
+    # @section -- Maintenance
+    tag: "latest"  # This tag will be replaced with the chart version at 
release time.
+
+  # @schema additionalProperties: {type: object, required: [schedule]}
+  # -- Define maintenance CronJobs. The key is the name of the job.
+  # @section -- Maintenance
+  jobs:
+    nosql-maintenance:
+      # -- Enable this maintenance job.
+      # @section -- Maintenance
+      enabled: false
+      # -- The schedule in Cron format.
+      # @section -- Maintenance
+      schedule: "0 2 * * *"
+      # -- The arguments to pass to the admin tool.
+      # @section -- Maintenance
+      args: ["nosql", "maintenance-run"]
+      # @schema enum: [Allow, Forbid, Replace]
+      # -- The concurrency policy, Valid values are: Allow, Forbid, Replace.
+      # @section -- Maintenance
+      concurrencyPolicy: Forbid
+      # @schema type: object
+      # -- Configures the resources requests and limits for this job's 
container.
+      # @section -- Maintenance
+      resources:
+        {}
+        # limits:
+        #   cpu: 100m
+        #   memory: 128Mi
+        # requests:
+        #   cpu: 100m
+        #   memory: 128Mi
+      # Optional per-job knobs (uncomment to use):
+      # restartPolicy: Never
+      # backoffLimit: 6
+      # activeDeadlineSeconds: 1800
+      # successfulJobsHistoryLimit: 3
+      # failedjobsHistoryLimit: 1
+      # podAnnotations:

Review Comment:
   1. Same here: users will wonder if `podAnnotations` will be merged with the 
global `podAnnotations`
   2. No `podLabels`?



##########
helm/polaris/templates/cronjob-maintenance.yaml:
##########
@@ -0,0 +1,149 @@
+{{/*
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+   http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing,
+  software distributed under the License is distributed on an
+  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+  KIND, either express or implied.  See the License for the
+  specific language governing permissions and limitations
+  under the License.
+*/}}
+
+{{- range $jobName, $jobSpec := .Values.maintenance.jobs }}
+{{- if $jobSpec.enabled }}
+{{- $cronName := printf "%s-maint-%s" (include "polaris.fullname" $) $jobName 
}}
+{{- if gt (len $cronName) 52 }}
+  {{- fail (printf "Maintenance CronJob name %q is %d chars; must be <= 52 
(K8s appends a timestamp+random suffix to generated Pod names, which are capped 
at 63). Shorten release name, nameOverride/fullnameOverride, or the job key 
%q." $cronName (len $cronName) $jobName) }}
+{{- end }}
+---
+apiVersion: batch/v1
+kind: CronJob
+metadata:
+  name: {{ $cronName }}
+  namespace: {{ $.Release.Namespace }}
+  labels:
+    {{- include "polaris.labels" $ | nindent 4 }}
+  {{- with $.Values.maintenance.annotations }}
+  annotations:
+    {{- tpl (toYaml .) $ | nindent 4 }}
+  {{- end }}
+spec:
+  schedule: {{ $jobSpec.schedule | quote }}
+  concurrencyPolicy: {{ default "Forbid" $jobSpec.concurrencyPolicy }}
+  {{- if hasKey $jobSpec "successfulJobsHistoryLimit" }}
+  successfulJobsHistoryLimit: {{ $jobSpec.successfulJobsHistoryLimit }}
+  {{- end }}
+  {{- if hasKey $jobSpec "failedJobsHistoryLimit" }}
+  failedJobsHistoryLimit: {{ $jobSpec.failedJobsHistoryLimit }}
+  {{- end }}
+  jobTemplate:
+    spec:
+      {{- if hasKey $jobSpec "backoffLimit" }}
+      backoffLimit: {{ $jobSpec.backoffLimit }}
+      {{- end }}
+      {{- with $jobSpec.activeDeadlineSeconds }}
+      activeDeadlineSeconds: {{ . }}
+      {{- end }}
+      template:
+        metadata:
+          labels:
+            helm.sh/chart: {{ include "polaris.chart" $ }}
+            app.kubernetes.io/name: {{ include "polaris.name" $ }}-maintenance
+            app.kubernetes.io/instance: {{ $.Release.Name }}
+            app.kubernetes.io/component: maintenance
+            app.kubernetes.io/managed-by: {{ $.Release.Service }}
+            {{- if $.Chart.AppVersion }}
+            app.kubernetes.io/version: {{ $.Chart.AppVersion | quote }}
+            {{- end }}
+          {{- with $jobSpec.podAnnotations }}
+          annotations:
+            {{- tpl (toYaml .) $ | nindent 12 }}
+          {{- end }}
+        spec:
+          {{- if $.Values.imagePullSecrets }}
+          imagePullSecrets:
+          {{- range $.Values.imagePullSecrets }}
+            - name: {{ . | quote }}
+          {{- end }}
+          {{- end }}
+          restartPolicy: {{ default "Never" $jobSpec.restartPolicy }}
+          {{- with $.Values.priorityClassName }}
+          priorityClassName: {{ tpl . $ | quote }}
+          {{- end }}
+          serviceAccountName: {{ include "polaris.serviceAccountName" $ }}
+          automountServiceAccountToken: false
+          {{- if $.Values.podSecurityContext }}
+          securityContext:
+            {{- tpl (toYaml $.Values.podSecurityContext) $ | nindent 12 }}
+          {{- end }}
+          containers:
+            - name: polaris-admin-tool

Review Comment:
   nit: we don't know what job users can run here, so I'd go with a more 
generic name, e.g. `polaris-job`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to