-
Notifications
You must be signed in to change notification settings - Fork 238
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
nfd-topology-updater: update NodeResourceTopology objects directly #980
nfd-topology-updater: update NodeResourceTopology objects directly #980
Conversation
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
/assign @fmuyassarov @ArangoGutierrez |
I will review the code ASAP. Designwise, I'm VERY much in favor of doing the changes directly from nfd-topology-updater instead of passing through nfd-master. I'm really happy this fits the design of NFD. |
Thanks @fromanirh. Basically the code changes are moving code from nfd-master to topology-updater (plus simplifying it as we don't need much data conversions anymore), dropping NFD gRPC client bits from topology-updater and the dropping the NFD topology-updater gRPC api, plus then adjusting deployment, docs and tests to these changes. On the TODO-list there is refactoring the
Yeah, we had the idea to have all K8s api comms in nfd-master. That was never the case for topology-updater and in retrospect that doesn't look that sensible 😊 It probably makes sense for Node objects but not for CRs. |
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.
A couple of notes from me.
In my opinion, this is a blessed change that simplifies the design.
deployment/base/topologyupdater-daemonset/topologyupdater-daemonset.yaml
Show resolved
Hide resolved
3b850e8
to
7a5bae8
Compare
7a5bae8
to
f58a88a
Compare
Thanks @Tal-or for the review. Review feedback responded/addressed. |
/retest |
Thanks. LGTM |
PING @fromanirh |
sure thing, reviewing today December 6, 2022 |
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.
/lgtm
thanks!
Rebased |
f58a88a
to
cdca66b
Compare
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.
Looks good apart from minor comments in line.
Drop the gRPC communication to nfd-master and connect to the Kubernetes API server directly when updating NodeResourceTopology objects. Topology-updater already has connection to the API server for listing Pods so this is not that dramatic change. It also simplifies the code a lot as there is no need for the NFD gRPC client and no need for managing TLS certs/keys. This change aligns nfd-topology-updater with the future direction of nfd-worker where the gRPC API is being dropped and replaced by a CRD-based API. This patch also update deployment files and documentation to reflect this change.
cdca66b
to
f13ed2d
Compare
Thanks @fmuyassarov for the review. Comments addressed. PTAL. |
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.
I'm not an expert on this but code looks good to me and I see other folks happy with the change.
/lgtm
Drop the gRPC communication to nfd-master and connect to the Kubernetes API server directly when updating NodeResourceTopology objects. Topology-updater already has connection to the API server for listing Pods so this is not that dramatic change. It also simplifies the code a lot as there is no need for the NFD gRPC client and no need for managing TLS certs/keys.
This change aligns nfd-topology-updater with the future direction of nfd-worker where the gRPC API is being dropped and replaced by a CRD-based API.
This patch also update deployment files and documentation to reflect this change.
TODO:: refactor the
pkg/nfd-client
package topkg/nfd-worker
nfd/tnfd-topology-updater
but I think that can be made in a separate PR (and make reviewing easier).