nfd-topology-updater: retrieve kubelet config from API /configz

[Garrybest]

Now nfd-topology-updater tries to retrieve topologyManagerPolicy of a node by reading kubelet config file. However, sometimes we use command line to start Kubelet instead of the config file, or sometimes the config file is modified but Kubelet has not restarted to make this file take effect.

I think we'd better retrieve the latest config from Kubelet API /configz, Kubelet will return the configuration in its memory. This config is obviously accurate.

[k8s-ci-robot]

Welcome @Garrybest!

It looks like this is your first PR to kubernetes-sigs/node-feature-discovery 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/node-feature-discovery has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @Garrybest. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Garrybest]

/cc @swatisehgal @fromanirh

[k8s-ci-robot]

@Garrybest: GitHub didn't allow me to request PR reviews from the following users: fromanirh.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @swatisehgal @fromanirh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Garrybest]

I have tested the whole deployment in my own minikube, the logs show this API would works.

I0702 12:03:12.757291       1 main.go:74] detected kubelet Topology Manager policy "SingleNUMANodeContainerLevel"

[ffromani]

Thanks for this PR! this is a interesting direction.
We tried an approach like this some time ago but we decided not to purse it. I'm in favour of this general direction though, so I'll do some archeology to see if the reasons why we stopped are still relevant or not.

[ffromani]

/cc @Tal-or

[k8s-ci-robot]

@fromanirh: GitHub didn't allow me to request PR reviews from the following users: Tal-or.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @Tal-or

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ffromani]

@Tal-or

[Tal-or]

Very nice.
I don't recall atm why eventually we decided not to move on in this direction, I need to do some digging to find out why.
It would be great if we can have some integration/e2e as part of this PR for validating this feature.

[Tal-or]

There is some note in K8S docs:
https://v1-23.docs.kubernetes.io/docs/tasks/administer-cluster/reconfigure-kubelet/#generating-a-file-that-contains-the-current-configuration

which says:
Caution: The kubelet's configz endpoint is there to help with debugging, and is not a stable part of kubelet behavior. Do not rely on the behavior of this endpoint for production scenarios or for use with automated tools.

This is the reason why we decided to not pursue this direction eventually.
Maybe this warning isn't relevant anymore but this is something that should be figured out.

[Garrybest]

There is some note in K8S docs: https://v1-23.docs.kubernetes.io/docs/tasks/administer-cluster/reconfigure-kubelet/#generating-a-file-that-contains-the-current-configuration

which says: Caution: The kubelet's configz endpoint is there to help with debugging, and is not a stable part of kubelet behavior. Do not rely on the behavior of this endpoint for production scenarios or for use with automated tools.

This is the reason why we decided to not pursue this direction eventually. Maybe this warning isn't relevant anymore but this is something that should be figured out.

Thanks for reminding. Now I try to use token first. If failed, we could fall back to use config file.

[marquiz]

/ok-to-test

[marquiz]

Thanks @Garrybest for the PR. I think this makes sense.

I'm sorry I didn't have the time to review the PR this week and now I'm off to summer holidays 🙄 I will be off for four weeks but will reivew this when I'm back.

I'm not entirely sure about the fallback 🧐 How about changing -kubelet-config-file to -kubelet-config and you could the give a http:// endpoint or a file://pointing to a local file?

[ffromani]

I'm not entirely sure about the fallback monocle_face How about changing -kubelet-config-file to -kubelet-config and you could the give a http:// endpoint or a file://pointing to a local file?

I like this idea!

[Garrybest]

/retest

[Garrybest]

Thanks @marquiz, I like this suggestion. Very cool 😄

[marquiz]

Thanks for the update @Garrybest! A few mpre comments but nothing bug

[Garrybest]

Thanks for the suggestions!

[marquiz]

Just two more small nits and I would be good to go with this

[Garrybest]

/retest

[Garrybest]

It seems that there is something wrong with the robot 🤣

[marquiz]

Thanks for working on this and the quick responses @Garrybest 😊 I think we can merge this but I'll give some time to others chime in, too

There really seems to be something odd in prow...
/retest

[Garrybest]

I think we can merge this but I'll give some time to others chime in, too

No problem. Thanks again for your rigorous reviewing 😄

[marquiz]

/retest

[ffromani]

Thanks for working on this and the quick responses @Garrybest blush I think we can merge this but I'll give some time to others chime in, too

There really seems to be something odd in prow... /retest

Thanks for this! I don't have any additional comments.

[marquiz]

Thanks for this! I don't have any additional comments.

👍
/assign @fmuyassarov

prow had some issues so please fix those

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Garrybest, 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]

@fmuyassarov @zvonkok you wanna check this or should we just merge?

[fmuyassarov]

@fmuyassarov @zvonkok you wanna check this or should we just merge?

I wanted to have a look. Will review it in a couple of hours today.

[fmuyassarov]

/lgtm
Thanks