Skip to content

CORENET-6363: Add PreconfiguredUDNAddresses duplicate IP detection tests #30197

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

CORENET-6363: Add PreconfiguredUDNAddresses duplicate IP detection tests #30197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc2c4c0
networking: Add primaryUDN VM Addr collision test w/ preconfigured addr
RamLavi Jul 22, 2025
9b471d7
networking, virt: carry over the requested IPs from the original VM
maiqueb Aug 14, 2025
f14d9ef
networking, virt: generalize conflict detection
maiqueb Aug 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions test/extended/networking/kubevirt/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import (
"strings"
"time"

"sigs.k8s.io/yaml"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
e2ekubectl "k8s.io/kubernetes/test/e2e/framework/kubectl"

Expand Down Expand Up @@ -93,6 +95,65 @@ func (c *Client) GetJSONPath(resource, name, jsonPath string) (string, error) {
}
return strings.TrimSuffix(strings.TrimPrefix(output, `"`), `"`), nil
}

func (c *Client) GetPodsByLabel(labelKey, labelValue string) ([]string, error) {
output, err := c.oc.AsAdmin().Run("get").Args("pods", "-n", c.oc.Namespace(), "-l", fmt.Sprintf("%s=%s", labelKey, labelValue), "-o", "jsonpath={.items[*].metadata.name}").Output()
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use -o name instead of the jsonpath also do we have to care of running vs completed pods during migration we can end up returning more pods than needed?

if err != nil {
return nil, err
}
if output == "" {
return []string{}, nil
}
return strings.Fields(output), nil
}

func (c *Client) GetEventsForPod(podName string) ([]string, error) {
output, err := c.oc.AsAdmin().Run("get").Args("events", "-n", c.oc.Namespace(), "--field-selector", fmt.Sprintf("involvedObject.name=%s,involvedObject.kind=Pod", podName), "-o", "jsonpath={.items[*].message}").Output()
if err != nil {
return nil, err
}
if output == "" {
return []string{}, nil
}
return strings.Fields(output), nil
}

type Option func(map[string]interface{})

func (c *Client) CreateVMIFromSpec(vmNamespace, vmName string, vmiSpec map[string]interface{}, opts ...Option) error {
newVMI := map[string]interface{}{
"apiVersion": "kubevirt.io/v1",
"kind": "VirtualMachineInstance",
"metadata": map[string]interface{}{
"name": vmName,
"namespace": vmNamespace,
},
"spec": vmiSpec,
}

for _, opt := range opts {
opt(newVMI)
}

newVMIYAML, err := yaml.Marshal(newVMI)
if err != nil {
return err
}
Comment on lines +124 to +141
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we use Unstructured stuff for this ?


return c.Apply(string(newVMIYAML))
}

func WithAnnotations(annotations map[string]string) Option {
return func(cr map[string]interface{}) {
metadata, hasMetadata := cr["metadata"].(map[string]interface{})
if !hasMetadata {
metadata = make(map[string]interface{})
cr["metadata"] = metadata
}
metadata["annotations"] = annotations
}
}

func ensureVirtctl(oc *exutil.CLI, dir string) (string, error) {
filepath := filepath.Join(dir, "virtctl")
_, err := os.Stat(filepath)
Expand Down
79 changes: 79 additions & 0 deletions test/extended/networking/livemigration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import (
"github.com/openshift/origin/test/extended/util/image"
)

const kvIPRequestsAnnot = "network.kubevirt.io/addresses"

var _ = Describe("[sig-network][OCPFeatureGate:PersistentIPsForVirtualization][Feature:Layer2LiveMigration] Kubevirt Virtual Machines", func() {
// disable automatic namespace creation, we need to add the required UDN label
oc := exutil.NewCLIWithoutNamespace("network-segmentation-e2e")
Expand Down Expand Up @@ -312,6 +314,20 @@ var _ = Describe("[sig-network][OCPFeatureGate:PersistentIPsForVirtualization][F
preconfiguredMAC: "02:0A:0B:0C:0D:51",
},
),
Entry(
"[OCPFeatureGate:PreconfiguredUDNAddresses] when the VM with preconfigured IP address is created when the address is already taken",
networkAttachmentConfigParams{
name: nadName,
topology: "layer2",
role: "primary",
allowPersistentIPs: true,
},
kubevirt.FedoraVMWithPreconfiguredPrimaryUDNAttachment,
duplicateVM,
workloadNetworkConfig{
preconfiguredIPs: []string{"203.203.0.100", "2014:100:200::100"},
},
),
)
},
Entry("NetworkAttachmentDefinitions", func(c networkAttachmentConfigParams) networkAttachmentConfig {
Expand Down Expand Up @@ -537,6 +553,65 @@ func verifyVMMAC(virtClient *kubevirt.Client, vmName, expectedMAC string) {
Should(Equal(expectedMAC))
}

func duplicateVM(cli *kubevirt.Client, vmNamespace, vmName string) {
GinkgoHelper()
duplicateVMName := vmName + "-duplicate"
By(fmt.Sprintf("Duplicating VM %s/%s to %s/%s", vmNamespace, vmName, vmNamespace, duplicateVMName))

vmiSpecJSON, err := cli.GetJSONPath("vmi", vmName, "{.spec}")
Expect(err).NotTo(HaveOccurred())
var vmiSpec map[string]interface{}
Expect(json.Unmarshal([]byte(vmiSpecJSON), &vmiSpec)).To(Succeed())

originalVMIRawAnnotations, err := cli.GetJSONPath("vmi", vmName, "{.metadata.annotations}")
Expect(err).NotTo(HaveOccurred())

originalVMIAnnotations := map[string]string{}
Expect(json.Unmarshal([]byte(originalVMIRawAnnotations), &originalVMIAnnotations)).To(Succeed())

var vmiCreationOptions []kubevirt.Option
var vmiExpectations []func()
if requestedIPs, hasIPRequests := originalVMIAnnotations[kvIPRequestsAnnot]; hasIPRequests {
vmiCreationOptions = append(
vmiCreationOptions,
kubevirt.WithAnnotations(ipRequests(requestedIPs)),
)
vmiExpectations = append(vmiExpectations, func() {
waitForVMPodEventWithMessage(
cli,
vmNamespace,
duplicateVMName,
"IP is already allocated",
2*time.Minute,
)
})
}
Expect(cli.CreateVMIFromSpec(vmNamespace, duplicateVMName, vmiSpec, vmiCreationOptions...)).To(Succeed())
for _, expectation := range vmiExpectations {
expectation()
}
}

func waitForVMPodEventWithMessage(vmClient *kubevirt.Client, vmNamespace, vmName, expectedEventMessage string, timeout time.Duration) {
GinkgoHelper()
By(fmt.Sprintf("Waiting for event containing %q on VM %s/%s virt-launcher pod", expectedEventMessage, vmNamespace, vmName))

Eventually(func(g Gomega) []string {
const vmLabelKey = "vm.kubevirt.io/name"
podNames, err := vmClient.GetPodsByLabel(vmLabelKey, vmName)
g.Expect(err).NotTo(HaveOccurred(), "Failed to get pods by label %s=%s", vmLabelKey, vmName)
g.Expect(podNames).To(HaveLen(1), "Expected exactly one virt-launcher pod for VM %s/%s, but found %d pods: %v", vmNamespace, vmName, len(podNames), podNames)
Copy link
Contributor

Choose a reason for hiding this comment

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

question: don't we need to filter out some states ? Let's take the "migration" scenario into consideration; a completed migration will leave the original (src) pod as completed. Won't that pod be caught in this filter ?

Do we want to address that scenario here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

depends.. Right now we not looking for event of vm-pods during migration/restart, so I would keep things simple.
Should the occasion arise (look for events during migration/restart/whatnot), we could change GetPodsByLabel --> GetPodsByLabelAndPhase ...

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough for me.


virtLauncherPodName := podNames[0]
eventMessages, err := vmClient.GetEventsForPod(virtLauncherPodName)
g.Expect(err).NotTo(HaveOccurred(), "Failed to get events for pod %s", virtLauncherPodName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we want to retry o error ?


return eventMessages
}).WithPolling(time.Second).WithTimeout(timeout).Should(
ContainElements(strings.Split(expectedEventMessage, " ")),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have to split "IP is already allocated", ?

Copy link
Contributor

@maiqueb maiqueb Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

because that's what's output by the GetEventsForPod function - a slice of strings, with each "word" a separate slice element.

Maybe it can be corrected, but I just found it simpler to do it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

For instance, the lane is failing right now on the MAC conflict tests because:

Expected to find an event containing "MAC address conflict detected:"
Expected
    <[]string | len:28, cap:28>: [
        "Successfully",
        "assigned",
        "e2e-network-segmentation-e2e-2361/virt-launcher-myvm-duplicate-btp47",
        "to",
        "worker-2",
        "failed",
        "to",
        "update",
        "pod",
        "e2e-network-segmentation-e2e-2361/virt-launcher-myvm-duplicate-btp47:",
        "failed",
        "to",
        "reserve",
        "MAC",
        "address",
        "\"02:0a:0b:0c:0d:10\"",
        "for",
        "owner",
        "\"e2e-network-segmentation-e2e-2361/myvm-duplicate\"",
        "on",
        "network",
        "attachment",
        "\"e2e-network-segmentation-e2e-2361/blue\":",
        "MAC",
        "address",
        "already",
        "in",
        "use",
    ]
to contain elements
    <[]string | len:4, cap:4>: ["MAC", "address", "conflict", "detected:"]
the missing elements were
    <[]string | len:2, cap:2>: ["conflict", "detected:"]}

@qinqon this shows you the output data.

@RamLavi I think it does not make sense to "fix" the tests since we have decided (out of band) to drop the MAC address related tests from this PR, and to later on push them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't split the string, we can end up with false possitives since the elemnts of the array can be at the testing array but a different places.

Copy link
Contributor

Choose a reason for hiding this comment

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

definitely. @RamLavi would you follow up on this one ?

We need to avoid calling the strings.Fields(...) in the GetEventsForPod function.

fmt.Sprintf("Expected to find an event containing %q", expectedEventMessage))
}

func waitForPodsCondition(fr *framework.Framework, pods []*corev1.Pod, conditionFn func(g Gomega, pod *corev1.Pod)) {
for _, pod := range pods {
Eventually(func(g Gomega) {
Expand Down Expand Up @@ -744,3 +819,7 @@ func formatAddressesAnnotation(preconfiguredIPs []string) (string, error) {

return string(staticIPs), nil
}

func ipRequests(ips string) map[string]string {
return map[string]string{kvIPRequestsAnnot: ips}
}