Remove journald mount by default #1424
Conversation
|
|
||
| // For reading journald | ||
| k.util.hostVolumeMount('etcmachineid', '/etc/machine-id', '/etc/machine-id', readOnly=true), | ||
| //k.util.hostVolumeMount('etcmachineid', '/etc/machine-id', '/etc/machine-id', readOnly=true), |
There was a problem hiding this comment.
I'm not sure about commenting this out, because people still might want it. Can we make it opt-in instead? i.e., logsJournaldMixin or something?
There was a problem hiding this comment.
+1 for making this a parameter, and off by default.
There was a problem hiding this comment.
This also might be a good opportunity to pull the bandaid and migrate everything to the v2 jsonnet, removing the versions. This is already optionally configurable in v2.
There was a problem hiding this comment.
yeah, ok. agreed on pulling the bandaid and migrating to v2 jsonnet.
@rgeyer this will delay delivering pod logs a bit (but not by much). i think it's important as we still want to maintain this non-operator deployment path going forward.
another advantage is we can finally provide a statefulset as the default (and i can quickly write an optional/experimental config to turn on eventhandler that we can provide in the onboarding plugin)
closing this PR
@rfratto i couldn't find how to configure the machine-id vol in the v2 jsonnet (https://github.com/grafana/agent/blob/main/production/tanka/grafana-agent/v2/internal/helpers/logs.libsonnet#L13) - could you point me in the right direction?
github-actions
bot
added
the
frozen-due-to-age
PR Description
Since these sample manifests are mostly used in tandem with the Cloud agent/logs quickstart ConfigMap and soon the K8s integration ConfigMap (which will be even more streamlined), and neither scrapes logs from the journal, I propose omitting the mount and volume from the default manifests entirely.
An alternative solution is changing the
etcmachineidvolume type tofilebut this is cleaner IMO. I prefer reducing the scope of our defaults.Which issue(s) this PR fixes
Fixes #451
Also see #1178
Notes to the Reviewer
PR Checklist