feat: conditionally enable k8s logs requirements #16
Conversation
matt-march
force-pushed
the
mattmarch/LOG-18839
branch
3 times, most recently
from
5ccf8c1
to
711f44e
Compare
| @@ -80,10 +99,18 @@ spec: | |||
| {{- toYaml .Values.resources | nindent 12 }} | |||
| volumeMounts: | |||
| - name: data | |||
| mountPath: "/vector-data-dir" | |||
| mountPath: "/data/vector" | |||
There was a problem hiding this comment.
Good call making this match the default.
charts/edge/README.md
Outdated
| @@ -40,5 +40,7 @@ helm install edge mezmo/edge \ | |||
| | service.sourcePorts.start | int | 8000 | The start of the port range (inclusive [start, end]) | |||
| | service.sourcePorts.end | int | 8010 | The end of the port range (set 0 or "" to disable port range) | |||
| | service.sourcePorts.list | array[] | [] | Optional list of discrete ports to configure on the service | |||
| | enableK8sLogs | boolean | false | Whether or not to add ClusterRole and Volume access required for k8s logs source | |||
| | pvcClaimName | string | "" | Optional name for an existing PersistentStorageClaim to use for default storage | |||
There was a problem hiding this comment.
nit: I think "pvcClaim" is redundant on the 'c'.
The description I think should say PersistentVolumeClaim.
Might want to mention what this storage is used for (disk buffer, aggregate persistence). (?)
There was a problem hiding this comment.
Might want to mention here or in comments that leaving unspecified uses ephemeral storage. (?)
There was a problem hiding this comment.
Good call, thanks. Fixed the name and added more to the comments
| @@ -71,6 +84,12 @@ spec: | |||
| valueFrom: | |||
| fieldRef: | |||
| fieldPath: metadata.labels['apps.kubernetes.io/pod-index'] | |||
| {{- if .Values.enableK8sLogs }} | |||
| - name: EDGE_SELF_NODE_NAME | |||
There was a problem hiding this comment.
Should we stick with MEZMO_ prefix for these vars, or is there a reason to specify EDGE_ here?
There was a problem hiding this comment.
The var refers to the node that the running instance is on, which is why I was thinking EDGE_ since that's the name of the executing thing. I can update for consistency though if you think that's better
There was a problem hiding this comment.
I don't feel too strongly about it. Just noticed it was the first var where we departed on the brand name. 🤷
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: spec.nodeName | ||
| {{- end }} |
There was a problem hiding this comment.
Re: disk buffer. I was also thinking about adding control values for Buffer sizes:
https://github.com/answerbook/pipeline-service/blob/feature/edge-disk-buffer-2/lib/pipeline/base.js#L619-L631
We could include with PV changes, or just bring those in separately. Or if you don't want to deal with other nit picking I can bring PV in with those after the feature is formally in service main.
There was a problem hiding this comment.
I can add them in. Is it MEZMO_DISK_BUFFER_MAX_BYTES and MEZMO_MEM_BUFFER_MAX_EVENTS or are there others too?
There was a problem hiding this comment.
Thanks. Those are the only two coming so far.
There was a problem hiding this comment.
Added config options for those two
matt-march
force-pushed
the
mattmarch/LOG-18839
branch
from
711f44e
to
3a55c94
Compare
Adds the ability to conditionally enable some role and volume access settings to enable k8s logs as a source Also adds the ability to supply a PersistentVolumeClaim name ref: LOG-18839
matt-march
force-pushed
the
mattmarch/LOG-18839
branch
from
3a55c94
to
b3b680a
Compare
Adds the ability to conditionally enable some role and volume access settings to enable k8s logs as a source
ref: LOG-18839