add check that vnic with same name doesn't has the same network #109

mgold1234 · 2022-02-03T13:19:54Z

Please describe the change you are making

This PR fixes #82 and check that vnic doesn't has same network

Are you the owner of the code you are sending in, or do you have permission of the owner?

yes

The code will be published under the BSD 3 clause license. Have you read and understood this license?

yes

ghost

Hey @mgold1234 thank you for your continued work on this. Can I ask you to add tests for both positive and negative for this case? It would be useful to verify the behavior against the real oVirt engine as well as the mock facility.

mgold1234 · 2022-02-09T13:54:40Z

Hey @mgold1234 thank you for your continued work on this. Can I ask you to add tests for both positive and negative for this case? It would be useful to verify the behavior against the real oVirt engine as well as the mock facility.

hi @janosdebugs, I added test that check different network (if there is exist one, else check the same network) and same name

ghost

Hey @mgold1234 thank you so much for working on this. I know this is a tricky issue, I commented on a few corner cases that we may want to check on. Could you try and get those covered too? Thank you.

ghost · 2022-02-09T16:36:14Z

client_nic_test.go

@@ -47,6 +47,19 @@ func assertCannotCreateNIC(
 	}
 }

+func assertCannotCreateNICWithSameNameDiffNetwork(


I would suggest renaming this to assertCannotCreateNICWithVNICProfile since that more accurately covers what this function does and makes the function reusable.

WDYT about assertCannotCreateNICWithSameVNICProfile?
it is not clear just with vnic profile.

We could do that, however, we are trying to keep the assert functions reusable in other test cases. The naming should convey what the assert functions are doing rather than the current use case so future contributors feel that they can safely reuse the assert function. If the assert function alone does not convey the logic in the test case using the assert function we can always add comments explaining what's happening.

client_nic_test.go

ghost · 2022-02-09T16:37:05Z

client_nic_test.go

+	params ovirtclient.BuildableNICParameters,
+) {
+	nic, _ := vm.CreateNIC(name, diffVNICProfile, params)
+	if nic != nil {


It is preferable from a readability perspective to check the error, not the returned object.

ghost · 2022-02-09T16:37:39Z

client_nic_test.go

@@ -34,7 +34,7 @@ func assertCanCreateNIC(
 	return nic
 }

-func assertCannotCreateNIC(
+func assertCannotCreateNICWithSameName(


This name change is misleading. The error message below should also be changed.

WDYT about -("create 2 NICs with same name %s (%v)", name, err)

@janosdebugs , we have some functions for create nic:

assertCanCreateNIC

assertCannotCreateNICWithSameName

assertCannotCreateNICWithVNICProfile
and more...

regarding 2 and 3 what do you suggest? if I change 2 as it was assertCannotCreateNIC, what is the differerent from 3?
maybe change it to 'assertCannotCreateNICWithName'?
and 3 will be assertCannotCreateNICWithVNICProfile?

I would personally prefer having two functions: assertCanCreateNICWithVNICProfile and assertCannotCreateNICWithVNICProfile. This should cover everything we need, right?

ghost · 2022-02-09T16:39:11Z

client_nic_create_test.go

+		nicName,
+		ovirtclient.CreateNICParams())
+	assertNICCount(t, vm, 1)
+	DiffVNICProfile, _ := assertCanFindDiffVNICProfile(helper, helper.GetVNICProfileID())


What happens if there is no second vnic profile? We cannot assume there is a secondary profile available. Can we create a second profile? What if the secondary profile is on the same network as the primary? Does it still work?

@janosdebugs , so I will change the function to catch the case that if there is no secondary profile then we will check the assertCannotCreateNICWithSameName and if we have more then 1 profile we will check this -assertCannotCreateNICWithVNICProfile.
WDYT?

Yes. Ideally, we'll skip the test if there is no secondary profile available. Later we can create a secondary profile to test.

ghost

Hey @mgold1234 thank you very much for your extensive work on this PR. There are several things to look at. The main points:

Assert functions in this code base should use t.Fatalf to fail or t.Skipf to skip a test. We skip tests if the preconditions are not met (e.g. a second network is not available), while we fail a test when an actual error happened (e.g. the engine didn't respond).
In Go please always check the returned error type instead of the return value. This makes the code more readable and less likely to introduce bugs.
It seems the mock implementation does not correctly implement the rules, it will fail if there is any name conflict. Since the test passes it means that there is at least one test missing. There should be a failing test on the current implementation. This test should check that you can create two NICs with the same name on different VNIC profiles or different networks (depending on your investigation). I would recommend always implementing the tests, checking if they fail, and then implementing the solution. (In this case the solution is fixing the mock.)

Since we don't know what the rules are, depending on your investigation of the naming conflict you need to implement one of the following two cases with 3 tests:

If the naming conflict applies to VNIC profiles:

I cannot create two NICs with the same name on the VNIC profile.
I can create two NICs with the same name on a different VNIC profile but the same network.
I can create two NICs with the same name on different networks.

If the naming conflict applies to networks:

I cannot create two NICs with the same name on the VNIC profile (which ever applies).
I cannot create two NICs with the same name on a different VNIC profile but the same network.
I can create two NICs with the same name on different networks.

client_nic_create_test.go

client_vnicprofile_test.go

client_nic_create_test.go

client_vnicprofile_test.go

mock_nic_create.go

ghost

Hey @mgold1234 as indicated in the previous review, the test needs to fail if the correct implementation is removed. This is not the case, when removing the latest change the test still passes. Please add a test that tests this case.

ghost · 2022-02-16T06:42:53Z

mock_nic_create.go

@@ -21,8 +21,8 @@ func (m *mockClient) CreateNIC(
 		return nil, newError(ENotFound, "VM with ID %s not found for NIC creation", vmid)
 	}
 	for _, n := range m.nics {
-		if n.name == name {
-			return nil, newError(ENotFound, "NIC with name %s is already in use", name)
+		if n.name == name && m.vnicProfiles[n.vnicProfileID].networkID == m.vnicProfiles[vnicProfileID].networkID {


I checked in my local setup, if I remove this change the test still passes. That means that there is no test that catches this bug.

@mgold1234 this is still not resolved. If I remove this change and run the test suite the test passes. There needs to be a test that fails if this change is not added.

mgold1234 · 2022-02-16T13:38:14Z

@janosdebugs , there is an option that I can see the result of run here, before you do the review?
there is a way to skip on workflows awaiting approval and the test will run ?

ghost · 2022-02-16T14:38:10Z

@janosdebugs , there is an option that I can see the result of run here, before you do the review? there is a way to skip on workflows awaiting approval and the test will run ?

Yes, you can run the tests locally (the CI does the same) as described in the contribution guidelines. The steps taken by the CI are in this file.

ghost · 2022-02-22T20:55:43Z

mock_nic_create.go

@@ -21,8 +21,8 @@ func (m *mockClient) CreateNIC(
 		return nil, newError(ENotFound, "VM with ID %s not found for NIC creation", vmid)
 	}
 	for _, n := range m.nics {
-		if n.name == name {
-			return nil, newError(ENotFound, "NIC with name %s is already in use", name)
+		if n.name == name && m.vnicProfiles[n.vnicProfileID].networkID == m.vnicProfiles[vnicProfileID].networkID {


@mgold1234 this is still not resolved. If I remove this change and run the test suite the test passes. There needs to be a test that fails if this change is not added.

mgold1234 · 2022-02-22T21:14:15Z

@janosdebugs I looked on this example test that also implement negative test:
func TestDiskAttachmentCannotBeAttachedToSecondVM(t *testing.T) {
t.Parallel()
helper := getHelper(t)

vm1 := assertCanCreateVM(
	t,
	helper,
	fmt.Sprintf("disk_attachment_test_%s", helper.GenerateRandomID(5)),
	ovirtclient.CreateVMParams(),
)
vm2 := assertCanCreateVM(
	t,
	helper,
	fmt.Sprintf("disk_attachment_test_%s", helper.GenerateRandomID(5)),
	ovirtclient.CreateVMParams(),
)
disk := assertCanCreateDisk(t, helper)
_ = assertCanAttachDisk(t, vm1, disk)
assertCannotAttachDisk(t, vm2, disk, ovirtclient.EConflict)

this test is not failure, and only catch the specfic error that appear in this case.
I implement the same idea.

ghost · 2022-02-22T21:18:28Z

@mgold1234 please remove the implementation highlighted above and run the test suite. It must fail. If I run it locally with the changes removed, it doesn't.

ghost · 2022-02-23T12:07:06Z

@mgold1234 as discussed, we don't have capacity to pursue this edge case further at this time. Closing this PR to revisit #82 after the current release cycle.

This PR fixes oVirt#82 and check that vnic doesn't has same network

d1442f1

ghost suggested changes Feb 3, 2022

View reviewed changes

This PR fixes oVirt#82 and check that vnic doesn't has same network

8f13077

mgold1234 requested a review from a user February 9, 2022 13:51

ghost suggested changes Feb 9, 2022

View reviewed changes

mgold1234 added 3 commits February 10, 2022 12:57

This PR fixes oVirt#82 and check that vnic doesn't has same network

9f00269

This PR fixes oVirt#82 and check that vnic doesn't has same network

0fbd0c1

This PR fixes oVirt#82 and check that vnic doesn't has same network

8d94d81

mgold1234 requested a review from a user February 13, 2022 08:53

ghost suggested changes Feb 14, 2022

View reviewed changes

This PR fixes oVirt#82 and check that vnic doesn't has same network

75a3fdd

mgold1234 requested a review from a user February 15, 2022 20:30

ghost suggested changes Feb 16, 2022

View reviewed changes

This PR fixes oVirt#82 and check that vnic doesn't has same network

40ad8ec

mgold1234 requested a review from a user February 17, 2022 16:15

Merge branch 'main' into nic_network

7633789

ghost suggested changes Feb 22, 2022

View reviewed changes

mgold1234 marked this pull request as draft February 23, 2022 10:53

ghost closed this Feb 23, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add check that vnic with same name doesn't has the same network #109

add check that vnic with same name doesn't has the same network #109

mgold1234 commented Feb 3, 2022

ghost left a comment

mgold1234 commented Feb 9, 2022

ghost left a comment

ghost Feb 9, 2022

mgold1234 Feb 9, 2022

ghost Feb 9, 2022

mgold1234 Feb 10, 2022

ghost Feb 9, 2022

mgold1234 Feb 9, 2022

ghost Feb 9, 2022

mgold1234 Feb 10, 2022

mgold1234 Feb 10, 2022

ghost Feb 10, 2022

ghost Feb 9, 2022

mgold1234 Feb 10, 2022

ghost Feb 10, 2022

ghost left a comment •

edited by ghost

Loading

ghost left a comment

ghost Feb 16, 2022

ghost Feb 22, 2022

mgold1234 commented Feb 16, 2022

ghost commented Feb 16, 2022 •

edited by ghost

Loading

ghost Feb 22, 2022

mgold1234 commented Feb 22, 2022

ghost commented Feb 22, 2022 •

edited by ghost

Loading

ghost commented Feb 23, 2022

add check that vnic with same name doesn't has the same network #109

add check that vnic with same name doesn't has the same network #109

Conversation

mgold1234 commented Feb 3, 2022

Please describe the change you are making

Are you the owner of the code you are sending in, or do you have permission of the owner?

The code will be published under the BSD 3 clause license. Have you read and understood this license?

ghost left a comment

Choose a reason for hiding this comment

mgold1234 commented Feb 9, 2022

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment • edited by ghost Loading

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgold1234 commented Feb 16, 2022

ghost commented Feb 16, 2022 • edited by ghost Loading

Choose a reason for hiding this comment

mgold1234 commented Feb 22, 2022

ghost commented Feb 22, 2022 • edited by ghost Loading

ghost commented Feb 23, 2022

ghost left a comment •

edited by ghost

Loading

ghost commented Feb 16, 2022 •

edited by ghost

Loading

ghost commented Feb 22, 2022 •

edited by ghost

Loading