CONSOLE-4945: Add Disk, Network, CPU and VM counts to node overview#16000
CONSOLE-4945: Add Disk, Network, CPU and VM counts to node overview#16000jeff-phillips-18 wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jeff-phillips-18: This pull request references CONSOLE-4945 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@jeff-phillips-18: GitHub didn't allow me to request PR reviews from the following users: kybaker. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
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-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jeff-phillips-18 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
kind/i18n
|
@jeff-phillips-18: This pull request references CONSOLE-4945 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis pull request extends the OpenShift Console nodes dashboard with hardware and virtual machine inventory visibility. It introduces localization keys for hardware metrics (Disk, Network, NIC, CPU) and KubeVirt inventory items. Two new utility modules—NodeBareMetalUtils.ts and NodeVmUtils.ts—provide plugin activation checks, Kubernetes resource watchers, and metric extractors for Bare Metal Hosts and Virtual Machine Instances respectively. NodesPage.tsx refactors to use the new hook-based utilities instead of direct resource retrieval. InventoryCard.tsx restructures from Stack to DescriptionList layout and integrates the new BareMetalInventoryItems component alongside a KubeVirt Virtual Machines section, both gated by feature flags. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/packages/console-app/src/components/nodes/NodeBareMetalUtils.ts`:
- Around line 33-49: The hook useWatchBareMetalHost currently watches
BareMetalHost cluster-wide (using useK8sWatchResource with
BareMetalHostGroupVersionKind and a fieldSelector) but BareMetalHost is
namespaced; update the function signature to accept a namespace parameter and
pass that namespace into the resource config (namespace: namespace) when
constructing the useK8sWatchResource args so the watch is scoped correctly; then
update callers (e.g., BareMetalInventoryItems.tsx where useWatchBareMetalHost is
called) to supply the node's namespace.
🧹 Nitpick comments (3)
frontend/packages/console-app/src/components/nodes/NodeVmUtils.ts (1)
44-67: Performance consideration: Client-side VMI filtering may not scale.The hook watches all VMIs cluster-wide and filters client-side by
nodeName. For clusters with many VMs, this could become expensive. I see the TODO at line 23 indicating this is temporary until the kubevirt plugin can own this column.For production readiness, consider:
- Adding a comment noting the expected scale limitations
- If the kubevirt plugin timeline is long, consider using a field selector on
status.nodeNameserver-side (if supported by the VMI API)Since the TODO indicates this is a bridge solution, not blocking—but worth tracking.
frontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsx (1)
109-115: Verify error prop type:vmsLoadErrormay be an Error object, not boolean.Looking at the
InventoryItemcomponent signature (from relevant snippets), theerrorprop appears to expect a boolean value (error = false). However,vmsLoadErrorfrom the watch hook is typically anErrorobject orundefined.While JavaScript's truthiness will make this work at runtime, consider explicit boolean conversion for type safety and clarity:
💡 Suggested fix
<InventoryItem isLoading={!vmsLoaded} title={t('console-app~Virtual machine')} titlePlural={t('console-app~Virtual machines')} count={vms.length} - error={vmsLoadError} + error={!!vmsLoadError} />frontend/packages/console-app/src/components/nodes/NodesPage.tsx (1)
905-916: Consider pre-grouping VMIs for O(n) lookup instead of O(n*m) filtering.The current implementation calls
filterVirtualMachineInstancesByNode(vmis, node.metadata.name)for each node, resulting in O(nodes × vmis) complexity. For clusters with many nodes and VMs, this could become noticeable.A more efficient approach would pre-group VMIs by nodeName once:
💡 Optional optimization
const vmsByNode = useMemo(() => { if (!isKubevirtPluginActive || !nodesLoaded || nodesLoadError || !vmisLoaded || vmisLoadError) { return undefined; } + // Pre-group VMIs by nodeName for O(1) lookup per node + const vmisByNodeName = new Map<string, K8sResourceKind[]>(); + vmis.forEach((vmi) => { + const nodeName = vmi.status?.nodeName; + if (nodeName) { + const existing = vmisByNodeName.get(nodeName) || []; + existing.push(vmi); + vmisByNodeName.set(nodeName, existing); + } + }); + return new Map( nodes.map((node) => [ - `${node.metadata.name}`, - filterVirtualMachineInstancesByNode(vmis, node.metadata.name), + node.metadata.name, + vmisByNodeName.get(node.metadata.name) || [], ]), ); }, [isKubevirtPluginActive, nodes, nodesLoadError, nodesLoaded, vmis, vmisLoadError, vmisLoaded]);Not blocking given the TODO indicates this is temporary, but worth considering if the kubevirt plugin integration timeline extends.
| export const useWatchBareMetalHost = (nodeName: string): WatchK8sResult<K8sResourceKind> => { | ||
| const isBareMetalPluginActive = useIsBareMetalPluginActive(); | ||
|
|
||
| const [bareMetalHosts, bareMetalHostsLoaded, bareMetalHostsLoadError] = useK8sWatchResource< | ||
| K8sResourceKind[] | ||
| >( | ||
| isBareMetalPluginActive | ||
| ? { | ||
| isList: true, | ||
| groupVersionKind: BareMetalHostGroupVersionKind, | ||
| fieldSelector: `metadata.name=${nodeName}`, | ||
| } | ||
| : undefined, | ||
| ); | ||
|
|
||
| return [bareMetalHosts?.[0], bareMetalHostsLoaded, bareMetalHostsLoadError]; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for BareMetalHost usage and namespace patterns
rg -n "BareMetalHost\|namespace\|fieldSelector" frontend/packages/console-app/src/components/nodes/NodeBareMetalUtils.ts -A 2 -B 2Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Find all files that import or reference BareMetalHost
rg -l "BareMetalHost\|useWatchBareMetalHost" frontend/packages --type=ts --type=tsxRepository: openshift/console
Length of output: 88
🏁 Script executed:
# Search for K8s namespace patterns in similar resource watchers
rg -n "useK8sWatchResource" frontend/packages/console-app/src/components --type=ts --type=tsx -A 3 | head -80Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Look for baremetal-related configuration or documentation
fd -e md -e ts -e tsx baremetal frontend/packages --type fRepository: openshift/console
Length of output: 1873
🏁 Script executed:
# Find usages of useWatchBareMetalHost
rg -n "useWatchBareMetalHost" frontend/packages --type=ts -B 2 -A 3Repository: openshift/console
Length of output: 2473
🏁 Script executed:
# Check the complete NodeBareMetalUtils.ts file
cat -n frontend/packages/console-app/src/components/nodes/NodeBareMetalUtils.tsRepository: openshift/console
Length of output: 2213
🏁 Script executed:
# Search for BareMetalHost namespace patterns in metal3-plugin
rg -n "BareMetalHost" frontend/packages/metal3-plugin/src --type=ts -B 1 -A 1 | head -120Repository: openshift/console
Length of output: 9649
🏁 Script executed:
# Check how useK8sWatchResource is used with namespaced resources
rg -n "useK8sWatchResource" frontend/packages/console-app/src/components --type=ts -B 2 -A 8 | head -100Repository: openshift/console
Length of output: 13262
🏁 Script executed:
# Check how BareMetalInventoryItems calls the hook and if it extracts namespace
cat -n frontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsx | head -80Repository: openshift/console
Length of output: 2958
🏁 Script executed:
# Look for namespace handling in metal3-plugin for BMH resources
rg -n "namespace.*BMH\|BMH.*namespace\|openshift-machine-api" frontend/packages/metal3-plugin/src --type=ts -B 2 -A 2 | head -100Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Check what the node object contains (obj parameter)
rg -n "obj\\.metadata" frontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsx -B 1 -A 1Repository: openshift/console
Length of output: 191
🏁 Script executed:
# Double-check: does useK8sWatchResource support implicit namespace behavior?
# Look at how it's typically used in the codebase with and without namespace
rg -n "useK8sWatchResource" frontend/packages/console-app/src/components/persistent-volumes -A 5 | head -60Repository: openshift/console
Length of output: 164
🏁 Script executed:
# Check if there's any Metal3 operator documentation about BMH namespace
rg -n "openshift-machine-api\|default.*namespace\|BMH.*namespace" frontend/packages --type=ts | head -20Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Search for useK8sWatchResource patterns with namespaced resources
rg -n "useK8sWatchResource" frontend/packages/console-app/src -A 6 | grep -A 6 "isList.*true" | head -80Repository: openshift/console
Length of output: 7969
🏁 Script executed:
# Check metal3 constants or configuration for namespace hints
rg -n "namespace\|NAMESPACE\|openshift-machine" frontend/packages/metal3-plugin/src/constants -A 2 -B 2Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Look at actual K8s hook implementation to understand default behavior
find frontend/packages -name "*k8s*hook*" -o -name "*watch*" | grep -E "\.(ts|js)$" | head -10Repository: openshift/console
Length of output: 285
🏁 Script executed:
# Check the watch hook type definitions to understand namespace behavior
cat -n frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/k8s-watch-types.tsRepository: openshift/console
Length of output: 1076
🏁 Script executed:
# Look at the watcher implementation
head -100 frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/k8s-watcher.tsRepository: openshift/console
Length of output: 3090
Add namespace parameter to useWatchBareMetalHost—BMH is a namespaced resource but the hook watches cluster-wide.
Line 33: The hook signature omits the namespace parameter, and line 40–44 doesn't scope the watch to any namespace. Since BareMetalHostModel is marked namespaced: true (line 19), the resource watch should explicitly include namespace in the resource config; otherwise, useK8sWatchResource queries all namespaces and relies only on fieldSelector: metadata.name to filter results. This can fail if multiple BMH resources with identical names exist across namespaces.
Update the hook to accept and pass the namespace:
export const useWatchBareMetalHost = (nodeName: string, namespace: string): WatchK8sResult<K8sResourceKind> => {
const isBareMetalPluginActive = useIsBareMetalPluginActive();
const [bareMetalHosts, bareMetalHostsLoaded, bareMetalHostsLoadError] = useK8sWatchResource<
K8sResourceKind[]
>(
isBareMetalPluginActive
? {
isList: true,
groupVersionKind: BareMetalHostGroupVersionKind,
namespace,
fieldSelector: `metadata.name=${nodeName}`,
}
: undefined,
);
return [bareMetalHosts?.[0], bareMetalHostsLoaded, bareMetalHostsLoadError];
};Update the call site in BareMetalInventoryItems.tsx line 64 to pass the namespace from the node context.
🤖 Prompt for AI Agents
In `@frontend/packages/console-app/src/components/nodes/NodeBareMetalUtils.ts`
around lines 33 - 49, The hook useWatchBareMetalHost currently watches
BareMetalHost cluster-wide (using useK8sWatchResource with
BareMetalHostGroupVersionKind and a fieldSelector) but BareMetalHost is
namespaced; update the function signature to accept a namespace parameter and
pass that namespace into the resource config (namespace: namespace) when
constructing the useK8sWatchResource args so the watch is scoped correctly; then
update callers (e.g., BareMetalInventoryItems.tsx where useWatchBareMetalHost is
called) to supply the node's namespace.
|
@jeff-phillips-18: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
Enhance the Node Details Inventory Card with bare metal hardware metrics and virtual machine counts. This PR migrates the inventory display from PatternFly Stack components to DescriptionList components and adds support for displaying hardware information when bare metal or kubevirt plugins are active.
Details
The Node Details page Inventory Card previously only displayed Pods and Images. This enhancement adds:
Bare Metal Hardware Metrics (when bare metal plugin is active):
Virtual Machine Metrics (when kubevirt plugin is active):
UI/UX Improvements:
Screen shots
Code Organization:
Test Plan
- Navigate to Compute → Nodes → [Any Node] → Overview tab
- Verify Inventory card displays Pods and Images sections
- Verify loading states show skeleton UI (not dashes)
- Navigate to a bare metal node's Overview tab
- Verify Disk, Network, and CPU sections appear
- Verify disk/NIC/CPU counts display correctly
- Click Disk count → verify navigates to BareMetalHost /disks tab
- Click NIC count → verify navigates to BareMetalHost /nics tab
- Navigate to a node running VMs
- Verify "Virtual machines (CNV)" section appears
- Verify VM count matches actual VMs on the node
- Click VM count → verify navigates to VirtualMachines list filtered by node (?rowFilter-node=)
- Node without bare metal host → bare metal sections don't appear
- Kubevirt disabled → VM section doesn't appear
- Node with 0 VMs → shows "0 Virtual machines"
- Data load errors → dash displays when data unavailable
/cc @kybaker
Summary by CodeRabbit
Release Notes