OCPBUGS-59404: Allow VolumeSnapshot restore when parent PVC is deleted#16447
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@sg00dwin: This pull request references Jira Issue OCPBUGS-59404, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
📝 WalkthroughWalkthroughThe change modifies the PVC restore modal to handle cases where the source PVC no longer exists. It introduces a 🚥 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/modals/restore-pvc/restore-pvc-modal.tsx`:
- Around line 83-93: The code treats any "loaded but no PVC object" (pvcNotFound
computed from pvcResourceLoaded && !pvcResource) as deletion which can
misclassify non-404 errors; update the logic that computes pvcNotFound to only
be true when the GET hook indicates an explicit 404 (check the hook's error
payload field name, e.g., error.status or error.code) returned by the useK8sGet
call for the PVC (use the same variable holding the error/status), and then use
that gated pvcNotFound when deriving formReady so transient/API/RBAC errors
won't set formReady 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: 2ed0bfd6-8878-41cc-b433-fe2ea06b485c
📒 Files selected for processing (1)
frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/**/*.{ts,tsx}: Never import from package index files (barrel imports) like@console/sharedin new code, as they create circular dependencies and slow builds. Import from specific file paths instead.
Never use backticks in i18n t() function calls as the i18n parser cannot extract keys from template literals. Use single or double quotes instead.
Never import from deprecated packages or use code marked with@deprecatedTSdoc tag in new code.
Files:
frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx
frontend/**/*.{ts,tsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Never use absolute URLs or paths. The console runs behind a proxy under an arbitrary path.
Files:
frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx
**/*
📄 CodeRabbit inference engine (STYLEGUIDE.md)
Use lowercase dash-separated names for all files (to avoid git issues with case-insensitive file systems)
Files:
frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
**/*.{ts,tsx,js,jsx}: New code MUST be written in TypeScript, not JavaScript
Run the linter and follow all rules defined in .eslintrc
Never use absolute paths in code; the app should be able to run behind a proxy under an arbitrary path
Use PascalCase for component file names, kebab-case for utility files, and*.spec.ts(x)for test filesUse camelCase for variable names in TypeScript and JavaScript files
**/*.{ts,tsx,js,jsx}: Any usage of i18next'sTFunction(rather than react-i18next'sTFunction) must be performed inside a function or component.
Don't use backticks inside of aTFunction. Our code parser will not automatically pick up the keys that contain backticks. Use single or double quotes instead.
Specify possible static values in comments for dynamic i18next keys that can't be interpolated by i18next-parser, such ast(key),t('key' + id), ort(key${id}).
Files:
frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
**/*.{ts,tsx}: Prefer functional programming patterns and immutable data structures in TypeScript/JavaScript
Use React functional components with hooks instead of class components in TypeScript
Use React hooks and Context API for state management (migrating away from legacy Redux/Immutable.js)
Use existing hooks fromconsole-sharedwhen possible (useK8sWatchResource,useUserSettings, etc.)
Use k8s resource hooks for data fetching andconsoleFetchJSONfor HTTP requests in TypeScript
Place plugin routes in plugin-specific route files
Check existing types inconsole-sharedbefore creating new types
Use SCSS modules co-located with components, PatternFly design system components, and avoid any SCSS/CSS if possible
Follow WCAG 2.1 AA standards for accessibility; use semantic HTML, ARIA labels where needed, ensure keyboard navigation, and test with screen readers
UseuseTranslation('namespace')hook withkeyformat for translation keys in TypeScript
Use ErrorBoundary components and graceful degradation patterns for error handling in TypeScript
UseuseCallbackto memoize callbacks and prevent unnecessary re-renders in React
UseuseMemofor expensive filtering and computations to prevent re-computation on every render
UseReact.lazy()to lazy load heavy components
Avoid using theanytype in TypeScript; suggest proper type definitions instead
Check that null/undefined are properly handled in TypeScript (e.g.,string | undefined)
Verify exported types for reusable components in TypeScript
Reuse types from existing components rather than duplicating type definitions in component props
UseusePluginInfohook for plugin data in TypeScript
Avoid deprecated components; check for JSDoc@deprecatedtags, import paths containing/deprecated, andDEPRECATED_file name prefixes
Use direct imports to specific files instead of barrel exports (index.ts) to avoid circular dependency cycles and improve build performance
Useimport typefor importing type...
Files:
frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx
frontend/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (README.md)
Follow internationalization guidelines as documented in INTERNATIONALIZATION.md for all frontend code
Files:
frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx
**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (INTERNATIONALIZATION.md)
**/*.{ts,tsx,jsx}: Thearia-label,aria-placeholder,aria-roledescription, andaria-valuetextattributes should be internationalized.
The optionali18nKeyproperty on the react-i18next Trans component should only be used as a last resort when the parser incorrectly generates keys containing HTML tags.
Files:
frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx
🔇 Additional comments (1)
frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx (1)
165-174: LGTM!Also applies to: 202-212
| const pvcNotFound = pvcResourceLoaded && !pvcResource; | ||
| const [scResource, scResourceLoaded, scResourceLoadError] = useK8sGet<StorageClassResourceKind>( | ||
| StorageClassModel, | ||
| pvcStorageClassName, | ||
| ); | ||
|
|
||
| // Form is ready when either: | ||
| // - PVC was found and its StorageClass has loaded | ||
| // - PVC was not found (deleted) — annotations and snapshot status provide sufficient data | ||
| const formReady = pvcNotFound || (!!pvcStorageClassName && scResourceLoaded); | ||
|
|
There was a problem hiding this comment.
Gate pvcNotFound on an explicit 404 signal.
Line 83 currently treats any “loaded but no PVC object” state as deleted. That can misclassify non-404 failures (RBAC/API/transient) as deletion and set formReady true incorrectly.
Suggested fix
- const pvcNotFound = pvcResourceLoaded && !pvcResource;
+ const pvcNotFound = pvcResourceLoaded && !pvcResource && pvcResourceLoadError?.code === 404;If this hook surfaces HTTP status via status instead of code, use that field instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pvcNotFound = pvcResourceLoaded && !pvcResource; | |
| const [scResource, scResourceLoaded, scResourceLoadError] = useK8sGet<StorageClassResourceKind>( | |
| StorageClassModel, | |
| pvcStorageClassName, | |
| ); | |
| // Form is ready when either: | |
| // - PVC was found and its StorageClass has loaded | |
| // - PVC was not found (deleted) — annotations and snapshot status provide sufficient data | |
| const formReady = pvcNotFound || (!!pvcStorageClassName && scResourceLoaded); | |
| const pvcNotFound = pvcResourceLoaded && !pvcResource && pvcResourceLoadError?.code === 404; | |
| const [scResource, scResourceLoaded, scResourceLoadError] = useK8sGet<StorageClassResourceKind>( | |
| StorageClassModel, | |
| pvcStorageClassName, | |
| ); | |
| // Form is ready when either: | |
| // - PVC was found and its StorageClass has loaded | |
| // - PVC was not found (deleted) — annotations and snapshot status provide sufficient data | |
| const formReady = pvcNotFound || (!!pvcStorageClassName && scResourceLoaded); |
🤖 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/modals/restore-pvc/restore-pvc-modal.tsx`
around lines 83 - 93, The code treats any "loaded but no PVC object"
(pvcNotFound computed from pvcResourceLoaded && !pvcResource) as deletion which
can misclassify non-404 errors; update the logic that computes pvcNotFound to
only be true when the GET hook indicates an explicit 404 (check the hook's error
payload field name, e.g., error.status or error.code) returned by the useK8sGet
call for the PVC (use the same variable holding the error/status), and then use
that gated pvcNotFound when deriving formReady so transient/API/RBAC errors
won't set formReady true.
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@sg00dwin: This pull request references Jira Issue OCPBUGS-59404, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: yapei. 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. |
QA Verification Evidence
Verification Steps
Animated overview (click to expand)
Step 1: VolumeSnapshot list page (pass)
Step 2: VolumeSnapshot detail page (pass)
Step 3: Restore modal - PVC exists / regression check (pass)
Both branches show identical behavior when the parent PVC exists: StorageClass= ⭐ Step 4: Restore modal - PVC deleted / bug fix (pass)This is the key comparison. When the parent PVC is deleted:
Step 5: Actual PVC restore - candidate only (pass)On the candidate branch, clicking Restore successfully created a new PVC
Step 6-7: Storage overview pages (pass)
Step 8: Console error check (pass)Both branches show identical console errors — only expected 404s:
No new console errors introduced by the fix. Warning This verification was performed by an AI agent. Results may contain false positives or miss Automated QA verification by Claude Code |
|
/verified by claude |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, sg00dwin 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 |
openshift-ci
Bot
added
the
approved
openshift-ci-robot
added
the
verified
|
@logonoff: This PR has been marked as verified by 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. |
|
@sg00dwin: 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. |
|
@sg00dwin: Jira Issue Verification Checks: Jira Issue OCPBUGS-59404 Jira Issue OCPBUGS-59404 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
3 participants
Analysis / Root cause:
The "Restore as new PVC" modal for VolumeSnapshots gates the StorageClass dropdown and Size input on
pvcStorageClassName— a value derived exclusively from the parent PVC. When the parent PVC is deleted,useK8sGetreturns a 404,pvcStorageClassNamebecomesundefined, and both fields permanently show skeleton loaders. The Restore button stays disabled because no storage class can be selected.The snapshot already carries all necessary metadata via annotations (storage class, access modes, volume mode) set at creation time, plus
status.restoreSizefor the size — but the modal's rendering conditionals never fall through to use them.Solution description:
Added two computed booleans to
restore-pvc-modal.tsx:pvcNotFound: detects when the PVC fetch completed with a 404formReady: true when PVC is confirmed deleted OR when PVC and its StorageClass have loadedChanged three rendering conditionals:
!pvcStorageClassName || !scResourceLoaded→!formReady!!pvcStorageClassName && scResourceLoaded→formReady, withisInputDisabledskipping SC-based checks when PVC is not foundWhen the parent PVC exists,
pvcNotFound=falseandformReadyreduces to the original condition — zero behavioral change.Screenshots with fix:
Restore as new PVC form
New PVC details
Test setup:
A cluster with CSI snapshot support (e.g., AWS with
gp3-csiandcsi-aws-vsc). AdjuststorageClassNameandvolumeSnapshotClassNamefor your environment.Test cases:
Regression check (PVC exists):
snapshot-restore-testtest-snapshot> "Restore as new PVC"gp3-csi, Access mode and Volume mode selectors render, Size shows1 GiB, Restore button is enabledBug fix verification (PVC deleted):
test-snapshot> "Restore as new PVC"gp3-csi), Access mode and Volume mode selectors render, Size shows1 GiB, Restore button is enabledCleanup:
Summary by CodeRabbit