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 1 commit
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
11 changes: 11 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 }}
volumeMounts:
{{- range .Values.core.config.eventTemplates.configMapNames }}
- name: {{ . }}
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,8 @@ spec:
secret:
secretName: {{ .Release.Name }}-proxy-tls
{{- end }}
{{- range .Values.core.config.eventTemplates.configMapNames}}
- name: {{ . }}
configMap:
Copy link
Member

Choose a reason for hiding this comment

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

Also, should we enforce that these configmaps are available at the time of installation (i.e. optional: false)?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a fair point.

Copy link
Member Author

Choose a reason for hiding this comment

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

From what I can tell, I think optional: false is the default (which makes sense), but I can mark them as explicitly non-optional.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, sounds good!

name: {{ . }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

Any thoughts on using a projected volume here to combine all config-map contents into a single volume? One benefit is that we can define a single volumeMount on /opt/cryostat.d/templates.d, thus reducing the manifest/object def size.

Reference: https://kubernetes.io/docs/concepts/storage/projected-volumes/

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I could do this. It would be a single mount but the definition for the projected volume itself would still look about the same, so I'm not really sure I understand what benefit it brings... ?

Copy link
Member

Choose a reason for hiding this comment

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

hm, I suppose reducing the number of volume mount definitions can help reduce the manifest size (thus, object size in etcd). And grouping them under a single volume seems to make it easier to recognize their purpose when reading the manifest (my opinion).

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can do the same for #219.

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: a
mountPath: /opt/cryostat.d/templates.d/a
readOnly: true
- name: b
mountPath: /opt/cryostat.d/templates.d/b
readOnly: true
- equal:
path: spec.template.spec.volumes
value:
- name: alpha-config
configMap:
name: RELEASE-NAME-alpha-config
- name: a
configMap:
name: a
- name: b
configMap:
name: b
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
Loading