-
Notifications
You must be signed in to change notification settings - Fork 57
WIP: CNV-77599: don't connect VMs on IPv6 single-stack cluster to Pod network #3432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@adamviktora: This pull request references CNV-77599 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.22.0" version, but no target version was set. 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 APPROVED This pull-request has been approved by: adamviktora 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 |
📝 WalkthroughWalkthroughAdds IPv6 single‑stack detection and behavior: new hook and model, UI warnings and translations, and changes to VM generation and network selection to omit Pod networking when cluster is IPv6 single‑stack. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
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 |
|
@adamviktora: This pull request references CNV-77599 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.22.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/components/NetworkInterfaceModal/components/NetworkInterfaceNetworkSelect/NetworkInterfaceNetworkSelect.tsx (1)
117-125:⚠️ Potential issue | 🟠 MajorBlock submit when IPv6 single‑stack removes Pod networking and no NADs exist.
Line 123 currently treats
!hasPodNetworkas sufficient to allow creation, but in IPv6 single‑stack the pod option is removed, so the form can be “valid” with zero options. Please tie eligibility to the actual option availability.🛠️ Suggested fix
- const canCreateNetworkInterface = useMemo( - () => hasNads || !hasPodNetwork, - [hasNads, hasPodNetwork], - ); + const canCreateNetworkInterface = useMemo( + () => hasNads || isPodNetworkingOptionExists, + [hasNads, isPodNetworkingOptionExists], + );
🤖 Fix all issues with AI agents
In `@src/utils/hooks/useIsIPv6SingleStackCluster.ts`:
- Around line 23-30: The computed isIPv6SingleStack in useIsIPv6SingleStack can
be undefined when networkConfig or its nested properties are missing; change the
useMemo to coerce the result to a boolean (e.g., wrap the existing expression
with Boolean(...) or add a fallback `|| false`) so the hook always returns a
boolean, referencing the isIPv6SingleStack variable and the
networkConfig?.status?.clusterNetwork/serviceNetwork checks within the useMemo
in useIsIPv6SingleStack.
In `@src/views/catalog/utils/quick-create-vm.ts`:
- Around line 101-109: The current block in isIPv6SingleStack can throw if
getInterfaces(draftVM) or getNetworks(draftVM) returns undefined; change the
assignments to defensively handle undefined by treating results as empty arrays
(e.g., use a fallback like (getInterfaces(draftVM) || []) and
(getNetworks(draftVM) || []) before calling .filter) so
draftVM.spec.template.spec.domain.devices.interfaces and
draftVM.spec.template.spec.networks are always set to arrays, then keep setting
draftVM.spec.template.spec.domain.devices.autoattachPodInterface = false;
reference getInterfaces, getNetworks, isPodNetwork, DEFAULT_NETWORK_INTERFACE
and the draftVM.spec.template.spec.domain.devices.* properties when making the
fix.
1bc3b18 to
1a4d6cf
Compare
|
@adamviktora: This pull request references CNV-77599 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.22.0" version, but no target version was set. 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. |
|
@adamviktora: This pull request references CNV-77599 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.22.0" version, but no target version was set. 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. |
1a4d6cf to
a170498
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/virtualmachines/navigator/VirtualMachineNavigator.tsx (1)
42-72:⚠️ Potential issue | 🟠 MajorMissing
isIPv6SingleStackin useMemo dependency array.
isIPv6SingleStackis used inside theuseMemocallback (line 47) but is not included in the dependency array (line 71). IfisIPv6SingleStackchanges after initial load (e.g., once the Network resource is fetched), the memoized value won't update, potentially showing an incorrect initial VM template.🐛 Proposed fix
return useMemo( () => location.pathname.endsWith(`${VirtualMachineModelRef}/~new`) ? ( <CreateResourceDefaultPage header={t('Create VirtualMachine')} initialResource={defaultVMYamlTemplate(isIPv6SingleStack)} /> ) : ( // ... rest of JSX ), - [isVirtualMachineListPage, cluster, location.pathname, namespace, t, treeProps], + [isVirtualMachineListPage, cluster, location.pathname, namespace, t, treeProps, isIPv6SingleStack, onFilterChange], );
🤖 Fix all issues with AI agents
In
`@src/views/catalog/templatescatalog/components/TemplatesCatalogDrawer/hooks/useCreateDrawerForm.tsx`:
- Around line 304-312: The code assumes getInterfaces(vmDraft) and
getNetworks(vmDraft) always return arrays; to prevent TypeError when they return
undefined, change the filtering to operate on a safe fallback array (e.g.,
(getInterfaces(vmDraft) ?? []) and (getNetworks(vmDraft) ?? [])) before calling
.filter(), and then assign those results to
vmDraft.spec.template.spec.domain.devices.interfaces and
vmDraft.spec.template.spec.networks; reference isIPv6SingleStack, vmDraft,
getInterfaces, getNetworks, DEFAULT_NETWORK_INTERFACE, and isPodNetwork when
making this defensive change.
🧹 Nitpick comments (1)
src/views/catalog/templatescatalog/components/TemplatesCatalogDrawer/TemplateInfoSection.tsx (1)
136-145: Move the Alert outside the DescriptionList.PatternFly DescriptionList is a
<dl>and expects only DescriptionListGroup children. Placing a raw Alert as a direct child breaks semantic HTML structure. Render it after the DescriptionList instead.♻️ Possible structure
- <DescriptionList> - ... - {isIPv6SingleStack && hasPodNetwork && ( - <Alert - isInline - title={t("Can't use Pod networking in IPv6 single-stack cluster")} - variant="warning" - > - {networks.length === 1 && - t('Click Customize VirtualMachine to add a different network interface')} - </Alert> - )} - ... - </DescriptionList> + <DescriptionList> + ... + </DescriptionList> + {isIPv6SingleStack && hasPodNetwork && ( + <Alert + isInline + title={t("Can't use Pod networking in IPv6 single-stack cluster")} + variant="warning" + > + {networks.length === 1 && + t('Click Customize VirtualMachine to add a different network interface')} + </Alert> + )}
...ews/catalog/templatescatalog/components/TemplatesCatalogDrawer/hooks/useCreateDrawerForm.tsx
Show resolved
Hide resolved
|
@adamviktora: This pull request references CNV-77599 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.22.0" version, but no target version was set. 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. |
|
Waiting for backend |
📝 Description
useIsIPv6SingleStackClusterhook to check if a cluster is IPv6 single-stackkind: Network apiVersion: config.openshift.io/v1 metadata.name: clusterFor an IPv6 single-stack cluster:
🎥 Demo
After:
Screen.Recording.2026-02-04.at.9.45.38.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Localization