e2e, persistent-ip: Add primary-UDN tests #71
Conversation
kubevirt-bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
RamLavi
force-pushed
the
udn_e2e
branch
2 times, most recently
from
94851d0
to
a1df094
Compare
| @@ -116,6 +116,8 @@ var _ = Describe("Persistent IPs on Primary UDN interface", func() { | |||
| WithTimeout(5 * time.Minute). | |||
| Should(testenv.ContainConditionVMIReady()) | |||
|
|
|||
| Expect(testenv.Client.Get(context.Background(), client.ObjectKeyFromObject(vmi), vmi)).To(Succeed()) | |||
There was a problem hiding this comment.
please consider making it part of the Eventually block
(also on the other places)
this what we are usually on kubevirt fwiw
There was a problem hiding this comment.
I don't think we need eventually, because this call is right after we wait for the VMI to be ready
There was a problem hiding this comment.
you already fetch the latest vmi as part of ThisVMI in the Eventually above (implicit Eventually happens for what you need)
so you can just set & use that value externally
func ThisVMI(vmi *kubevirtv1.VirtualMachineInstance) func() (*kubevirtv1.VirtualMachineInstance, error) {
return func() (p *kubevirtv1.VirtualMachineInstance, err error) {
p = &kubevirtv1.VirtualMachineInstance{}
err = Client.Get(context.Background(), client.ObjectKeyFromObject(vmi), p)
if apierrors.IsNotFound(err) {
return nil, nil
}
return
}
}
There was a problem hiding this comment.
true, but this change has bigger scope than what I want to achieve in this PR.
What my last commit is doing is only aligning the behavior to what is done in other tests..
But we should definitely should do it on a follow up PR (change ThisVMI + remove all the "fetch" that is used before the assertion)
There was a problem hiding this comment.
honestly i dont understand why to not add
vmi = testenv.ThisVMI(vmi) in the first line of the Eventually
(returning vmi in the func of it etc as we do on kubevirt)
iiuc this all what needed instead those additional lines, and imho better
There was a problem hiding this comment.
I intend to do so, but in a larger scope, in a follow up PR
for now they are stable, i wouldnt suggest to add unless we see either by code reading or by a failure that it is really needed |
ah no no ... my bad. I need to just squash that commit with the former.. The issue cannot happen in the secondary case. |
This will enable the network-segmentation feature on OVNK, needed in order to run VM workloads with primary UDN. Signed-off-by: Ram Lavi <[email protected]>
Currently GenerateLayer2WithSubnetNAD is creating NADs for secondary roles (which is the default). Add role input param to the function in order to support primary role that will be used in future commits. Signed-off-by: Ram Lavi <[email protected]>
This is done to differentiate this test from the primary udn tests that will be added in future commits. Signed-off-by: Ram Lavi <[email protected]>
|
Change: Rebase |
These tests run practically the same tests as in secondary interfaces, with a few semantic changes: - networkInterfaceName is 'ovn-udn1' - VMI is created with primary UDN - NAD is created with role: Primary - interface IPS are extracted using network-status annotation, and not vmi.status as that is not yet supported for primaryUDN. Signed-off-by: Ram Lavi <[email protected]>
|
@RamLavi let's refactor current tests a little to parameterize role and VM's interface (so we can pass the proper primary UDN network binding). |
|
closing in favor of #72 |
What this PR does / why we need it:
This PR adds the needed tests needed in order to cover ipam-extentions operation on VMI/VMs using primary-UDN.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Release note: