client: fixed a bug where the bandwidth of reserved cores were not taken into account

Author: mvegter

.changelog/26768.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+client: fixed a bug where the bandwidth of reserved cores were not taken into account
+```

client/client.go

@@ -1816,8 +1816,19 @@ func (c *Client) updateNodeFromFingerprint(response *fingerprint.FingerprintResp
 		}
 
 		// update config with total cpu compute if it was detected
-		if cpu := response.NodeResources.Processors.TotalCompute(); cpu > 0 {
-			newConfig.CpuCompute = cpu
+		totalCompute := response.NodeResources.Processors.TotalCompute()
+		usableCompute := response.NodeResources.Processors.UsableCompute()
+		if totalCompute > 0 {
+			if newConfig.CpuCompute != totalCompute {
+				newConfig.CpuCompute = totalCompute
+				nodeHasChanged = true
+			}
+
+			reservedCompute := int64(totalCompute - usableCompute)
+			if newConfig.Node.ReservedResources.Cpu.CpuShares != reservedCompute {
+				newConfig.Node.ReservedResources.Cpu.CpuShares = reservedCompute
+				nodeHasChanged = true
+			}
 		}
 	}
 
@@ -3323,8 +3334,9 @@ func (c *Client) setGaugeForDiskStats(hStats *hoststats.HostStats, baseLabels []
 // setGaugeForAllocationStats proxies metrics for allocation specific statistics
 func (c *Client) setGaugeForAllocationStats(baseLabels []metrics.Label) {
 	node := c.GetConfig().Node
-	total := node.NodeResources
-	res := node.ReservedResources
+
+	available := node.NodeResources.Comparable()
+	available.Subtract(node.ReservedResources.Comparable())
 	allocated := c.getAllocatedResources(node)
 
 	// Emit allocated
@@ -3342,20 +3354,15 @@ func (c *Client) setGaugeForAllocationStats(baseLabels []metrics.Label) {
 	}
 
 	// Emit unallocated
-	unallocatedMem := total.Memory.MemoryMB - res.Memory.MemoryMB - allocated.Flattened.Memory.MemoryMB
-	unallocatedDisk := total.Disk.DiskMB - res.Disk.DiskMB - allocated.Shared.DiskMB
-
-	// The UsableCompute function call already subtracts and accounts for any
-	// reserved CPU within the client configuration. Therefore, we do not need
-	// to subtract that here.
-	unallocatedCpu := int64(total.Processors.Topology.UsableCompute()) - allocated.Flattened.Cpu.CpuShares
+	unallocatedMem := available.Flattened.Memory.MemoryMB - allocated.Flattened.Memory.MemoryMB
+	unallocatedDisk := available.Shared.DiskMB - allocated.Shared.DiskMB
+	unallocatedCpu := available.Flattened.Cpu.CpuShares - allocated.Flattened.Cpu.CpuShares
 
 	metrics.SetGaugeWithLabels([]string{"client", "unallocated", "memory"}, float32(unallocatedMem), baseLabels)
 	metrics.SetGaugeWithLabels([]string{"client", "unallocated", "disk"}, float32(unallocatedDisk), baseLabels)
 	metrics.SetGaugeWithLabels([]string{"client", "unallocated", "cpu"}, float32(unallocatedCpu), baseLabels)
 
-	totalComparable := total.Comparable()
-	for _, n := range totalComparable.Flattened.Networks {
+	for _, n := range available.Flattened.Networks {
 		// Determined the used resources
 		var usedMbits int
 		totalIdx := allocated.Flattened.Networks.NetIndex(n)

client/client_test.go

@@ -1688,6 +1688,54 @@ func TestClient_UpdateNodeFromDevicesAccumulates(t *testing.T) {
 
 }
 
+func TestClient_UpdateNodeFromFingerprintCalculatesReservedResources(t *testing.T) {
+	ci.Parallel(t)
+
+	// Client without network configured updates to match fingerprint
+	client, cleanup := TestClient(t, func(c *config.Config) {
+		c.Node.ReservedResources.Cpu = structs.NodeReservedCpuResources{
+			CpuShares:        100,
+			ReservedCpuCores: []uint16{0},
+		}
+	})
+	defer cleanup()
+
+	// Set up a basic topology where the first core is and an
+	// additionally 100 MHz are reserved
+	basicTopology := structs.MockBasicTopology()
+	basicTopology.Cores[0].Disable = true
+	basicTopology.OverrideWitholdCompute = 100
+	client.updateNodeFromFingerprint(&fingerprint.FingerprintResponse{
+		// overrides the detected hardware in TestClient
+		NodeResources: &structs.NodeResources{
+			Memory: structs.NodeMemoryResources{MemoryMB: 1024},
+			Processors: structs.NodeProcessorResources{
+				Topology: basicTopology,
+			},
+		},
+	})
+
+	// initial check
+	conf := client.GetConfig()
+
+	expectedReservedResources := &structs.NodeReservedResources{
+		Cpu: structs.NodeReservedCpuResources{
+			CpuShares:        3_600, // 1 * 3500 MHz and 100 MHz
+			ReservedCpuCores: []uint16{0},
+		},
+		Memory: structs.NodeReservedMemoryResources{
+			MemoryMB: 256,
+		},
+		Disk: structs.NodeReservedDiskResources{
+			DiskMB: 4096,
+		},
+		Networks: structs.NodeReservedNetworkResources{
+			ReservedHostPorts: "22",
+		},
+	}
+	must.Eq(t, expectedReservedResources, conf.Node.ReservedResources)
+}
+
 // TestClient_UpdateNodeFromFingerprintKeepsConfig asserts manually configured
 // network interfaces take precedence over fingerprinted ones.
 func TestClient_UpdateNodeFromFingerprintKeepsConfig(t *testing.T) {

client/lib/numalib/topology.go

@@ -282,6 +282,16 @@ func (st *Topology) NumECores() int {
 	return total
 }
 
+// TotalCores returns the number of logical cores detected. The UsableCores will
+// be equal to or less than this value.
+func (st *Topology) TotalCores() *idset.Set[hw.CoreID] {
+	result := idset.Empty[hw.CoreID]()
+	for _, cpu := range st.Cores {
+		result.Insert(cpu.ID)
+	}
+	return result
+}
+
 // UsableCores returns the number of logical cores usable by the Nomad client
 // for running tasks. Nomad must subtract off any reserved cores (reserved.cores)
 // and/or must mask the cpuset to the one set in config (config.reservable_cores).

nomad/structs/funcs.go

@@ -232,20 +232,13 @@ func AllocsFit(node *Node, allocs []*Allocation, netIdx *NetworkIndex, checkDevi
 }
 
 func computeFreePercentage(node *Node, util *ComparableResources) (freePctCpu, freePctRam float64) {
-	reserved := node.ReservedResources.Comparable()
-	res := node.NodeResources.Comparable()
-
 	// Determine the node availability
-	nodeCpu := float64(res.Flattened.Cpu.CpuShares)
-	nodeMem := float64(res.Flattened.Memory.MemoryMB)
-	if reserved != nil {
-		nodeCpu -= float64(reserved.Flattened.Cpu.CpuShares)
-		nodeMem -= float64(reserved.Flattened.Memory.MemoryMB)
-	}
+	available := node.NodeResources.Comparable()
+	available.Subtract(node.ReservedResources.Comparable())
 
 	// Compute the free percentage
-	freePctCpu = 1 - (float64(util.Flattened.Cpu.CpuShares) / nodeCpu)
-	freePctRam = 1 - (float64(util.Flattened.Memory.MemoryMB) / nodeMem)
+	freePctCpu = 1 - (float64(util.Flattened.Cpu.CpuShares) / float64(available.Flattened.Cpu.CpuShares))
+	freePctRam = 1 - (float64(util.Flattened.Memory.MemoryMB) / float64(available.Flattened.Memory.MemoryMB))
 	return freePctCpu, freePctRam
 }
 

nomad/structs/funcs_test.go

@@ -102,10 +102,22 @@ func node2k() *Node {
 						ID:        0,
 						Grade:     numalib.Performance,
 						BaseSpeed: 1000,
+						Disable:   false,
 					}, {
 						ID:        1,
 						Grade:     numalib.Performance,
 						BaseSpeed: 1000,
+						Disable:   false,
+					}, {
+						ID:        2,
+						Grade:     numalib.Performance,
+						BaseSpeed: 1000,
+						Disable:   true,
+					}, {
+						ID:        3,
+						Grade:     numalib.Performance,
+						BaseSpeed: 1000,
+						Disable:   true,
 					}},
 					OverrideWitholdCompute: 1000, // set by client reserved field
 				},
@@ -137,7 +149,8 @@ func node2k() *Node {
 		},
 		ReservedResources: &NodeReservedResources{
 			Cpu: NodeReservedCpuResources{
-				CpuShares: 1000,
+				CpuShares:        3000, // set by fingerprinting callback
+				ReservedCpuCores: []uint16{2, 3},
 			},
 			Memory: NodeReservedMemoryResources{
 				MemoryMB: 1024,
@@ -248,6 +261,38 @@ func TestAllocsFit(t *testing.T) {
 	must.Eq(t, 1000, used.Flattened.Cpu.CpuShares)
 	must.Eq(t, []uint16{0}, used.Flattened.Cpu.ReservedCores)
 	must.Eq(t, 1024, used.Flattened.Memory.MemoryMB)
+
+	a3 := &Allocation{
+		AllocatedResources: &AllocatedResources{
+			Tasks: map[string]*AllocatedTaskResources{
+				"web": {
+					Cpu: AllocatedCpuResources{
+						CpuShares: 1000,
+					},
+					Memory: AllocatedMemoryResources{
+						MemoryMB: 512,
+					},
+				},
+			},
+		},
+	}
+
+	// Should fit one allocation
+	fit, dim, used, err = AllocsFit(n, []*Allocation{a3}, nil, false)
+	must.NoError(t, err)
+	must.True(t, fit, must.Sprintf("failed for dimension %q", dim))
+	must.Eq(t, 1000, used.Flattened.Cpu.CpuShares)
+	must.Eq(t, []uint16{}, used.Flattened.Cpu.ReservedCores)
+	must.Eq(t, 512, used.Flattened.Memory.MemoryMB)
+
+	// Should not fit second allocation
+	fit, dim, used, err = AllocsFit(n, []*Allocation{a3, a3}, nil, false)
+	must.NoError(t, err)
+	must.False(t, fit)
+	must.Eq(t, "cpu", dim)
+	must.Eq(t, 2000, used.Flattened.Cpu.CpuShares)
+	must.Eq(t, []uint16{}, used.Flattened.Cpu.ReservedCores)
+	must.Eq(t, 1024, used.Flattened.Memory.MemoryMB)
 }
 
 func TestAllocsFit_Cores(t *testing.T) {
@@ -649,8 +694,23 @@ func TestScoreFitBinPack(t *testing.T) {
 				Cores: []numalib.Core{{
 					ID:        0,
 					Grade:     numalib.Performance,
-					BaseSpeed: 4096,
+					BaseSpeed: 2048,
+				}, {
+					ID:        1,
+					Grade:     numalib.Performance,
+					BaseSpeed: 2048,
+				}, {
+					ID:        2,
+					Grade:     numalib.Performance,
+					BaseSpeed: 2048,
+					Disable:   true,
+				}, {
+					ID:        3,
+					Grade:     numalib.Performance,
+					BaseSpeed: 2048,
+					Disable:   true,
 				}},
+				OverrideWitholdCompute: 2048, // set by client reserved field
 			},
 		},
 		Memory: NodeMemoryResources{
@@ -661,7 +721,8 @@ func TestScoreFitBinPack(t *testing.T) {
 	node.NodeResources.Compatibility()
 	node.ReservedResources = &NodeReservedResources{
 		Cpu: NodeReservedCpuResources{
-			CpuShares: 2048,
+			CpuShares:        6144, // set by fingerprinting callback
+			ReservedCpuCores: []uint16{2, 3},
 		},
 		Memory: NodeReservedMemoryResources{
 			MemoryMB: 4096,

nomad/structs/numa.go

@@ -172,3 +172,10 @@ func (r *NodeProcessorResources) TotalCompute() int {
 	}
 	return int(r.Topology.TotalCompute())
 }
+
+func (r *NodeProcessorResources) UsableCompute() int {
+	if r == nil || r.Topology == nil {
+		return 0
+	}
+	return int(r.Topology.UsableCompute())
+}

nomad/structs/structs.go

@@ -3244,16 +3244,16 @@ func (n *NodeResources) Comparable() *ComparableResources {
 		return nil
 	}
 
-	usableCores := n.Processors.Topology.UsableCores().Slice()
-	reservableCores := helper.ConvertSlice(usableCores, func(id hw.CoreID) uint16 {
+	totalCores := n.Processors.Topology.TotalCores().Slice()
+	cores := helper.ConvertSlice(totalCores, func(id hw.CoreID) uint16 {
 		return uint16(id)
 	})
 
 	c := &ComparableResources{
 		Flattened: AllocatedTaskResources{
 			Cpu: AllocatedCpuResources{
 				CpuShares:     int64(n.Processors.Topology.TotalCompute()),
-				ReservedCores: reservableCores,
+				ReservedCores: cores,
 			},
 			Memory: AllocatedMemoryResources{
 				MemoryMB: n.Memory.MemoryMB,