CORENET-6363: Add PreconfiguredUDNAddresses duplicate IP detection tests #30197
+156
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RamLavi
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@RamLavi: This pull request references CORENET-6363 which is a valid jira issue. In 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. |
|
/test e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview |
failed as expected |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 6b6dfb1
New tests seen in this PR at sha: 6b6dfb1
|
|
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/ovn-kubernetes#2744 |
1 similar comment
|
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/ovn-kubernetes#2744 |
results: ip detection passes, MAC detection currently fails. |
maiqueb
reviewed
Sep 4, 2025
| 2*time.Minute, | ||
| ) | ||
| }) | ||
| } else if hasMACAddressRequest(cli, vmName) { |
There was a problem hiding this comment.
don't we want to drop the "else" ? Otherwise, how will we be able to create a VM w/ MAC & IP requests ?
As this stands, I think it'll get either MAC or (exclusive or) IP.
There was a problem hiding this comment.
I removed the MAC related code. We will reintroduce it when we add the MAC test.
Wanted to again clarify though - the pod will not err on both MAC and IP conflicts, it will return err on the first problem it finds and rollover.
There was a problem hiding this comment.
yeah, still, that's the production code.
The test, imho, should be prepared to work independently of that implementation detail.
not a must, I won't insist further.
There was a problem hiding this comment.
hmm let's pick up this thread in the follow up PR where we add the MAC dupliacate
maiqueb
reviewed
Sep 4, 2025
| 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) |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 ...
RamLavi
force-pushed
the
add_PreconfiguredUDNAddresses_conflict_ip_mac_e2e
branch
from
6b6dfb1 to
5e64ef9
Compare
RamLavi
changed the title
|
@RamLavi: This pull request references CORENET-6363 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. In 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. |
|
/jira refresh |
|
@maiqueb: This pull request references CORENET-6363 which is a valid jira issue. In 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. |
RamLavi
force-pushed
the
add_PreconfiguredUDNAddresses_conflict_ip_mac_e2e
branch
2 times, most recently
from
f82bd17 to
f14d9ef
Compare
|
Changes: Remove code regarding MAC duplicate test. |
|
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/ovn-kubernetes#2744 |
qinqon
suggested changes
Sep 4, 2025
| } | ||
|
|
||
| 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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
You can use -o name instead of the jsonpath
ACK
also do we have to care of running vs completed pods during migration we can end up returning more pods than needed?
@maiqueb also raised this here - I think that right now we should keep it simple (we only check restarted VMs, not migrated ones), but having said that - since both of you suggested it - if you still think it's needed to be added now - I'll do it. your call.
Comment on lines
+124
to
+157
| 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 | ||
| } |
There was a problem hiding this comment.
Why don't we use Unstructured stuff for this ?
There was a problem hiding this comment.
not sure I follow - can you elaborate?
|
|
||
| virtLauncherPodName := podNames[0] | ||
| eventMessages, err := vmClient.GetEventsForPod(virtLauncherPodName) | ||
| g.Expect(err).NotTo(HaveOccurred(), "Failed to get events for pod %s", virtLauncherPodName) |
There was a problem hiding this comment.
Are we sure we want to retry o error ?
There was a problem hiding this comment.
why not? if k8s service is down or something we should retry.. My reasoning is: it's not what's the test is about, we should try and get the events and fail only if the relevant even isn't there.
|
|
||
| return eventMessages | ||
| }).WithPolling(time.Second).WithTimeout(timeout).Should( | ||
| ContainElements(strings.Split(expectedEventMessage, " ")), |
There was a problem hiding this comment.
Why do we have to split "IP is already allocated", ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
definitely. @RamLavi would you follow up on this one ?
We need to avoid calling the strings.Fields(...) in the GetEventsForPod function.
There was a problem hiding this comment.
well the thing was that jsonpath returns space separated words..
changed to using custom-columns
|
Job Failure Risk Analysis for sha: f14d9ef
|
|
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/ovn-kubernetes#2744 |
RamLavi
force-pushed
the
add_PreconfiguredUDNAddresses_conflict_ip_mac_e2e
branch
from
f14d9ef to
f43feb8
Compare
|
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/ovn-kubernetes#2744 |
|
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/ovn-kubernetes#2744 |
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
1 similar comment
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
|
/hold Revision fdb0584 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
|
/retest-required |
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
2 similar comments
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
|
/retest-required |
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
2 similar comments
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
|
/tide-refresh |
|
/retest-required |
|
@RamLavi: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
1 similar comment
|
Job Failure Risk Analysis for sha: fdb0584
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: fdb0584
New tests seen in this PR at sha: fdb0584
|
|
/test e2e-aws-ovn-fips |
openshift-merge-bot
bot
merged commit 26420d3
into
openshift:main
24 of 31 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a followup PR to #30010, adding tests to check duplicate IP detection: