optional pod selector labels #98

tanmaykm · 2022-06-23T05:16:57Z

This allows enabling core-dump-handler only on pods that match a configured selector pod-label. Useful if we need core-dump-handler only for a subset of our workload. The composer checks for the pod label and only prepares the upload only if the pod matches the selector configured. The default (empty selector label) is to have no selectors and enable collection for all. Only the existence of the label is checked for.

The selector can be configured via daemonSet.podSelectorLabel in values.yaml, which translates to the POD_SELECTOR_LABEL environment variable in the daemonset.

No9 · 2022-06-23T07:16:27Z

Thanks for this as well.
In principle this is a great feature but I need to map the specifics to the cloudevent work going on in this branch.
https://github.com/IBM/core-dump-handler/tree/cloud-event
I want to keep the labeling for the different features consistent. It might be able to land 'as-is' or it may make sense to define a specific label.
I'm busy today but I'll provide guidance before the weekend.
If you could apply the rustfmt fix I'd appreciate it.

tanmaykm · 2022-06-23T07:31:49Z

Thanks for looking into it!
Added 89010c6 to fix ci and rustfmt

This allows enabling core-dump-handler only on pods that match a configured selector pod-label. Useful if we need core-dump-handler only for a subset of our workload. The composer checks for the pod label and only prepares the upload only if the pod matches the selector configured. The default (empty selector label) is to have no selectors and enable collection for all. Only the existence of the label is checked for. The selector can be configured via `daemonSet.podSelectorLabel` in `values.yaml`, which translates to the `POD_SELECTOR_LABEL` environment variable in the daemonset.

No9 · 2022-06-26T09:40:54Z

charts/core-dump-handler/README.md

@@ -214,6 +214,7 @@ The agent pod has the following environment variables and these are all set by t
 * INTERVAL - The amount of time in milliseconds between each check of the core dump folder for files to upload.
 * SCHEDULE - A CRON formatted string [See cron library](https://github.com/mvniekerk/tokio-cron-scheduler#usage).
 * USE_INOTIFY - Set a listener for the coredump folder can be used in conjunction with SCHEDULE
+* POD_SELECTOR_LABEL - Optional selector label to filter pods that have core dump collection enabled. Default (empty) disables filter.


As per conversation this should be COMP_POD_SELECTOR_LABEL
Also add an an example label so it's clear how this works

No9 · 2022-06-26T09:41:28Z

charts/core-dump-handler/ci/inotify-manage-store.yaml

@@ -33,6 +33,7 @@ daemonset:
  hostDirectory: "/var/mnt/core-dump-handler"
  coreDirectory: "/var/mnt/core-dump-handler/cores"
  crioEndpoint: "unix:///run/containerd/containerd.sock"
+  podSelectorLabel: ""


As per conversation should be in composer not daemonset

No9 · 2022-06-26T09:41:38Z

charts/core-dump-handler/ci/interval-manage-store.yaml

@@ -33,6 +33,7 @@ daemonset:
  hostDirectory: "/var/mnt/core-dump-handler"
  coreDirectory: "/var/mnt/core-dump-handler/cores"
  crioEndpoint: "unix:///run/containerd/containerd.sock"
+  podSelectorLabel: ""


As per conversation should be in composer not daemonset

No9 · 2022-06-26T09:41:47Z

charts/core-dump-handler/ci/schedule-no-manage-store.yaml

@@ -33,6 +33,7 @@ daemonset:
  hostDirectory: "/var/mnt/core-dump-handler"
  coreDirectory: "/var/mnt/core-dump-handler/cores"
  crioEndpoint: "unix:///run/containerd/containerd.sock"
+  podSelectorLabel: ""


As per conversation should be in composer not daemonset

No9 · 2022-06-26T09:41:55Z

charts/core-dump-handler/ci/tolerations.yaml

@@ -34,6 +34,7 @@ daemonset:
  hostDirectory: "/var/mnt/core-dump-handler"
  coreDirectory: "/var/mnt/core-dump-handler/cores"
  crioEndpoint: "unix:///run/containerd/containerd.sock"
+  podSelectorLabel: ""


As per conversation should be in composer not daemonset

No9 · 2022-06-26T09:42:54Z

charts/core-dump-handler/templates/daemonset.yaml

@@ -45,6 +45,8 @@ spec:
            value:  {{ .Values.daemonset.deployCrioConfig | quote }}
          - name: CRIO_ENDPOINT
            value:  {{ .Values.daemonset.crioEndpoint | quote }}
+          - name: POD_SELECTOR_LABEL


Should have COMP_ prefix

No9 · 2022-06-26T09:43:13Z

charts/core-dump-handler/values.schema.json

@@ -205,6 +205,9 @@
                "crioEndpoint": {
                    "type": "string"
                },
+                "podSelectorLabel": {
+                    "type": "string"


Should move to composer definition

No9 · 2022-06-26T09:43:35Z

charts/core-dump-handler/values.yaml

@@ -33,6 +33,7 @@ daemonset:
  hostDirectory: "/var/mnt/core-dump-handler"
  coreDirectory: "/var/mnt/core-dump-handler/cores"
  crioEndpoint: "unix:///run/containerd/containerd.sock"
+  podSelectorLabel: ""


As per conversation should be in composer not daemonset

No9 · 2022-06-26T09:47:14Z

The good news is that this doesn't interfere with the cloud events.

The bad news is that the pod selector config mapping isn't quite in the right place.

All config that is passed to the composer should be in the composer section
https://github.com/IBM/core-dump-handler/blob/main/charts/core-dump-handler/values.yaml#L23
The values should map to an environment variable prefixed with COMP_ in the agent.

Could you add an example of a label value in the Environment config
https://github.com/IBM/core-dump-handler/pull/98/files#diff-35330df803a4bbfce6b8b734490d5f0b77bb117755aeac1958500a4934a86bb8R217

The values section in the docs also needs to be updated along with the environment section.
https://github.com/IBM/core-dump-handler/pull/98/files#diff-35330df803a4bbfce6b8b734490d5f0b77bb117755aeac1958500a4934a86bb8R248

I've gone through the PR to also highlight where I think these changes need to be done.
Hope it isn't too noisy.

(Edit) Once the mapping is aligned I'm happy to merge and push a release.

tanmaykm · 2022-06-27T03:31:00Z

I have now added 1186de2 which hopefully addresses these. Thanks for the guidance!

No9 · 2022-06-29T21:23:26Z

Looks good - I have some dep updates to land then I'll put together a release

tanmaykm added 2 commits June 24, 2022 06:29

fix ci and rustfmt

abba8f9

tanmaykm force-pushed the tan/podselector branch from 89010c6 to abba8f9 Compare June 24, 2022 01:00

No9 reviewed Jun 26, 2022

View reviewed changes

move podselector config to composer section

1186de2

No9 merged commit a1ccae1 into IBM:main Jun 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optional pod selector labels #98

optional pod selector labels #98

tanmaykm commented Jun 23, 2022

No9 commented Jun 23, 2022

tanmaykm commented Jun 23, 2022

No9 Jun 26, 2022

No9 Jun 26, 2022

No9 Jun 26, 2022

No9 Jun 26, 2022

No9 Jun 26, 2022

No9 Jun 26, 2022

No9 Jun 26, 2022

No9 Jun 26, 2022

No9 commented Jun 26, 2022 •

edited

Loading

tanmaykm commented Jun 27, 2022

No9 commented Jun 29, 2022

optional pod selector labels #98

optional pod selector labels #98

Conversation

tanmaykm commented Jun 23, 2022

No9 commented Jun 23, 2022

tanmaykm commented Jun 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

No9 commented Jun 26, 2022 • edited Loading

tanmaykm commented Jun 27, 2022

No9 commented Jun 29, 2022

No9 commented Jun 26, 2022 •

edited

Loading