CORENET-6363: Add PreconfiguredUDNAddresses duplicate IP detection tests #30197
Conversation
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. |
| 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
| 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 |
| @@ -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() | |||
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.
| 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 |
Validates that KubeVirt VMs with preconfigured MAC and IP addresses maintain those addresses correctly before and after a vmi with duplicate IP/MAC request is made, and that the vmi with the duplicate address get the appropriate address conflict error event. Co-authored-by: Miguel Duarte Barroso <[email protected]> Signed-off-by: Ram Lavi <[email protected]>
The requested IPs for the primary UDN attachment are not in the VMI spec, but in an annotation in the VMI. Hence, we need to fetch that particular annotation, and set it in the duplicate VMI. This was implemented using the builder pattern, since I suspect in the future we will need to further customize the VMI spec / metadata; this will make it simpler to extend the framework in the future. Signed-off-by: Miguel Duarte Barroso <[email protected]>
This way we can ensure appropriate cases are caught in a generic way. With it, we can safely expect to find IP conflicts when there are duplicate IPs in the network. In future commits, we will be able to catch MAC conflicts when there are duplicate MACs in the network Co-authored-by: Ram Lavi <[email protected]> Signed-off-by: Miguel Duarte Barroso <[email protected]>
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 |
|
@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. |
|
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/ovn-kubernetes#2744 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: qinqon, RamLavi The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/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#2750 |
This PR is a followup PR to #30010, adding tests to check duplicate IP detection: