Skip to content

Conversation

@galkremer1
Copy link
Member

@galkremer1 galkremer1 commented Nov 3, 2025

📝 Description

Adds VMI labels as a fallback in getServicesForVmi to improve service matching when pods are unavailable.

  • Updated getServicesForVmi to accept optional vmi parameter
  • Added VMI labels check in fallback chain: pod labels → VMI labels → VM template labels → VM name
  • Updated call sites to pass VMI when available

KubeVirt v1.7+ introduces vmi.kubevirt.io/id on pods for service matching. Labels like vm.kubevirt.io/name can also exist on the VMI resource (e.g., manually set or inherited from templates).
Checking VMI labels ensures matching when the pod is not available (stopped VMs, etc.).

Jira ticket: CNV-71151
KubeVirt PR #15783 introducing the change

🎥 Demo

In addition to adding the support for the new selector (vmi.kubevirt.io/id), this PR also fixes a bug where the services are not listed under the VMI:

Before:

Before.mov

After:

After.mov

Summary by CodeRabbit

  • Bug Fixes

    • Improved RDP and SSH service discovery by making label selection pod-aware and strengthening fallbacks when pod/VMI context is missing.
  • Refactor

    • Centralized and optimized service lookup and label handling for VM consoles and access paths to ensure consistent selector generation.
  • Tests

    • Updated unit tests to cover pod-aware selector logic and new fallback scenarios.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Nov 3, 2025

@galkremer1: This pull request references CNV-71151 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 story to target the "4.21.0" version, but no target version was set.

Details

In response to this:

📝 Description

Adds VMI labels as a fallback in getServicesForVmi to improve service matching when pods are unavailable.

  • Updated getServicesForVmi to accept optional vmi parameter
  • Added VMI labels check in fallback chain: pod labels → VMI labels → VM template labels → VM name
  • Updated call sites to pass VMI when available

KubeVirt v1.7+ introduces vmi.kubevirt.io/id on pods for service matching. Labels like vm.kubevirt.io/name can also exist on the VMI resource (e.g., manually set or inherited from templates).
Checking VMI labels ensures matching when the pod is not available (stopped VMs, etc.).

Jira ticket: CNV-71151
KubeVirt PR #15783 introducing the change

🎥 Demo

In addition to adding the support for the new selector (vmi.kubevirt.io/id), this PR also fixes a bug where the services are not listed under the VMI:

Before:

Before.mov

After:

After.mov

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
Copy link
Contributor

openshift-ci bot commented Nov 3, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: galkremer1

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved This issue is something we want to fix label Nov 3, 2025
@coderabbitai
Copy link

coderabbitai bot commented Nov 3, 2025

Walkthrough

Services lookup and creation for RDP/SSH were made pod-aware: components fetch pods and derive pod-based label selectors; utility functions and service builders accept optional pod parameters and prefer pod-derived labels, with fallbacks to VMI/VM labels.

Changes

Cohort / File(s) Change Summary
RDP modal
src/utils/components/Consoles/components/DesktopViewer/Components/RDPServiceModal.tsx
Retrieve pods via usePods/getVMIPod; derive pod before submit; pass pod to createRDPService; rename local checkbox setter to setIsChecked.
RDP utils
src/utils/components/Consoles/components/DesktopViewer/utils/utils.ts
Added optional pod param to findVMServiceWithPort, findRDPServiceAndPort, createRDPService; compute service selector from pod via getVMILabelForServiceSelector when available; use dynamic labelKey/labelValue for VM/VMI patches and Service selector.
SSH components
src/utils/components/SSHAccess/components/SSHCommand.tsx, src/utils/components/SSHAccess/useSSHService.tsx
Use useVMIAndPodsForVM and getVMIPod to obtain pod; compute label selector via getVMILabelForServiceSelector if pod exists; call createSSHService and addSSHSelectorLabelToVM with pod/labelSelector.
SSH utils
src/utils/components/SSHAccess/utils.ts
buildSSHServiceFromVM and createSSHService accept optional pod; label selector derived from pod when present; addSSHSelectorLabelToVM now accepts {labelKey,labelValue} selector object and updates VM/VMI labels accordingly; service spec selector uses computed selector.
VMI service core
src/utils/resources/vmi/utils/services.ts
Added constants VMI_ID_LABEL, VM_NAME_LABEL; new getVMILabelForServiceSelector(pod, vm) returning {labelKey,labelValue}; getServicesForVmi now accepts (services, pod?, vm?, vmi?) and matches services against pod/VMI/VM-derived labels with VMI_ID fallback handling.
Service tests
src/utils/resources/vmi/utils/tests/services.test.ts
Added createMockPod, createMockVM, createMockVMI; updated tests to call getServicesForVmi(services, pod, vm, vmi); added scenarios covering vmi.kubevirt.io/id selector and pod/no-pod fallbacks.
Overview & Services UI
src/views/virtualmachines/details/tabs/overview/components/VirtualMachinesOverviewTabService/VirtualMachinesOverviewTabService.tsx, src/views/virtualmachinesinstance/details/tabs/details/components/Services/Services.tsx
Use useVMIAndPodsForVM/getVMIPod to derive pod; call getServicesForVmi with pod (and vm/vmi) instead of purely label-based inputs.

Sequence Diagram(s)

sequenceDiagram
    participant Component
    participant useVMIAndPodsForVM
    participant getVMIPod
    participant getVMILabelForServiceSelector
    participant ServiceUtils
    participant K8sAPI

    rect rgb(240,248,255)
    Note over Component,ServiceUtils: Pod-aware resolution flow
    Component->>useVMIAndPodsForVM: request vmi + pods for VM
    useVMIAndPodsForVM-->>Component: {vmi, pods}
    Component->>getVMIPod: getVMIPod(vmi, pods)
    getVMIPod-->>Component: pod (optional)
    alt pod exists
        Component->>getVMILabelForServiceSelector: derive {labelKey,labelValue} from pod
        getVMILabelForServiceSelector-->>Component: {labelKey,labelValue}
        Component->>ServiceUtils: find/create service with pod and selector
    else no pod
        Component->>ServiceUtils: find/create service using vmi/vm labels
    end
    ServiceUtils->>K8sAPI: create/patch Service, patch VM/VMI labels (using labelKey)
    K8sAPI-->>ServiceUtils: result
    ServiceUtils-->>Component: service created / port resolved
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Inspect signature changes: ensure all callers updated for createRDPService, createSSHService, findVMServiceWithPort, findRDPServiceAndPort.
  • Verify label-key/label-value derivation and correctness in getVMILabelForServiceSelector and its priority order.
  • Confirm VM/VMI patch paths use dynamic labelKey consistently (no hard-coded keys left).
  • Review tests in services.test.ts for adequate coverage of pod/no-pod and VMI_ID fallback scenarios.
  • Check type usage for Pod objects across utilities (imports and optionality).

Poem

🐰 I sniffed the pods beneath the tree,
Labels whispered secrets just for me.
I hopped and patched with careful art,
Services found — no longer apart.
A rabbit cheers: pod-aware, smart! 🎉

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: updating service label selectors to use vmi.kubevirt.io/id for KubeVirt v1.7+ compatibility, which aligns with the core objective of the changeset.
Description check ✅ Passed The description provides a clear explanation of the change, rationale, and includes demo assets showing before/after behavior. However, it does not follow the provided template structure with the specified sections.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7367b9d and da9afd5.

📒 Files selected for processing (9)
  • src/utils/components/Consoles/components/DesktopViewer/Components/RDPServiceModal.tsx (4 hunks)
  • src/utils/components/Consoles/components/DesktopViewer/utils/utils.ts (6 hunks)
  • src/utils/components/SSHAccess/components/SSHCommand.tsx (4 hunks)
  • src/utils/components/SSHAccess/useSSHService.tsx (2 hunks)
  • src/utils/components/SSHAccess/utils.ts (4 hunks)
  • src/utils/resources/vmi/utils/services.ts (1 hunks)
  • src/utils/resources/vmi/utils/tests/services.test.ts (2 hunks)
  • src/views/virtualmachines/details/tabs/overview/components/VirtualMachinesOverviewTabService/VirtualMachinesOverviewTabService.tsx (2 hunks)
  • src/views/virtualmachinesinstance/details/tabs/details/components/Services/Services.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/views/virtualmachines/details/tabs/overview/components/VirtualMachinesOverviewTabService/VirtualMachinesOverviewTabService.tsx
  • src/views/virtualmachinesinstance/details/tabs/details/components/Services/Services.tsx
  • src/utils/components/SSHAccess/useSSHService.tsx
  • src/utils/components/Consoles/components/DesktopViewer/Components/RDPServiceModal.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
src/utils/resources/vmi/utils/tests/services.test.ts (1)
src/utils/resources/vmi/utils/services.ts (1)
  • getServicesForVmi (54-99)
src/utils/components/SSHAccess/utils.ts (4)
src/utils/resources/vmi/utils/services.ts (1)
  • getVMILabelForServiceSelector (17-45)
src/utils/components/SSHAccess/constants.ts (3)
  • VM_LABEL_AS_SSH_SERVICE_SELECTOR (6-6)
  • PORT (3-3)
  • SSH_PORT (4-4)
src/utils/utils/utils.ts (2)
  • getRandomChars (73-78)
  • ensurePath (119-132)
src/utils/resources/shared.ts (1)
  • buildOwnerReference (157-167)
src/utils/components/SSHAccess/components/SSHCommand.tsx (5)
src/utils/resources/vm/hooks/useVMIAndPodsForVM.ts (1)
  • useVMIAndPodsForVM (14-37)
src/multicluster/helpers/selectors.ts (1)
  • getCluster (6-7)
src/utils/resources/vmi/utils/services.ts (1)
  • getVMILabelForServiceSelector (17-45)
src/utils/components/SSHAccess/constants.ts (1)
  • VM_LABEL_AS_SSH_SERVICE_SELECTOR (6-6)
src/utils/components/SSHAccess/utils.ts (2)
  • addSSHSelectorLabelToVM (65-101)
  • createSSHService (103-116)
src/utils/components/Consoles/components/DesktopViewer/utils/utils.ts (2)
src/utils/resources/vmi/utils/services.ts (2)
  • VMI_ID_LABEL (8-8)
  • getVMILabelForServiceSelector (17-45)
src/utils/components/Consoles/components/DesktopViewer/utils/constants.ts (2)
  • DEFAULT_RDP_PORT (1-1)
  • VMI_LABEL_AS_RDP_SERVICE_SELECTOR (7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build
  • GitHub Check: i18n
  • GitHub Check: unit-test

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Nov 3, 2025

@galkremer1: This pull request references CNV-71151 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 story to target the "4.21.0" version, but no target version was set.

Details

In response to this:

📝 Description

Adds VMI labels as a fallback in getServicesForVmi to improve service matching when pods are unavailable.

  • Updated getServicesForVmi to accept optional vmi parameter
  • Added VMI labels check in fallback chain: pod labels → VMI labels → VM template labels → VM name
  • Updated call sites to pass VMI when available

KubeVirt v1.7+ introduces vmi.kubevirt.io/id on pods for service matching. Labels like vm.kubevirt.io/name can also exist on the VMI resource (e.g., manually set or inherited from templates).
Checking VMI labels ensures matching when the pod is not available (stopped VMs, etc.).

Jira ticket: CNV-71151
KubeVirt PR #15783 introducing the change

🎥 Demo

In addition to adding the support for the new selector (vmi.kubevirt.io/id), this PR also fixes a bug where the services are not listed under the VMI:

Before:

Before.mov

After:

After.mov

Summary by CodeRabbit

  • Bug Fixes

  • Improved service discovery for SSH and RDP connections by using pod-level identifiers for more reliable service matching.

  • Refactor

  • Enhanced internal service selection logic to leverage pod information alongside virtual machine metadata for better service integration.

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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
src/utils/components/Consoles/components/DesktopViewer/utils/constants.ts (1)

8-8: Duplicate constant definition.

This is the same duplication issue flagged in src/utils/components/SSHAccess/constants.ts. Please consolidate to a single source of truth.

🧹 Nitpick comments (2)
src/utils/components/SSHAccess/components/SSHCommand.tsx (1)

49-50: Consider using the error and loaded states from useVMIAndPodsForVM.

The hook returns { error, loaded, pods, vmi } but only pods and vmi are destructured. While the pod derivation logic will handle missing data gracefully, explicitly checking error or loaded could provide better user feedback or prevent unnecessary operations when data is still loading or has failed to load.

Consider this refactor if you want to add explicit error handling:

- const { pods, vmi } = useVMIAndPodsForVM(getName(vm), getNamespace(vm), getCluster(vm));
+ const { pods, vmi, loaded: vmiAndPodsLoaded, error: vmiAndPodsError } = useVMIAndPodsForVM(getName(vm), getNamespace(vm), getCluster(vm));
  const pod = getVMIPod(vmi, pods);

Then you could use these states to conditionally render or handle errors.

src/utils/components/Consoles/components/DesktopViewer/utils/utils.ts (1)

37-74: Verify consistency with getServicesForVmi matching logic.

The findVMServiceWithPort function implements pod-aware service matching with special handling for VMI_ID_LABEL (lines 59-61) and TEMPLATE_VM_NAME_LABEL (lines 64-66). This logic is similar but not identical to the matching logic in getServicesForVmi (src/utils/resources/vmi/utils/services.ts, lines 86-97).

Differences observed:

  • Line 65: Falls back to vmiName === selectorValue for TEMPLATE_VM_NAME_LABEL
  • The getServicesForVmi function has a more general approach that checks all selectors against labelsToMatch

While both implementations appear correct for their specific contexts, having duplicate matching logic increases maintenance burden and the risk of inconsistency.

Consider refactoring to use a shared matching utility or ensuring both implementations stay aligned, especially if the backward compatibility logic needs to evolve.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 56e53d9 and 6606a21.

📒 Files selected for processing (11)
  • src/utils/components/Consoles/components/DesktopViewer/Components/RDPServiceModal.tsx (4 hunks)
  • src/utils/components/Consoles/components/DesktopViewer/utils/constants.ts (1 hunks)
  • src/utils/components/Consoles/components/DesktopViewer/utils/utils.ts (7 hunks)
  • src/utils/components/SSHAccess/components/SSHCommand.tsx (4 hunks)
  • src/utils/components/SSHAccess/constants.ts (1 hunks)
  • src/utils/components/SSHAccess/useSSHService.tsx (2 hunks)
  • src/utils/components/SSHAccess/utils.ts (3 hunks)
  • src/utils/resources/vmi/utils/services.ts (1 hunks)
  • src/utils/resources/vmi/utils/tests/services.test.ts (2 hunks)
  • src/views/virtualmachines/details/tabs/overview/components/VirtualMachinesOverviewTabService/VirtualMachinesOverviewTabService.tsx (2 hunks)
  • src/views/virtualmachinesinstance/details/tabs/details/components/Services/Services.tsx (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
src/utils/components/SSHAccess/constants.ts (2)
src/utils/components/Consoles/components/DesktopViewer/utils/constants.ts (1)
  • VMI_ID_LABEL (8-8)
src/utils/resources/vmi/utils/services.ts (1)
  • VMI_ID_LABEL (8-8)
src/views/virtualmachines/details/tabs/overview/components/VirtualMachinesOverviewTabService/VirtualMachinesOverviewTabService.tsx (3)
src/utils/resources/vm/hooks/useVMIAndPodsForVM.ts (1)
  • useVMIAndPodsForVM (14-37)
src/multicluster/helpers/selectors.ts (1)
  • getCluster (6-7)
src/utils/resources/vmi/utils/services.ts (1)
  • getServicesForVmi (54-99)
src/utils/components/SSHAccess/utils.ts (4)
src/utils/resources/vmi/utils/services.ts (1)
  • getVMILabelForServiceSelector (17-45)
src/utils/components/SSHAccess/constants.ts (3)
  • VM_LABEL_AS_SSH_SERVICE_SELECTOR (6-6)
  • PORT (3-3)
  • SSH_PORT (4-4)
src/utils/utils/utils.ts (1)
  • getRandomChars (73-78)
src/utils/resources/shared.ts (1)
  • buildOwnerReference (157-167)
src/utils/components/SSHAccess/useSSHService.tsx (3)
src/utils/resources/vm/hooks/useVMIAndPodsForVM.ts (1)
  • useVMIAndPodsForVM (14-37)
src/multicluster/helpers/selectors.ts (1)
  • getCluster (6-7)
src/utils/resources/vmi/utils/services.ts (1)
  • getServicesForVmi (54-99)
src/utils/resources/vmi/utils/tests/services.test.ts (1)
src/utils/resources/vmi/utils/services.ts (1)
  • getServicesForVmi (54-99)
src/utils/components/SSHAccess/components/SSHCommand.tsx (4)
src/utils/resources/vm/hooks/useVMIAndPodsForVM.ts (1)
  • useVMIAndPodsForVM (14-37)
src/multicluster/helpers/selectors.ts (1)
  • getCluster (6-7)
src/utils/resources/vmi/utils/services.ts (1)
  • getVMILabelForServiceSelector (17-45)
src/utils/components/SSHAccess/utils.ts (2)
  • addSSHSelectorLabelToVM (65-99)
  • createSSHService (101-114)
src/utils/components/Consoles/components/DesktopViewer/utils/constants.ts (2)
src/utils/components/SSHAccess/constants.ts (1)
  • VMI_ID_LABEL (7-7)
src/utils/resources/vmi/utils/services.ts (1)
  • VMI_ID_LABEL (8-8)
src/utils/resources/vmi/utils/services.ts (2)
src/utils/components/Consoles/components/DesktopViewer/utils/constants.ts (1)
  • VMI_ID_LABEL (8-8)
src/utils/components/SSHAccess/constants.ts (1)
  • VMI_ID_LABEL (7-7)
src/utils/components/Consoles/components/DesktopViewer/Components/RDPServiceModal.tsx (1)
src/utils/components/Consoles/components/DesktopViewer/utils/utils.ts (1)
  • createRDPService (259-329)
src/utils/components/Consoles/components/DesktopViewer/utils/utils.ts (2)
src/utils/components/Consoles/components/DesktopViewer/utils/constants.ts (4)
  • VMI_ID_LABEL (8-8)
  • TEMPLATE_VM_NAME_LABEL (2-2)
  • DEFAULT_RDP_PORT (1-1)
  • VMI_LABEL_AS_RDP_SERVICE_SELECTOR (7-7)
src/utils/resources/vmi/utils/services.ts (2)
  • VMI_ID_LABEL (8-8)
  • getVMILabelForServiceSelector (17-45)
src/views/virtualmachinesinstance/details/tabs/details/components/Services/Services.tsx (1)
src/utils/resources/vmi/utils/services.ts (1)
  • getServicesForVmi (54-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: unit-test
  • GitHub Check: i18n
  • GitHub Check: build
🔇 Additional comments (16)
src/utils/components/SSHAccess/useSSHService.tsx (1)

27-36: LGTM! Pod-aware service resolution implemented correctly.

The changes properly integrate pod context into SSH service discovery:

  1. Fetches VMI and pods using useVMIAndPodsForVM
  2. Derives the VM's pod via getVMIPod
  3. Passes pod context to getServicesForVmi for label-based matching

This enables support for the new vmi.kubevirt.io/id label selector in KubeVirt v1.7+ while maintaining backward compatibility.

src/views/virtualmachines/details/tabs/overview/components/VirtualMachinesOverviewTabService/VirtualMachinesOverviewTabService.tsx (1)

28-31: LGTM! Consistent pod-aware service resolution.

The implementation correctly follows the same pattern as other components in this PR:

  • Fetches VMI and pods
  • Derives the VM's pod
  • Uses pod labels for service matching
src/views/virtualmachinesinstance/details/tabs/details/components/Services/Services.tsx (1)

23-25: LGTM! Correct implementation for VMI context.

The changes properly handle the VMI-only context (no VM reference available on this page). The fallback chain in getServicesForVmi will use pod labels when available, otherwise VMI labels.

src/utils/components/Consoles/components/DesktopViewer/Components/RDPServiceModal.tsx (2)

32-33: LGTM! Pod context added to RDP service creation.

The changes enable pod-aware label selection for RDP services, consistent with SSH service changes.


30-30: Minor naming improvement.

Renaming setChecked to setIsChecked improves clarity by aligning with the boolean state naming convention.

src/utils/resources/vmi/utils/tests/services.test.ts (2)

13-59: Well-structured test helpers.

The mock factory functions (createMockPod, createMockVM, createMockVMI) provide clean, reusable test data construction.


112-152: Excellent test coverage for new functionality.

The new test cases comprehensively verify:

  1. vmi.kubevirt.io/id selector matching (KubeVirt v1.7+)
  2. Fallback to VMI labels when pod unavailable
  3. Fallback to VM name when both pod and VMI unavailable

This ensures backward compatibility while supporting the new label selector.

src/utils/components/SSHAccess/utils.ts (2)

18-55: LGTM! Dynamic selector logic correctly implemented.

The refactored buildSSHServiceFromVM function now:

  1. Accepts an optional pod parameter
  2. Uses getVMILabelForServiceSelector to derive the appropriate label key/value from pod context (prioritizing vmi.kubevirt.io/id when available)
  3. Falls back to vm.kubevirt.io/name with VM name when pod unavailable

This enables KubeVirt v1.7+ compatibility while maintaining backward compatibility.


101-114: LGTM! Signature correctly updated.

The createSSHService function signature properly propagates the optional pod parameter through the service creation flow.

src/utils/components/SSHAccess/components/SSHCommand.tsx (2)

62-64: LGTM! Proper fallback chain for label derivation.

The logic correctly derives the labelValue from the pod when available (using getVMILabelForServiceSelector) and falls back to the VM name otherwise. This aligns with the PR's goal of supporting both KubeVirt v1.7+ (with vmi.kubevirt.io/id) and older versions.


66-66: LGTM! Pod parameter correctly passed to createSSHService.

The pod is now passed as the third argument to createSSHService, enabling pod-based label selector logic in service creation.

src/utils/resources/vmi/utils/services.ts (2)

17-45: LGTM! Well-structured label resolution with clear priority chain.

The function correctly implements the priority chain:

  1. vmi.kubevirt.io/id from pod (KubeVirt v1.7+)
  2. vm.kubevirt.io/name from pod (fallback)
  3. VM name (ultimate fallback)

The inline comments clearly document the intent, and the logic handles missing pod gracefully.


60-78: LGTM! Comprehensive fallback chain for label matching.

The priority chain for labelsToMatch is well-structured:

  1. Pod labels (running VMs)
  2. VMI labels (when pod unavailable)
  3. VM template labels
  4. VM name (ultimate fallback)

This aligns with the PR's objectives to support both running and stopped VMs, with appropriate fallbacks at each level.

src/utils/components/Consoles/components/DesktopViewer/utils/utils.ts (3)

79-79: LGTM! Pod parameter correctly threaded through the call chain.

The pod parameter is properly passed from findRDPServiceAndPortfindVMServiceWithPort, enabling pod-aware service matching for RDP services.


266-276: LGTM! Dynamic label selector construction with proper fallback.

The logic correctly:

  1. Uses getVMILabelForServiceSelector(pod, vm) when pod is available to get the optimal label (preferring vmi.kubevirt.io/id)
  2. Falls back to VMI_LABEL_AS_RDP_SERVICE_SELECTOR with VM template labels or VM name when pod is unavailable

This aligns with the PR's objectives for KubeVirt v1.7+ compatibility while maintaining backward compatibility.


282-282: LGTM! Correct JSON Pointer escaping for patch paths.

The use of replaceAll('/', '~1') correctly escapes forward slashes in the label key for JSON Patch paths according to RFC 6901 (JSON Pointer specification). This is necessary because label keys like vmi.kubevirt.io/id contain forward slashes that must be escaped.

Also applies to: 294-294

@galkremer1 galkremer1 force-pushed the CNV-71151-update-service-label branch from 6606a21 to 7367b9d Compare November 3, 2025 19:31
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Nov 3, 2025

@galkremer1: This pull request references CNV-71151 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 story to target the "4.21.0" version, but no target version was set.

Details

In response to this:

📝 Description

Adds VMI labels as a fallback in getServicesForVmi to improve service matching when pods are unavailable.

  • Updated getServicesForVmi to accept optional vmi parameter
  • Added VMI labels check in fallback chain: pod labels → VMI labels → VM template labels → VM name
  • Updated call sites to pass VMI when available

KubeVirt v1.7+ introduces vmi.kubevirt.io/id on pods for service matching. Labels like vm.kubevirt.io/name can also exist on the VMI resource (e.g., manually set or inherited from templates).
Checking VMI labels ensures matching when the pod is not available (stopped VMs, etc.).

Jira ticket: CNV-71151
KubeVirt PR #15783 introducing the change

🎥 Demo

In addition to adding the support for the new selector (vmi.kubevirt.io/id), this PR also fixes a bug where the services are not listed under the VMI:

Before:

Before.mov

After:

After.mov

Summary by CodeRabbit

  • Bug Fixes

  • Improved service discovery reliability for RDP and SSH access through enhanced label matching.

  • Strengthened fallback logic to ensure services remain accessible when pod information is unavailable.

  • Refactor

  • Optimized service lookup mechanisms for virtual machine console and access services.

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.

Copy link

@coderabbitai coderabbitai bot left a 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/utils/components/SSHAccess/utils.ts (1)

65-98: addSSHSelectorLabelToVM must honor the selector key returned from the pod.
This helper still writes every update under VM_LABEL_AS_SSH_SERVICE_SELECTOR (vm.kubevirt.io/name). When the caller passes a pod-derived selector (vmi.kubevirt.io/id), we end up overwriting the VM/VMI “name” label with the pod’s ID and never set the actual selector key that the new Service expects. That breaks existing consumers of vm.kubevirt.io/name and leaves the Service selector unsatisfied for future pods. Please update the function (and its callers) to accept both labelKey and labelValue, and write them verbatim:

-export const addSSHSelectorLabelToVM = async (
-  vm: V1VirtualMachine,
-  vmi: V1VirtualMachineInstance,
-  labelValue,
-) => {
+export const addSSHSelectorLabelToVM = async (
+  vm: V1VirtualMachine,
+  vmi: V1VirtualMachineInstance,
+  labelKey: string,
+  labelValue: string,
+) => {
   const vmWithLabel = produce(vm, (draftVM) => {
     ensurePath(draftVM, 'spec.template.metadata.labels');
 
-    draftVM.spec.template.metadata.labels[VM_LABEL_AS_SSH_SERVICE_SELECTOR] = labelValue;
+    draftVM.spec.template.metadata.labels[labelKey] = labelValue;
   });
 
   if (vmi) {
     const vmiWithLabel = produce(vmi, (draftVMI) => {
       ensurePath(draftVMI, 'metadata.labels');
 
-      draftVMI.metadata.labels[VM_LABEL_AS_SSH_SERVICE_SELECTOR] = labelValue;
+      draftVMI.metadata.labels[labelKey] = labelValue;
     });

Make sure any remaining call sites pass the key as well so the VM/VMI labels stay in sync with the Service selector.

♻️ Duplicate comments (1)
src/utils/resources/vmi/utils/services.ts (1)

8-9: Centralize the VMI label constants.
We are still exporting VMI_ID_LABEL and VM_NAME_LABEL here while DesktopViewer and SSHAccess carry identical definitions. Please extract these to a shared constants module (e.g., src/utils/constants/vmiLabels.ts) and import them everywhere to avoid drift. Based on learnings.

🧹 Nitpick comments (1)
src/utils/resources/vmi/utils/tests/services.test.ts (1)

158-175: Add test case for VM template labels fallback.

The fallback tests cover VMI labels (priority 2) and VM name (priority 4), but miss the VM template labels (priority 3) path. Consider adding a test where pod and VMI are undefined, but the VM has spec.template.metadata.labels with the matching label.

Example test case:

it('Fallback to VM template labels when pod and VMI are not provided', () => {
  const services: IoK8sApiCoreV1Service[] = [serviceWithMatchingSelectors];
  const vm: V1VirtualMachine = {
    ...createMockVM('fedora-proposed-rodent'),
    spec: {
      ...createMockVM('fedora-proposed-rodent').spec,
      template: {
        metadata: {
          labels: { 'vm.kubevirt.io/name': 'fedora-proposed-rodent' },
        },
        spec: {
          domain: {
            devices: {},
          },
        },
      },
    },
  };

  const result = getServicesForVmi(services, undefined, vm);

  expect(result).toEqual([serviceWithMatchingSelectors]);
});
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6606a21 and 7367b9d.

📒 Files selected for processing (9)
  • src/utils/components/Consoles/components/DesktopViewer/Components/RDPServiceModal.tsx (4 hunks)
  • src/utils/components/Consoles/components/DesktopViewer/utils/utils.ts (6 hunks)
  • src/utils/components/SSHAccess/components/SSHCommand.tsx (4 hunks)
  • src/utils/components/SSHAccess/useSSHService.tsx (2 hunks)
  • src/utils/components/SSHAccess/utils.ts (3 hunks)
  • src/utils/resources/vmi/utils/services.ts (1 hunks)
  • src/utils/resources/vmi/utils/tests/services.test.ts (2 hunks)
  • src/views/virtualmachines/details/tabs/overview/components/VirtualMachinesOverviewTabService/VirtualMachinesOverviewTabService.tsx (2 hunks)
  • src/views/virtualmachinesinstance/details/tabs/details/components/Services/Services.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/views/virtualmachinesinstance/details/tabs/details/components/Services/Services.tsx
  • src/utils/components/SSHAccess/useSSHService.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
src/utils/components/Consoles/components/DesktopViewer/Components/RDPServiceModal.tsx (1)
src/utils/components/Consoles/components/DesktopViewer/utils/utils.ts (1)
  • createRDPService (261-331)
src/utils/resources/vmi/utils/tests/services.test.ts (1)
src/utils/resources/vmi/utils/services.ts (1)
  • getServicesForVmi (54-99)
src/utils/components/SSHAccess/utils.ts (4)
src/utils/resources/vmi/utils/services.ts (1)
  • getVMILabelForServiceSelector (17-45)
src/utils/components/SSHAccess/constants.ts (3)
  • VM_LABEL_AS_SSH_SERVICE_SELECTOR (6-6)
  • PORT (3-3)
  • SSH_PORT (4-4)
src/utils/utils/utils.ts (1)
  • getRandomChars (73-78)
src/utils/resources/shared.ts (1)
  • buildOwnerReference (157-167)
src/utils/components/Consoles/components/DesktopViewer/utils/utils.ts (2)
src/utils/resources/vmi/utils/services.ts (2)
  • VMI_ID_LABEL (8-8)
  • getVMILabelForServiceSelector (17-45)
src/utils/components/Consoles/components/DesktopViewer/utils/constants.ts (2)
  • DEFAULT_RDP_PORT (1-1)
  • VMI_LABEL_AS_RDP_SERVICE_SELECTOR (7-7)
src/utils/components/SSHAccess/components/SSHCommand.tsx (4)
src/utils/resources/vm/hooks/useVMIAndPodsForVM.ts (1)
  • useVMIAndPodsForVM (14-37)
src/multicluster/helpers/selectors.ts (1)
  • getCluster (6-7)
src/utils/resources/vmi/utils/services.ts (1)
  • getVMILabelForServiceSelector (17-45)
src/utils/components/SSHAccess/utils.ts (2)
  • addSSHSelectorLabelToVM (65-99)
  • createSSHService (101-114)
src/views/virtualmachines/details/tabs/overview/components/VirtualMachinesOverviewTabService/VirtualMachinesOverviewTabService.tsx (3)
src/utils/resources/vm/hooks/useVMIAndPodsForVM.ts (1)
  • useVMIAndPodsForVM (14-37)
src/multicluster/helpers/selectors.ts (1)
  • getCluster (6-7)
src/utils/resources/vmi/utils/services.ts (1)
  • getServicesForVmi (54-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: unit-test
  • GitHub Check: i18n
  • GitHub Check: build
🔇 Additional comments (7)
src/utils/resources/vmi/utils/tests/services.test.ts (3)

13-59: LGTM! Clean and reusable mock helpers.

The mock factory functions create minimal but complete objects that align well with the API types and support flexible test scenarios.


61-110: LGTM! Existing tests properly refactored.

The existing test cases have been correctly updated to use the new API signature with pod and VM parameters while maintaining their original test intent.


112-156: LGTM! KubeVirt v1.7+ ID selector logic properly tested.

These tests correctly verify the new vmi.kubevirt.io/id selector matching with proper backward compatibility fallback to vm.kubevirt.io/name.

src/utils/components/Consoles/components/DesktopViewer/utils/utils.ts (4)

39-76: Clarify the VMI name fallback logic for vm.kubevirt.io/name selector.

At line 67, when the service selector uses vm.kubevirt.io/name, the code falls back to checking vmiName directly:

if (key === TEMPLATE_VM_NAME_LABEL) {
  return labelValue === selectorValue || vmiName === selectorValue;
}

This differs from getServicesForVmi in services.ts, which uses the full priority chain (pod labels → VMI labels → VM template labels → VM name) without special fallback logic for vm.kubevirt.io/name. The direct VMI name fallback here bypasses the standard chain.

Questions:

  1. Is this VMI name fallback intentional for RDP-specific backward compatibility?
  2. Could this cause false positives if the VMI name happens to match the selector but the pod labels don't?

Consider documenting this behavior if intentional, or aligning with the services.ts priority chain if not.


78-138: LGTM! Pod parameter properly threaded through the call chain.

The optional pod parameter is correctly passed from getRdpAddressPort through findRDPServiceAndPort to findVMServiceWithPort, enabling pod-aware service matching for RDP connections.


261-331: LGTM! Dynamic label selector properly implemented.

The createRDPService function correctly uses getVMILabelForServiceSelector when a pod is available to prefer vmi.kubevirt.io/id, with appropriate fallback to vm.kubevirt.io/name from template labels or VM name. The JSON patch path escaping (replaceAll('/', '~1')) correctly follows RFC 6901.


16-19: LGTM! Imports properly added.

The new imports for getVMILabelForServiceSelector and VMI_ID_LABEL are correctly added and used throughout the file to support pod-aware service selection.

@galkremer1 galkremer1 force-pushed the CNV-71151-update-service-label branch from 7367b9d to da9afd5 Compare November 3, 2025 19:43
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Nov 3, 2025

@galkremer1: This pull request references CNV-71151 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 story to target the "4.21.0" version, but no target version was set.

Details

In response to this:

📝 Description

Adds VMI labels as a fallback in getServicesForVmi to improve service matching when pods are unavailable.

  • Updated getServicesForVmi to accept optional vmi parameter
  • Added VMI labels check in fallback chain: pod labels → VMI labels → VM template labels → VM name
  • Updated call sites to pass VMI when available

KubeVirt v1.7+ introduces vmi.kubevirt.io/id on pods for service matching. Labels like vm.kubevirt.io/name can also exist on the VMI resource (e.g., manually set or inherited from templates).
Checking VMI labels ensures matching when the pod is not available (stopped VMs, etc.).

Jira ticket: CNV-71151
KubeVirt PR #15783 introducing the change

🎥 Demo

In addition to adding the support for the new selector (vmi.kubevirt.io/id), this PR also fixes a bug where the services are not listed under the VMI:

Before:

Before.mov

After:

After.mov

Summary by CodeRabbit

  • Bug Fixes

  • Improved RDP and SSH service discovery by making label selection pod-aware and strengthening fallbacks when pod/VMI context is missing.

  • Refactor

  • Centralized and optimized service lookup and label handling for VM consoles and access paths to ensure consistent selector generation.

  • Tests

  • Updated unit tests to cover pod-aware selector logic and new fallback scenarios.

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.

@galkremer1
Copy link
Member Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants