Sync RTE with NFD #32

Tal-or · 2021-09-14T09:47:54Z

Sync podres package
Bump NRT API to latest:
Update vendor files
Fix code to match the new API
Populate Capacity, Available and Allocatable as NFD is doing

TODO (on separate PR) when openshift-kni/resource-topology-exporter#23 and jaypipes/ghw#268 get merged we should take care of memory capacity accounting as well

swatisehgal · 2021-09-14T10:01:48Z

The available accounting will be added on a separate commit

Just an FYI, the scheduler plugin uses available field to make its NUMA aware scheduling decision, so we need to make sure that available field is populated to make sure that it works with the latest version of the scheduler plugin.

swatisehgal · 2021-09-14T10:12:46Z

pkg/resourcemonitor/resourcemonitor_test.go

@@ -691,23 +691,23 @@ func TestResourcesScan(t *testing.T) {
 					Resources: topologyv1alpha1.ResourceInfoList{
 						topologyv1alpha1.ResourceInfo{
 							Name:        "cpu",
-							Allocatable: intstr.FromString("10"),
-							Capacity:    intstr.FromString("12"),
+							Allocatable: resource.MustParse("10"),


Wanted to point out how I went ahead with this change in NFD, I assumed that capacity=allocatable (i.e. we don't account for kube-reserved and system reserved) and changed the values in the allocatable field to be captured in the available field some like I did here: kubernetes-sigs/node-feature-discovery@041aa80 and then later in a subsequent commit took the reserved resources into consideration: kubernetes-sigs/node-feature-discovery@043c7a8.

Tal-or · 2021-09-14T11:03:22Z

The available accounting will be added on a separate commit

Just an FYI, the scheduler plugin uses available field to make its NUMA aware scheduling decision, so we need to make sure that available field is populated to make sure that it works with the latest version of the scheduler plugin.

This is going to be part of this PR but in a separate commit for readability porpuses

Tal-or · 2021-09-14T11:07:43Z

@swatisehgal @fromanirh FYI the Allocatable field in RTE is behaving as the Available filed in NFD.
I just tested it on my setup, one you deploy/delete a guaranteed pod, the values under Allocatable are changing, while it shouldn't, becuase it should reflect the amount of capacity - reserved and it doesn't depend on the number of running pods in the cluster.
As you can see: https://github.com/k8stopologyawareschedwg/resource-topology-exporter/blob/master/pkg/resourcemonitor/resourcemonitor.go#L171
is this change in the behavior is intentional?

ffromani · 2021-09-14T11:10:18Z

pkg/resourcemonitor/resourcemonitor_test.go

@@ -18,12 +18,12 @@ package resourcemonitor

 import (
 	"encoding/json"
+	"k8s.io/apimachinery/pkg/api/resource"


please put the import in the group below alongside with the other k8s imports

ffromani · 2021-09-14T11:14:45Z

@swatisehgal @fromanirh FYI the Allocatable field in RTE is behaving as the Available filed in NFD.
I just tested it on my setup, one you deploy/delete a guaranteed pod, the values under Allocatable are changing, while it shouldn't, becuase it should reflect the amount of capacity - reserved and it doesn't depend on the number of running pods in the cluster.
As you can see: https://github.com/k8stopologyawareschedwg/resource-topology-exporter/blob/master/pkg/resourcemonitor/resourcemonitor.go#L171
is this change in the behavior is intentional?

With nodresourcetopology API 0.0.8 "Allocatable" behaves like "Available" in API 0.0.10:

v0.0.8		v0.0.10
N/A		Capacity
Capacity	Allocatable
Allocatable	Available

Tal-or · 2021-09-14T11:20:08Z

With nodresourcetopology API 0.0.8 "Allocatable" behaves like "Available" in API 0.0.10:
v0.0.8           v0.0.10
N/A               Capacity
Capacity      Allocatable
Allocatable   Available

Oh I see. Sorry I wasn't aware of that, I misread what Swati mentioned above

swatisehgal · 2021-09-14T12:01:12Z

pkg/resourcemonitor/resourcemonitor_test.go

 						},
 						topologyv1alpha1.ResourceInfo{
 							Name:        "fake.io/net",
-							Allocatable: intstr.FromString("4"),
-							Capacity:    intstr.FromString("4"),
+							Available:   resource.MustParse("12"),


This looks like a mistake to me: available should be equal to 4 here as well. Also note that available will always be <= both allocatable and capacity.

yes. copy paste mistake I just found out myself. Thanks!

ffromani · 2021-09-14T12:58:37Z

the podres sync patch can be a separate PR which we can merge quickly.

Signed-off-by: Talor Itzhak <[email protected]>

- Update vendor files - Fix code to match the new API This patch is following the way of how the new API integration was done on NFD: It assumes capacity is equal to allocatable and in a subsequent patch we need to account for kube-reserved and system reserved to determine the allocatable, as allocatable = capcity - kube-reserved/system-reserved Signed-off-by: Talor Itzhak <[email protected]>

This patch updates the Capacity, Allocatable and Available accountig logic to be identical to what NFD has: k8stopologyawareschedwg/node-feature-discovery@043c7a8 Signed-off-by: Talor Itzhak <[email protected]>

ffromani · 2021-09-15T11:08:09Z

good enough, we can improve in followup PRs.

Tal-or changed the title ~~WIP: Sync rte with nfd~~ WIP: Sync RTE with NFD Sep 14, 2021

swatisehgal reviewed Sep 14, 2021

View reviewed changes

ffromani reviewed Sep 14, 2021

View reviewed changes

Tal-or force-pushed the sync_rte_with_nfd branch from e28642b to 2ed1d45 Compare September 14, 2021 11:51

swatisehgal reviewed Sep 14, 2021

View reviewed changes

Tal-or force-pushed the sync_rte_with_nfd branch from 2ed1d45 to af0b0d4 Compare September 14, 2021 12:14

Sync podres package

a7ae692

Signed-off-by: Talor Itzhak <[email protected]>

Tal-or force-pushed the sync_rte_with_nfd branch from 51c6e19 to 20d613b Compare September 14, 2021 16:27

Tal-or force-pushed the sync_rte_with_nfd branch from 20d613b to 223068b Compare September 14, 2021 16:33

Sync resource accounting with NFD

fcc4691

This patch updates the Capacity, Allocatable and Available accountig logic to be identical to what NFD has: k8stopologyawareschedwg/node-feature-discovery@043c7a8 Signed-off-by: Talor Itzhak <[email protected]>

Tal-or force-pushed the sync_rte_with_nfd branch from 223068b to fcc4691 Compare September 14, 2021 17:02

Tal-or changed the title ~~WIP: Sync RTE with NFD~~ Sync RTE with NFD Sep 14, 2021

ffromani merged commit 02e7192 into master Sep 15, 2021

ffromani deleted the sync_rte_with_nfd branch September 15, 2021 11:08

ffromani mentioned this pull request Sep 15, 2021

NFD resync cleanup #34

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync RTE with NFD #32

Sync RTE with NFD #32

Tal-or commented Sep 14, 2021 •

edited

Loading

swatisehgal commented Sep 14, 2021 •

edited

Loading

swatisehgal Sep 14, 2021 •

edited

Loading

Tal-or commented Sep 14, 2021

Tal-or commented Sep 14, 2021 •

edited

Loading

ffromani Sep 14, 2021

ffromani commented Sep 14, 2021 •

edited

Loading

Tal-or commented Sep 14, 2021 •

edited

Loading

swatisehgal Sep 14, 2021

Tal-or Sep 14, 2021 •

edited

Loading

ffromani commented Sep 14, 2021

ffromani commented Sep 15, 2021

Sync RTE with NFD #32

Sync RTE with NFD #32

Conversation

Tal-or commented Sep 14, 2021 • edited Loading

swatisehgal commented Sep 14, 2021 • edited Loading

swatisehgal Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Tal-or commented Sep 14, 2021

Tal-or commented Sep 14, 2021 • edited Loading

ffromani Sep 14, 2021

Choose a reason for hiding this comment

ffromani commented Sep 14, 2021 • edited Loading

Tal-or commented Sep 14, 2021 • edited Loading

swatisehgal Sep 14, 2021

Choose a reason for hiding this comment

Tal-or Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

ffromani commented Sep 14, 2021

ffromani commented Sep 15, 2021

Tal-or commented Sep 14, 2021 •

edited

Loading

swatisehgal commented Sep 14, 2021 •

edited

Loading

swatisehgal Sep 14, 2021 •

edited

Loading

Tal-or commented Sep 14, 2021 •

edited

Loading

ffromani commented Sep 14, 2021 •

edited

Loading

Tal-or commented Sep 14, 2021 •

edited

Loading

Tal-or Sep 14, 2021 •

edited

Loading