Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(eventtemplates): declarative configuration of Event Template ConfigMaps #215

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat(eventtemplates): declarative configuration of Event Template ConfigMaps #215

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
71cb0f8
feat(eventtemplates): declarative configuration of Event Template Con…
andrewazores Nov 27, 2024
871ae55
explicitly non-optional
andrewazores Dec 2, 2024
3f0dbc8
projected volume
andrewazores Dec 2, 2024
ad25e1c
fs mode
andrewazores Dec 4, 2024
cdacc38
readme
andrewazores Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions charts/cryostat/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ helm install cryostat ./charts/cryostat
| `core.discovery.kubernetes.portNames` | List of port names that the Cryostat application should look for in order to consider a target as JMX connectable | `[]` |
| `core.discovery.kubernetes.builtInPortNumbersDisabled` | When false and `portNumbers` is empty, the Cryostat application will use the default port number `9091` to look for JMX connectable targets. | `false` |
| `core.discovery.kubernetes.portNumbers` | List of port numbers that the Cryostat application should look for in order to consider a target as JMX connectable | `[]` |
| `core.config.eventTemplates.configMapNames` | List of ConfigMap names. Each ConfigMap is expected to contain one or more files, which are .jfc (XML) JFR Event Templates, to be mounted to the Cryostat container. | `[]` |

### Report Generator Deployment

Expand Down
14 changes: 14 additions & 0 deletions charts/cryostat/templates/cryostat_deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,12 @@ spec:
failureThreshold: 18
resources:
{{- toYaml .Values.core.resources | nindent 12 }}
{{- if .Values.core.config.eventTemplates.configMapNames}}
volumeMounts:
- name: declarative-event-templates
mountPath: /opt/cryostat.d/templates.d
readOnly: true
{{- end }}
- name: {{ printf "%s-%s" .Chart.Name "grafana" }}
securityContext:
{{- toYaml .Values.grafana.securityContext | nindent 12 }}
Expand Down Expand Up @@ -216,3 +222,11 @@ spec:
secret:
secretName: {{ .Release.Name }}-proxy-tls
{{- end }}
- name: declarative-event-templates
Copy link
Member

@tthvo tthvo Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we set the defaultMode to read-only (i.e. 0440)?

I think we should be okayy to set 0440 without running into #225 (comment), since Cryostat has default GID 0 in the image.

projected:
sources:
{{- range .Values.core.config.eventTemplates.configMapNames}}
- configMap:
name: {{ . }}
optional: false
{{- end }}
27 changes: 27 additions & 0 deletions charts/cryostat/tests/cryostat_deployment_test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,8 @@ tests:
requests:
cpu: 500m
memory: 384Mi
- notExists:
path: spec.template.spec.contains[?(@.name=='cryostat')].volumeMounts

- it: should set log level
set:
Expand Down Expand Up @@ -514,3 +516,28 @@ tests:
path: spec.template.spec.containers[?(@.name=='cryostat-jfr-datasource')].imagePullPolicy
value: "IfNotPresent"

- it: should add volume mounts for declarative event templates
set:
core.config.eventTemplates.configMapNames: ['a', 'b']
asserts:
- equal:
path: spec.template.spec.containers[?(@.name=='cryostat')].volumeMounts
value:
- name: declarative-event-templates
mountPath: /opt/cryostat.d/templates.d
readOnly: true
- equal:
path: spec.template.spec.volumes
value:
- name: alpha-config
configMap:
name: RELEASE-NAME-alpha-config
- name: declarative-event-templates
projected:
sources:
- configMap:
name: a
optional: false
- configMap:
name: b
optional: false
16 changes: 16 additions & 0 deletions charts/cryostat/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,22 @@
}
}
}
},
"config": {
"type": "object",
"properties": {
"eventTemplates": {
"type": "object",
"properties": {
"configMapNames": {
"type": "array",
"description": "List of ConfigMap names. Each ConfigMap is expected to contain one or more files, which are .jfc (XML) JFR Event Templates, to be mounted to the Cryostat container.",
"default": [],
"items": {}
}
}
}
}
}
}
},
Expand Down
4 changes: 4 additions & 0 deletions charts/cryostat/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@ core:
builtInPortNumbersDisabled: false
## @param core.discovery.kubernetes.portNumbers [array] List of port numbers that the Cryostat application should look for in order to consider a target as JMX connectable
portNumbers: []
config:
eventTemplates:
## @param core.config.eventTemplates.configMapNames [array] List of ConfigMap names. Each ConfigMap is expected to contain one or more files, which are .jfc (XML) JFR Event Templates, to be mounted to the Cryostat container.
configMapNames: []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we can allow specifying the key(s) within the configmaps? Something similar to Cryostat CR.

I guess one case could be that a user uses a Configmap to put all Cryostat configurations (i.e. Event template and Automated Rules). I might be overthinking :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but it seems like it'd be pretty messy trying to put all of the configurations of different types into one ConfigMap and use filenames to differentiate them. It would also much more quickly run into the ConfigMap size limit that way. I think using one or more ConfigMaps for each type of configuration file is a good medium option, where if there are many custom Event Templates that add up to too large a filesize then they can be split across multiple ConfigMaps, for example, but otherwise they can just be combined together as separate files and mounted that way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that makes sense! I think the current way is good then^^


## @section Report Generator Deployment
## @extra reports Configuration for the Reports Generator deployment
Expand Down