-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Add PreconfiguredUDNAddresses FG tests #30010
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?
Add PreconfiguredUDNAddresses FG tests #30010
Conversation
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.
This PR is really nice, thanks for that.
I just have some questions, nits, nothing big.
Let's see how this fares on CI. Fingers crossed.
expectedIPs := strings.Split(netConfig.preconfiguredIP, ",") | ||
for _, expectedIP := range expectedIPs { |
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.
nit: this is opinionated; couldn't you inline these ? like
expectedIPs := strings.Split(netConfig.preconfiguredIP, ",") | |
for _, expectedIP := range expectedIPs { | |
for _, expectedIP := range strings.Split(netConfig.preconfiguredIP, ",") { |
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.
fixed.
ShouldNot(BeEmpty()) | ||
Expect(actualMAC).To(Equal(netConfig.preconfiguredMAC)) |
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.
wouldn't it make sense to actually mix these two up ?
Something like
ShouldNot(BeEmpty()) | |
Expect(actualMAC).To(Equal(netConfig.preconfiguredMAC)) | |
Should(Equal(netConfig.preconfiguredMAC)) |
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.
totally.
GinkgoHelper() | ||
|
||
var err error | ||
postMigrationMAC, err = obtainMAC(virtClient, vmName) | ||
g.Expect(err).NotTo(HaveOccurred(), "Failed to obtain MAC address for VM after migration") | ||
return postMigrationMAC |
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.
maybe you can export this into an helper func ? which would be re-used in the other test - AFAIU, the only difference at play is the name of the variable where you put the MAC addr.
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.
DONE
func macFromStatus(cli *kubevirt.Client, vmName string) (string, error) { | ||
macStr, err := cli.GetJSONPath("vmi", vmName, "{@.status.interfaces[0].mac}") | ||
if err != nil { | ||
return "", fmt.Errorf("failed to extract the MAC address from VM %q: %w", vmName, err) | ||
} | ||
return macStr, nil | ||
} | ||
|
||
func obtainMAC(virtClient *kubevirt.Client, vmName string) (string, error) { | ||
return macFromStatus(virtClient, vmName) | ||
} |
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.
why do we have these 2 fns ? one just calls the other.
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.
I was following the IP obtain function
func obtainAddresses(virtClient *kubevirt.Client, vmName string) ([]string, error) {
return addressFromStatus(virtClient, vmName)
}
but might as well unify the calls into one. DONE.
return macStr, nil | ||
} | ||
|
||
func obtainMAC(virtClient *kubevirt.Client, vmName string) (string, error) { |
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.
don't we already have a function to extract the VM's MAC address from the status @qinqon ?
@@ -93,6 +93,29 @@ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: do we really need to be "admin" for this ? I assume yes, since we're entering arbitrary namespaces though.
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.
I assume so, will check once I have a cluster running.
duplicateVMName := vmName + "-duplicate" | ||
By(fmt.Sprintf("Duplicating VM %s/%s to %s/%s", vmNamespace, vmName, vmNamespace, duplicateVMName)) | ||
|
||
vmiSpecJSON, err := cli.GetJSONPath("vmi", vmName, "{.spec}") |
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.
couldn't you do this without reading from the API ? I mean, just do some oldVMI.DeepCopy()
kind of thing ?
I think you'd need to turn duplicateVM
into an higher order function though - you'd pass the VM from the test into the function returned by this higher order fn.
Opinionated nit.
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.
Now, the thing is - If I want to reuse this DescribeTable - I need the function to fit opCmd:
opCmd func(cli *kubevirt.Client, vmNamespace, vmName string)
So in order to do so I had to duplicate the VM with only these params.
I kinda think it's keeps things tidy, but if you prefer I can copy this big DescribeTable to a standalone test where I can create the duplicate VM is a more standard way.
When I tried it I had to create a bunch of helpers to try and avoid duplicate code from the current test, and it ended up less nice and tidy than the current way imo..
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.
let's keep it as is then.
return []string{} | ||
} | ||
|
||
virtLauncherPodName := podNames[0] |
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.
ok, we're "safe" in the sense we won't see > 1 pod here since your get pods by label function checks pods in the test's namespace, right ?
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.
that, and the fact that I don't expect this VM to be migrating.
3ad3231
to
8d6569e
Compare
/test e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview |
Job Failure Risk Analysis for sha: 8d6569e
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 8d6569e
New tests seen in this PR at sha: 8d6569e
|
/testwith e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
@maiqueb,
|
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
Image failed to build: |
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
8d6569e
to
be08098
Compare
Change: fix kubevirt.io/network/addresses annotation expected value format. |
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
Job Failure Risk Analysis for sha: be08098
|
@qinqon I dug in the must gather files a bit, could this be relevant to your PR?
|
This is not related to my PR, this is ipam extensions creating the default-network annotation with wrong namespace, and that means we are missing the openshift virt integratin that configure ipam extensions with "openshift-ovn-kubernetes" namespace instead of "ovn-kubernetes". |
ah. so we need to wait for that in order to make the tests pass.. ACK. |
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
be08098
to
44f8b30
Compare
Change: rebase |
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
Job Failure Risk Analysis for sha: 44f8b30
|
Last run was affected by: openshift/machine-config-operator#5213 /testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
Last ran failed early on cluster creation, it seems that it was still affected openshift/machine-config-operator#5213 /testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
cluster failed to create. |
New changes are detected. LGTM label has been removed. |
Change: refactor to not use pointer when variadic param is already used. |
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
94416d4
to
4ee6f4f
Compare
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
4ee6f4f
to
7c16f26
Compare
Change: Attempt to fix annotation output |
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
7c16f26
to
32a45e5
Compare
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
Job Failure Risk Analysis for sha: 32a45e5
|
32a45e5
to
62304a5
Compare
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
Change: Remove ipv6 instance from static ip test to check if reason of failure is ipv6 not supported |
Job Failure Risk Analysis for sha: 62304a5
|
Validates that KubeVirt VMs with preconfigured IP addresses maintain those addresses correctly before and after restart operation. Signed-off-by: Ram Lavi <ralavi@redhat.com>
Validates that KubeVirt VMs with preconfigured MAC addresses maintain those addresses correctly before and after restart operation. Signed-off-by: Ram Lavi <ralavi@redhat.com>
Validates that KubeVirt VMs with preconfigured MAC and IP addresses maintain those addresses correctly before and after live migration operation. Signed-off-by: Ram Lavi <ralavi@redhat.com>
62304a5
to
18f6d17
Compare
Change: Add back the ipv6 ip |
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
Only the conflict detection related tests are failing in this job execution. Let's try with the updated PR openshift/ovn-kubernetes#2666, which brings along the PR with IP conflict detection. Fingers crossed. /testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 |
Job Failure Risk Analysis for sha: 18f6d17
|
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. Signed-off-by: Ram Lavi <ralavi@redhat.com>
18f6d17
to
64ee3a6
Compare
/testwith openshift/origin/main/e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview openshift/cluster-network-operator#2743 openshift/ovn-kubernetes#2666 the IP conflict checks were configuring a single static IP in a dual stack cluster, something which (despite my personal thoughts on the matter) will not be allowed in a follow up production code changes to the OVN-K repo. |
@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: 64ee3a6
|
This PR adds tests to check the PreconfiguredUDNAddresses FG: