source/custom: dump config in more human-readable form

[marquiz]

You get log entries like this (with -v 1):

I0310 21:59:19.778694   12976 custom.go:84] custom features configuration:
I0310 21:59:19.778963   12976 custom.go:84]   - matchOn:
I0310 21:59:19.778981   12976 custom.go:84]     - pciId:
I0310 21:59:19.778985   12976 custom.go:84]         vendor:
I0310 21:59:19.778988   12976 custom.go:84]         - 15b3
I0310 21:59:19.778991   12976 custom.go:84]     name: rdma.capable
I0310 21:59:19.778994   12976 custom.go:84]   - matchOn:
I0310 21:59:19.778997   12976 custom.go:84]     - loadedKMod:
I0310 21:59:19.779000   12976 custom.go:84]       - ib_uverbs
I0310 21:59:19.779004   12976 custom.go:84]       - rdma_ucm
I0310 21:59:19.779009   12976 custom.go:84]     name: rdma.available
I0310 21:59:19.779015   12976 custom.go:84]   - matchOn:
I0310 21:59:19.779018   12976 custom.go:84]     - kConfig:
I0310 21:59:19.779023   12976 custom.go:84]       - X86
I0310 21:59:19.779026   12976 custom.go:84]     name: kconfig-test

[marquiz]

For development this is nice. Dunno about production 🤔
/hold

[ArangoGutierrez]

For development this is nice. Dunno about production thinking
/hold

maybe -v 2 ? a debug lvl log is always helpful. I like it. sometimes we need to know what the users did. although we could jjust check the configMap.
I would say -v 2 is a safe bet

[marquiz]

maybe -v 2 ? a debug lvl log is always helpful. I like it. sometimes we need to know what the users did. although we could jjust check the configMap.
I would say -v 2 is a safe bet

What I was merely pondering was the oneline vs multi-line format. Multi-line is not that friendly for external log servers (I think). There would be some options to tackle this if we . One would be to dump a oneliner with -v 1 and multi-line with v -2. Another solution would be to have another config option for controlling this

[ArangoGutierrez]

maybe -v 2 ? a debug lvl log is always helpful. I like it. sometimes we need to know what the users did. although we could jjust check the configMap.
I would say -v 2 is a safe bet

What I was merely pondering was the oneline vs multi-line format. Multi-line is not that friendly for external log servers (I think). There would be some options to tackle this if we . One would be to dump a oneliner with -v 1 and multi-line with v -2. Another solution would be to have another config option for controlling this

What about a nfd-worker dump command, I like this patch main idea, bc I think when I am in the pod doing some debugging , so why not to have a way to simple call the binary and retrieve the config and maybe other info that can help to fix a problem

[marquiz]

What about a nfd-worker dump command

Why not, something like that would probably be useful. But, I think that's a separate issue from this one (Could you open an issue about that?)

This PR is about making the existing debug print more readable.

[ArangoGutierrez]

What about a nfd-worker dump command

Why not, something like that would probably be useful. But, I think that's a separate issue from this one (Could you open an issue about that?)

This PR is about making the existing debug print more readable.

my concern with this PR is that the custom config is a config map, I don't see the value of printing it on the logs, if you can kubectl get cm/blablabla -o yaml or inside the pod go to the mounted path and read it.
Printing a custom config would make logs really long and hard to read, more when working on a remote terminal or something.
Not so sure we want this many logs
Any thoughts other reviewers?

[marquiz]

my concern with this PR is that the custom config is a config map,

It's not just a configmap. It's composed of several sources: the static rules, main config file and extra rules under /etc/kubernetes/node-feature-discovery/custom.d.

Printing a custom config would make logs really long and hard to read, more when working on a remote terminal or something.
Not so sure we want this many logs

This is more of a debugging aid (and should probably be -v 2). I think it is useful to be able to see the complete list of rules that are present. That said, I also understand the viewpoint that this log message should be removed completely. Then there's the option I mentioned about multi-line vs. single-line (json?) dumps 🤔

[marquiz]

Rebased and increased log level to 2.

[ArangoGutierrez]

As level 2 I like this idea
looks good to me (lgtm)
WDYT @Ethyling

[jjacobelli]

WDYT @Ethyling

LGTM
Should we merge it for the v0.8 release?

[ArangoGutierrez]

WDYT @Ethyling

LGTM
Should we merge it for the v0.8 release?

I am ok with it being in the release, more since we are getting the core-config feature, debugging NFD in prod systems, this will help
Thoughts
@marquiz

[ArangoGutierrez]

Since 2 reviewers already gave lgtm, I am creating the flag but not removing the hold label
Leaving the unhold to Marquiz
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, marquiz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [marquiz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[marquiz]

Thanks for the reviews! I think we can merge this, especially as it's -v 2 and far from enabled by default
/unhold