A workbench cannot be stopped if it's hardware profile doesn't exist - #6947
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/retest |
|
@DaoDaoNoCode wb_bug.mp4 |
| import useHardwareProfile from '#~/pages/hardwareProfiles/useHardwareProfile'; | ||
| import { HardwareProfileBindingState } from './const'; | ||
|
|
||
| export const useHardwareProfileBindingState = ( |
There was a problem hiding this comment.
I think this was a new feature introduced to 3.0 GA, I'm not sure if we can add this to 2.5.x like this
There was a problem hiding this comment.
Relevant ticket: https://redhat.atlassian.net/browse/RHOAIENG-22846
There was a problem hiding this comment.
That's a good point @nananosirova
I have simplified the hook to only fix the bug in d743e70
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## stable-2.x #6947 +/- ##
==============================================
- Coverage 67.85% 67.68% -0.18%
==============================================
Files 2235 2238 +3
Lines 50753 50876 +123
Branches 14208 14267 +59
==============================================
- Hits 34439 34433 -6
- Misses 16314 16443 +129
... and 47 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
… is deleted (opendatahub-io#5392) * On notebook start/stop, remove hwp annotations from notebook when hwp is deleted * Ensure deleted label persists on notebook stop/start * Avoid duplicate hook call in HardwareProfileTableColumn * Fix tests
Made-with: Cursor
Replace useHardwareProfileBindingState (3.0 feature code) with a minimal useDeletedHardwareProfilePatches hook that only detects 404 on hardware profile fetch and returns removal patches. Made-with: Cursor
d743e70 to
e9765be
Compare
DaoDaoNoCode
left a comment
DaoDaoNoCode
left a comment
There was a problem hiding this comment.
PR Review -- RHOAIENG-50913: A workbench cannot be stopped if its hardware profile doesn't exist
Summary
This PR is a backport of #5392 from main to stable-2.x. It fixes a bug where workbenches could not be stopped or started if their associated hardware profile had been deleted. The fix adds an extraPatches parameter to startNotebook and stopNotebook to strip stale hardware profile annotations when the referenced hardware profile returns a 404. The approach is correct and the adaptation to the stable-2.x codebase is well done.
Jira AC Coverage
- Workbench can be stopped when hardware profile is deleted -- implemented via
useDeletedHardwareProfilePatcheshook +extraPatchesonstopNotebook - Workbench can be started when hardware profile is deleted -- implemented via
extraPatchesonstartNotebook - Hardware profile annotations are cleaned up on stop/start --
REMOVE_HARDWARE_PROFILE_ANNOTATIONS_PATCHremovesnameandnamespaceannotations
Test Coverage
- Unit tests: Not present for
useDeletedHardwareProfilePatches-- consistent with the original PR #5392 on main which also lacks unit tests for this hook. See inline suggestion. - Cypress mock tests: Present -- two comprehensive tests covering stop and start scenarios with deleted hardware profiles, including request payload validation.
data-testidattributes: N/A -- no new UI elements added.
CI / Local Test Results
| Check | Result |
|---|---|
| Lint | PASS |
| Type-check (frontend) | PASS (pre-existing llama-stack-client errors only) |
| Type-check (backend) | FAIL (pre-existing, unrelated to this PR) |
| Cypress mock (projects/tabs) | FAIL (infra: turbo: not found) |
| Cypress mock (storageClasses) | FAIL (infra: turbo: not found) |
All CI failures are infrastructure issues (turbo: not found in the runner environment), not caused by this PR's code changes.
Overall
APPROVED
The core bug fix is correct and matches the original PR on main. The backport adaptations are well-reasoned (keeping tolerationChanges, simplified hook without binding state UI, dropping files that don't exist on stable-2.x). Inline suggestions are non-blocking improvements.
| if (hardwareProfileName && loaded && loadError && getGenericErrorCode(loadError) === 404) { | ||
| return REMOVE_HARDWARE_PROFILE_ANNOTATIONS_PATCH; | ||
| } | ||
| return []; |
There was a problem hiding this comment.
[SUGGESTION]
This returns a new empty array [] on every render when the hardware profile is not deleted (the common path). Since deletedHardwareProfilePatches is used in useCallback dependency arrays in both NotebookTableRow and ProjectTableRowNotebookTableRow, this causes onStart and handleStop callbacks to be recreated on every render.
Consider using a module-level constant for the empty array to ensure reference stability:
| return []; | |
| const EMPTY_PATCHES: Patch[] = []; | |
| // ... (at the top of the file, after imports) | |
| // then in the hook: | |
| return EMPTY_PATCHES; |
REMOVE_HARDWARE_PROFILE_ANNOTATIONS_PATCH is already a module-level constant so the deleted-profile path is already stable.
| return []; | ||
| }; | ||
|
|
||
| export default useDeletedHardwareProfilePatches; |
There was a problem hiding this comment.
[SUGGESTION]
No unit test exists for this hook. While the original PR #5392 on main also lacks one, this would be a good candidate for a testHook unit test since the logic is straightforward to test in isolation (mock useHardwareProfile to return loaded+404 error, verify patches are returned; mock it to return loaded+no error, verify empty array).
Not blocking since the Cypress mock tests cover the end-to-end behavior.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DaoDaoNoCode The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
117bbcd
into
opendatahub-io:stable-2.x
CI Failure Analysis -- Supplemental NoteFailing Jobs
Root Cause: GitHub Actions Cache EvictionBoth failures are caused by GitHub Actions cache misses, not by any test or code issue. The evidence:
Impact on This PRThe The Verification
|
3 participants
JIRA ticket - https://redhat.atlassian.net/browse/RHOAIENG-50913
Testing instructions and more details in #5392
From Claude:
The changes aren't identical because stable-2.x has diverged significantly from main where PR #5392 was originally merged. During conflict resolution, I had to adapt the code to fit the stable-2.x codebase. Here are the specific differences and why each was necessary:
startNotebook keeps tolerationChanges parameter On main, a separate prior PR removed tolerationChanges from startNotebook. On stable-2.x, it still exists. So the backport keeps tolerationChanges AND adds extraPatches (4 params instead of 3).
const.ts includes HardwareProfileBindingState enum On main, this enum already existed before PR On notebook start/stop, remove hwp annotations from notebook when hwp is deleted #5392 (added by a different PR). On stable-2.x, it doesn't exist. I had to add it because useHardwareProfileBindingState.ts and getDeletedHardwareProfilePatches depend on it. I omitted HARDWARE_PROFILE_BINDING_CONFIG and HARDWARE_PROFILE_SELECTION_HELP since they're not needed for the fix.
types.ts includes HardwareProfileBindingStateInfo type Same situation — this type existed on main before PR On notebook start/stop, remove hwp annotations from notebook when hwp is deleted #5392 but doesn't exist on stable-2.x. Added as a dependency.
useHardwareProfileBindingState.ts is a new file (not a modification) On main, this file existed and PR On notebook start/stop, remove hwp annotations from notebook when hwp is deleted #5392 only modified it. On stable-2.x, the file doesn't exist at all, so the entire file had to be added.
DeploymentHardwareProfileCell.tsx and HardwareProfileTableColumn.tsx are dropped These files exist on main but not on stable-2.x. PR On notebook start/stop, remove hwp annotations from notebook when hwp is deleted #5392 modified them to pass bindingState as a prop. Since the files don't exist on stable-2.x, those changes are not applicable.
NotebookTableRow.tsx keeps stable-2.x UI components On main, the hardware profile column uses HardwareProfileTableColumn. On stable-2.x, it uses NotebookTableRowHardwareProfile. I kept the stable-2.x UI but added the binding state + extra patches for start/stop (the actual bug fix).
utils.ts omits resourceTypeOf PR On notebook start/stop, remove hwp annotations from notebook when hwp is deleted #5392 added both getDeletedHardwareProfilePatches and resourceTypeOf. The latter was only used by HardwareProfileTableColumn which doesn't exist on stable-2.x, so it was omitted.
Cypress test removes findHardwareProfileColumn assertion This test helper method doesn't exist on stable-2.x's page object, and HardwareProfileModel import was missing from the auto-merge.
The core fix is identical: extraPatches on startNotebook/stopNotebook, the REMOVE_HARDWARE_PROFILE_ANNOTATIONS_PATCH constant, getDeletedHardwareProfilePatches utility, useHardwareProfileBindingState hook with the DELETED early-return, and the call sites passing those patches. The differences are all adaptations to dependencies and UI components that differ between the two branches.