CONSOLE-5284: Put new node inventory items behind tech-preview#16414
CONSOLE-5284: Put new node inventory items behind tech-preview#16414jeff-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-5284 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.22" instead. 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. |
|
[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 |
📝 WalkthroughWalkthroughThe InventoryCard component gates inventory display sections behind a feature flag ( 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsx`:
- Around line 61-64: The VM watch hook useWatchVirtualMachineInstances is being
invoked unconditionally (creating side effects) even when FLAG_NODE_MGMT_V1 is
off; modify the hook or its usage so watching is gated by the feature flag:
update useWatchVirtualMachineInstances to accept an enabled boolean (e.g.,
useWatchVirtualMachineInstances(nodeName: string, enabled = true)) or, if you
cannot change the hook, only call it when nodeMgmtV1Enabled && showVms and
provide safe fallback values for vms, vmsLoaded, and vmsLoadError; in
InventoryCard.tsx replace the unconditional call with a guarded call using
nodeMgmtV1Enabled (and showVms) so the VM watch traffic only starts when
FLAG_NODE_MGMT_V1 is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3569d736-5944-47b0-ac9a-35fe6aa34216
📒 Files selected for processing (1)
frontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsx
📜 Review details
🔇 Additional comments (1)
frontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsx (1)
95-124: Good boundary for tech-preview UI gating.This correctly keeps Pods/Images always visible while placing bare metal + VM inventory UI behind
nodeMgmtV1Enabled.
| const nodeMgmtV1Enabled = useFlag(FLAG_NODE_MGMT_V1); | ||
| const showVms = useIsKubevirtPluginActive(); | ||
|
|
||
| const [vms, vmsLoaded, vmsLoadError] = useWatchVirtualMachineInstances(obj.metadata.name); |
There was a problem hiding this comment.
Gate VM watch side effects behind the feature flag, not just rendering.
Line 64 still invokes useWatchVirtualMachineInstances(...) unconditionally, so VM watch traffic can continue even when FLAG_NODE_MGMT_V1 is off. That weakens the tech-preview gate and adds avoidable API/RBAC churn.
Proposed fix
const nodeMgmtV1Enabled = useFlag(FLAG_NODE_MGMT_V1);
const showVms = useIsKubevirtPluginActive();
+ const shouldShowVms = nodeMgmtV1Enabled && showVms;
- const [vms, vmsLoaded, vmsLoadError] = useWatchVirtualMachineInstances(obj.metadata.name);
+ const [vms, vmsLoaded, vmsLoadError] = useWatchVirtualMachineInstances(
+ obj.metadata.name,
+ shouldShowVms,
+ );- {showVms ? (
+ {shouldShowVms ? (// If needed in NodeVmUtils:
useWatchVirtualMachineInstances(nodeName: string, enabled = true)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@frontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsx`
around lines 61 - 64, The VM watch hook useWatchVirtualMachineInstances is being
invoked unconditionally (creating side effects) even when FLAG_NODE_MGMT_V1 is
off; modify the hook or its usage so watching is gated by the feature flag:
update useWatchVirtualMachineInstances to accept an enabled boolean (e.g.,
useWatchVirtualMachineInstances(nodeName: string, enabled = true)) or, if you
cannot change the hook, only call it when nodeMgmtV1Enabled && showVms and
provide safe fallback values for vms, vmsLoaded, and vmsLoadError; in
InventoryCard.tsx replace the unconditional call with a guarded call using
nodeMgmtV1Enabled (and showVms) so the VM watch traffic only starts when
FLAG_NODE_MGMT_V1 is true.
jeff-phillips-18
force-pushed
the
node-inventory-items-flagged
branch
from
7c39c36 to
8ac9499
Compare
|
@jeff-phillips-18: The following test failed, say
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
Analysis / Root cause:
New node inventory items (bare metal hardware metrics and virtual machines) need to be gated behind the tech-preview
NODE_MGMT_V1feature flag as part of the node management v1 feature rollout.Solution description:
FLAG_NODE_MGMT_V1flag check to the node dashboard InventoryCard componentBareMetalInventoryItemsand virtual machine inventory items in a conditional render based on the flag**Screenshots **:
*** Before / With Tech-Preview:
*** After / without Tech-Preview
Test setup:
NODE_MGMT_V1feature flag both enabled and disabledTest cases:
NODE_MGMT_V1flag disabled: BareMetalInventoryItems and VM inventory items should not be displayed on node dashboardNODE_MGMT_V1flag enabled: BareMetalInventoryItems (disks, NICs, CPUs) should be displayed when a BareMetalHost is associated with the nodeNODE_MGMT_V1flag enabled: VM inventory count should be displayed when KubeVirt plugin is active and VMs exist on the nodeBrowser conformance:
Additional info:
This change is part of the larger node management v1 feature work tracked in CONSOLE-5284. The feature flag allows for controlled rollout and testing of the new inventory items.
Reviewers and assignees:
/cc @jhadvig
Summary by CodeRabbit