MGMT-22320: Expose GPU data in Agent CR inventory #8495
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@omer-vishlitzky: This pull request references MGMT-22320 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 the "4.21.0" version, but no target version was set. In 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. |
WalkthroughAdds GPU support to host inventory: defines a new public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)api/v1beta1/zz_generated.deepcopy.go (1)
🔇 Additional comments (6)
Comment |
openshift-ci
bot
added
the
size/M
openshift-ci
bot
added
the
api-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/controller/controllers/agent_controller.go (1)
1541-1551: GPU inventory propagation mirrors existing patterns and looks correctThe new block preallocates the slice and copies all relevant GPU fields, consistent with how disks and interfaces are handled. Only optional improvement (for future robustness) would be to explicitly clear
agent.Status.Inventory.Gpuswheninventory.Gpusis nil, if you ever need to reflect GPUs being removed; current behavior matches the existing “leave last known data” pattern used elsewhere.internal/controller/controllers/agent_controller_test.go (1)
2277-2285: GPU inventory propagation test looks solidThe test correctly seeds
models.Inventory.Gpusand then asserts that all GPU fields are present and mapped as expected intoagent.Status.Inventory.Gpus[0]. This gives good coverage for the new HostGpu wiring.If you later expand behavior (e.g., multiple GPUs, optional fields), consider adding a second GPU to the inventory and asserting slice length to guard against partial copies, but this is not required for the current change.
Also applies to: 2331-2336
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
vendor/github.com/openshift/assisted-service/api/v1beta1/agent_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (4)
api/v1beta1/agent_types.go(2 hunks)api/v1beta1/zz_generated.deepcopy.go(2 hunks)internal/controller/controllers/agent_controller.go(1 hunks)internal/controller/controllers/agent_controller_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
api/v1beta1/agent_types.gointernal/controller/controllers/agent_controller.gointernal/controller/controllers/agent_controller_test.goapi/v1beta1/zz_generated.deepcopy.go
🧬 Code graph analysis (2)
internal/controller/controllers/agent_controller.go (1)
api/v1beta1/agent_types.go (1)
HostGpu(168-179)
api/v1beta1/zz_generated.deepcopy.go (1)
api/v1beta1/agent_types.go (1)
HostGpu(168-179)
🔇 Additional comments (4)
api/v1beta1/zz_generated.deepcopy.go (2)
589-602: HostGpu DeepCopy helpers are consistent with other Host types*Implementation mirrors other scalar-only structs (e.g., HostBoot, HostIOPerf); shallow copy is sufficient and nil-safe via DeepCopy.
694-698: HostInventory deepcopy correctly adds GPU slice handlingThe new Gpus slice handling uses the same
make + copypattern as other value-only slices, so GPU data will be faithfully preserved on deep copies.api/v1beta1/agent_types.go (2)
168-179: HostGpu struct definition is clear and appropriately scopedFields and JSON tags line up with the inventory GPU attributes and keep everything optional; no functional or compatibility concerns here.
181-194: Extending HostInventory with Gpus is a safe, backwards-compatible additionAdding
Gpus []HostGputo HostInventory cleanly exposes GPU data on the Agent status without impacting existing fields or wire formats.
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maorfr, omer-vishlitzky The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8495 +/- ##
=======================================
Coverage 43.50% 43.50%
=======================================
Files 411 411
Lines 71080 71089 +9
=======================================
+ Hits 30920 30926 +6
- Misses 37406 37408 +2
- Partials 2754 2755 +1
🚀 New features to boost your workflow:
|
GPU data is collected by the agent from hosts but was not exposed in the Agent CR status inventory. This adds GPU information to the Agent CR so users can see GPU details when using kubeAPI mode (MCE, ACM, infrastructure-operator). Changes: - Add HostGpu struct to agent_types.go with fields: Address, DeviceID, Name, Vendor, VendorID - Add Gpus field to HostInventory struct - Update agent_controller to copy GPU data from host inventory to Agent CR status - Add DeepCopy functions for HostGpu type - Add unit test verifying GPU data is copied to Agent CR
omer-vishlitzky
force-pushed
the
MGMT-22320-gpu-inventory
branch
from
6490554 to
59517b4
Compare
openshift-ci
bot
added
size/L
| } | ||
| } | ||
|
|
||
| return r.updateLabels(log, ctx, agent) |
There was a problem hiding this comment.
do the labels we update here get aggregated to a ManagedCluster CR?
There was a problem hiding this comment.
No, we don't integrate with ManagedCluster at all really.
|
/retest |
|
@omer-vishlitzky: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/lgtm If we want labels we could always add them later. Otherwise just unhold this when you are done testing |
4 participants
Summary
Adds GPU information to Agent CR
status.inventoryso users in kubeAPI mode can see GPU details for discovered hosts.Changes:
HostGpustruct andGpusfield toHostInventoryJira: https://issues.redhat.com/browse/MGMT-22320