search

kubernetes-sigs / node-feature-discovery

Node feature discovery for Kubernetes
Apache License 2.0
775 stars 240 forks source link

Exposing Hardware Topology through CRDs in Node feature discovery #333

Closed swatisehgal closed 1 year ago

swatisehgal commented 4 years ago

Currently Node feature Discovery consists of nfd-master and nfd-worker. The former is responsible for labeling Kubernetes node objects and the latter is responsible for detecting features and communicating them to nfd-master. nfd-worker runs as a daemon-set on all the nodes in the cluster.

Resource Topology Exporter (KEP, Code) can nicely fit into NFD architecture by:

Design document explaining the approach and proposed changes are provided in detail here.

Possible implementation approaches of introducing Resource Topology Exporter in NFD operand are summarized below:

  1. Option 1:Introducing Resource Topology Exporter as a source or a new command line option in the nfd-worker. New source (or new command line option) is where nfd-worker would gather hardware topology information. Nfd-master would update NumaTopology CRD

  2. Option 2 (preferred): A new helper daemon eg nfd-node-topology (similar to nfd-worker).

Changes to the gRPC interface: Introducing another gRPC endpoint for communication between nfd-worker and nfd-master. (completely separate proto as shown below)

syntax = "proto3";

option go_package = "sigs.k8s.io/node-feature-discovery/pkg/topologyupdater";
import "github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/apis/topology/v1alpha1/generated.proto";

package topologyupdater;

service NodeTopology{
    rpc UpdateNodeTopology(NodeTopologyRequest) returns (NodeTopologyResponse);
}
message NodeTopologyRequest {
    string nfd_version = 1;
    string node_name = 2;
    repeated string topology_policies = 3;
    repeated v1alpha1.Zone zones = 4;
}
message NodeTopologyResponse {
}

Additionally, we propose that NFD becomes home for NodeResourceTopology CRD API definition (and informer, clientset, handlers etc)

marquiz commented 4 years ago

I'd suggest to use a more generic name for the package, "cr-updater" or something.

Regarding the data/CRD, I was pondering should we prepare for more detailed information, e.g. bake in or make it possible to present numa distances(?)

swatisehgal commented 4 years ago

@marquiz Sure, we can make the package name more generic. Can you elaborate on how do you see information related to numa distances being exposed here? In order to gather resource information out of tree we are currently relying on podResource API. As our only source of truth for obtaining device information is (device manager in) kubelet, we have device information in kubelet (health, device ids and numa node as part of topologyInfo) but need to extend podResource API (as proposed here: kubernetes/enhancements#1884) . Numa distances cannot be obtained from kubelet as it is not supported currently in kubelet.

As per the design of topology manager, cpu manager and device manager implement the hint providers interface and provide numa node id as a hint to topology manager. We are open to suggestion here, but as a starting point we could expose the constructs in terms of numa node ids (as it aligns with the current design of topology manager) and more importantly that's the only information at our disposal currently. We would be more than happy to update later when topology information becomes more descriptive in topology manager as the API itself would evolve and would allow us to expose this information. Let me know your thoughts.

ffromani commented 4 years ago

A way we can expose the numa distance could probably be like

message NUMANodeResource {
    int numa_id = 1;
    map<string, string> resources = 2;
    map<int, int> distances = 3;
}

However I second what @swatisehgal wrote, the thing is we were using the podresources API - and the extensions we were proposing here - as source of truth. However, if there is interest specifically about the numa distances we can maybe learn them reliably from sysfs, kinda like numactl does. That should be pretty safe to reconcile. It could require access to more system resources though.

swatisehgal commented 4 years ago

Thanks @fromanirh for this code snippet. Getting numa node distance from sysfs is totally doable and populating this as part of NUMANodeResource makes sense! I was considering numa node distance from the point of view of resources and their distance from each other (in my previous comment).

marquiz commented 4 years ago

I don't have the answers myself, either, just throwing some ideas 😊 I was just trying to think a bit ahead and broader and not "cement" this too tightly just for this one use case and scheduler extension. When we add something we should at least try to think about possible other users and usages, too, e.g. some other scheduler extensions and possibly alternative rte daemons digging more detailed data. Just trying to avoid the situation where we quickly patch together the api for one narrow use case and after a while we're stuck with that and have problems serving new users. This case is simple, but still. Maybe we have something to learn from how the Linux kernel handles apis 😄

swatisehgal commented 4 years ago

@marquiz Thanks for the input. I see your point and have updated the design document to include NumaNode distances as part of the NumaNodeResource structure

fejta-bot commented 3 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

swatisehgal commented 3 years ago

/remove-lifecycle stale

swatisehgal commented 3 years ago

Development work is in progress to support this feature. PR will be linked here soon!

fejta-bot commented 3 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale

zvonkok commented 3 years ago

/remove-lifecycle stale

swatisehgal commented 3 years ago
No Task Break Down PR Owner Status
1. NFD Topology Updater Implementation https://github.com/kubernetes-sigs/node-feature-discovery/pull/525 @swatisehgal Merged
2. NFD Topology Updater Documentation https://github.com/kubernetes-sigs/node-feature-discovery/pull/526 @swatisehgal Merged
3. NFD Topology Updater E2E tests https://github.com/kubernetes-sigs/node-feature-discovery/pull/528 @fromanirh
4. Enable Configz Endpoint for obtaining Kubelet config https://github.com/kubernetes-sigs/node-feature-discovery/pull/530 @Tal-or
5. Allow excluding of specific resource accounting from NRT's objects https://github.com/kubernetes-sigs/node-feature-discovery/pull/545 @Tal-or
6. NFD Topology Updater: Add memory accounting in NRT https://github.com/kubernetes-sigs/node-feature-discovery/pull/593 @cynepco3hahue Merged
7. Simplify accounting logic in NFD-topology Updater TBD @fromanirh
8. NFD-topology Updater Helm deployment if topologyupdater explicitly enabled https://github.com/kubernetes-sigs/node-feature-discovery/pull/623 @zwpaper Merged
9. Event based NFD-topology Updater: Ensure that CRs are updated on every deviceplugin and/or pod creation/deletion event as opposed to the current timer based approach TBD TBD
ffromani commented 3 years ago

Expect the missing two PRs by monday

ffromani commented 3 years ago
No Task Break Down PR Owner
1. NFD Topology Updater Implementation https://github.com/kubernetes-sigs/node-feature-discovery/pull/525 @swatisehgal
2. NFD Topology Updater Documentation https://github.com/kubernetes-sigs/node-feature-discovery/pull/526 @swatisehgal
3. NFD Topology Updater E2E tests https://github.com/kubernetes-sigs/node-feature-discovery/pull/528 @fromanirh
4. Enable Configz Endpoint for obtaining Kubelet config https://github.com/kubernetes-sigs/node-feature-discovery/pull/530 @Tal-or
ArangoGutierrez commented 3 years ago

/label kind/feature

k8s-ci-robot commented 3 years ago

@ArangoGutierrez: The label(s) /label kind/feature cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda

In response to [this](https://github.com/kubernetes-sigs/node-feature-discovery/issues/333#issuecomment-897172226): >/label kind/feature Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
ArangoGutierrez commented 3 years ago

/kind feature

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

ffromani commented 2 years ago

/remove-lifecycle stale

ArangoGutierrez commented 2 years ago

Hey all, any updates on this feature set?

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

marquiz commented 2 years ago

There is progress lately in e2e-tests and /configz endpoint

/remove-lifecycle stale

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 1 year ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

marquiz commented 1 year ago

This is still alive and tracking /remove-lifecycle rotten

marquiz commented 1 year ago

@ffromani @Tal-or @swatisehgal are we expecting new PRs here or should we close this issue? We could also track the missing pieces as separate issues

marquiz commented 1 year ago

@ffromani @PiotrProkop @Tal-or @swatisehgal should we close this issue as implemented and track any future enhancements in separate issues?

ffromani commented 1 year ago

@ffromani @PiotrProkop @Tal-or @swatisehgal should we close this issue as implemented and track any future enhancements in separate issues?

From my perspective yes, we're now in a pretty good shape and we can track future enhancements with separate issues.

swatisehgal commented 1 year ago

@ffromani @PiotrProkop @Tal-or @swatisehgal should we close this issue as implemented and track any future enhancements in separate issues?

+1. Nice to see that we have come a long way :) Let's close this issue. Thanks!

marquiz commented 1 year ago

Closing as suggested. Thanks for everybody involved for working on this 🎉

/close

k8s-ci-robot commented 1 year ago

@marquiz: Closing this issue.

In response to [this](https://github.com/kubernetes-sigs/node-feature-discovery/issues/333#issuecomment-1453500157): >Closing as suggested. Thanks for everybody involved for working on this 🎉 > >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.