-
Notifications
You must be signed in to change notification settings - Fork 23
feat: support filter list #264
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.
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Jack Yu <[email protected]>
Signed-off-by: Jack Yu <[email protected]>
Signed-off-by: Jack Yu <[email protected]>
Signed-off-by: Jack Yu <[email protected]>
Signed-off-by: Jack Yu <[email protected]>
Signed-off-by: Jack Yu <[email protected]>
Signed-off-by: Jack Yu <[email protected]>
Signed-off-by: Jack Yu <[email protected]>
Signed-off-by: Jack Yu <[email protected]>
Signed-off-by: Jack Yu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds ConfigMap-backed filter/autoprovision configuration (with fallback to env vars) and exposes filter “Details” for debug logging, enabling easier per-node disk selection without relying solely on pod env configuration.
Changes:
- Introduces
ConfigMapLoaderto load/mergefilters.yamlandautoprovision.yamlconfigs (with hostname matching). - Updates scanner to reload config on each scan and emit detailed debug output for active filters.
- Adds Helm chart ConfigMap template/values and unit tests for loader behavior.
Reviewed changes
Copilot reviewed 16 out of 303 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/fake/configmap.go | Adds a fake ConfigMap client adapter for tests. |
| pkg/filter/vendor_filter.go | Adds Details() output for vendor filters. |
| pkg/filter/path_filter.go | Adds Details() output for mount path filters. |
| pkg/filter/part_type_filter.go | Adds Details() output for partition type filters. |
| pkg/filter/label_filter.go | Adds Details() output for label filters. |
| pkg/filter/filter.go | Extends filter interfaces with Details() and adds device-path exclusion support. |
| pkg/filter/drive_type_filter.go | Adds Details() output for drive type filters. |
| pkg/filter/device_path_filter.go | Adds Details() output for device path filters. |
| pkg/filter/configmap_loader.go | Implements ConfigMap loading/parsing/merge logic for filters and autoprovision. |
| pkg/filter/configmap_loader_test.go | Adds unit tests for ConfigMapLoader behavior across multiple scenarios. |
| pkg/controller/blockdevice/scanner.go | Reloads config each scan and logs final filter configuration; threads ctx through scan. |
| pkg/controller/blockdevice/controller.go | Updates scanner start call to pass context. |
| cmd/node-disk-manager/main.go | Wires ConfigMapLoader into scanner creation and removes direct env-filter wiring. |
| deploy/charts/harvester-node-disk-manager/values.yaml | Adds chart values structure for ConfigMap-backed configuration. |
| deploy/charts/harvester-node-disk-manager/templates/configmap.yaml | Adds chart template to render filters/autoprovision data into a ConfigMap. |
| go.mod | Adds direct dependency on gopkg.in/yaml.v3 for YAML parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
deploy/charts/harvester-node-disk-manager/templates/configmap.yaml
Outdated
Show resolved
Hide resolved
deploy/charts/harvester-node-disk-manager/templates/configmap.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: Jack Yu <[email protected]>
Signed-off-by: Jack Yu <[email protected]>
| func addNDMConfigMap(clientset *kubernetes.Clientset) error { | ||
| configMap := &corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: ConfigMapName, | ||
| Namespace: ConfigMapNamespace, | ||
| }, | ||
| Data: map[string]string{ | ||
| "filters.yaml": `- hostname: "*" | ||
| excludeLabels: ["COS_*", "HARV_*"] | ||
| `, | ||
| "autoprovision.yaml": "", | ||
| }, | ||
| } | ||
|
|
||
| _, err := clientset.CoreV1().ConfigMaps(ConfigMapNamespace).Create(context.TODO(), configMap, metav1.CreateOptions{}) | ||
| if err != nil && !apierrors.IsAlreadyExists(err) { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I put it here is about managedChart issue (harvester/harvester#9522 (comment)).
Problem:
We only support filter and auto-provision from pod env, which isn't easy to use.
Solution:
I added config map structure and the following features:
Adde device filter, which we didn't have before.
Loading config map to filter the blockdevice and select candidates for auto provision.
Add creating a default config map resource under pkg/data. (reason)
Set exclude filter and auto provision every time when scanner starts. It's useful when config map is changed. But, I don't think we need to notify scanner to scan again when the config map is changed. Just one way direction is enough.
Fallback mechanism. If config map doesn't exist, it directly uses the env variables. This is just for the upgrade if the upgrade fails, which causes some unexpected behaviors.
Added debug message like this. We have some default settings so it's better to have a clear message to check.
p.s. I don't change too much logic inside the scanner, I'd like to keep it simple, so I just reuse it and change the interface of accepting the filters from the outside.
Out of the scope
Related Issue:
harvester/harvester#5059
Test plan:
This is an example testing. There are too many test cases. I'll cover as many as possible in our unit tests.
harvester1node["COS_*, HARV_*"]["longhorn", "thisisaexample", "harvester1"]["/dev/sdd"]["/dev/sdc", "/dev/sdd"]harvester2node["COS_*, HARV_*"]["longhorn", "thisisaexample", "harvester2"]["/dev/sdd"]["/dev/sdc", "/dev/sdd"]In the end,
/dev/sddis skipped because the of exclusion although it's in auto-provision list.