General cleanup in validate.go #422

vrindle · 2023-03-20T02:31:34Z

Uses more expressive return types and also reduces cyclomatic complexity.

github-actions · 2023-03-20T02:31:47Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

coveralls · 2023-03-20T02:38:34Z

Pull Request Test Coverage Report for Build 5072635988

16 of 37 (43.24%) changed or added relevant lines in 1 file are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.1%) to 25.661%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/webhook/validate.go	16	37	43.24%

Files with Coverage Reduction	New Missed Lines	%
pkg/webhook/validate.go	1	54.71%
controllers/sriovibnetwork_controller.go	2	66.04%

Totals
Change from base Build 5022019978:	0.1%
Covered Lines:	1940
Relevant Lines:	7560

💛 - Coveralls

wizhaoredhat · 2023-03-20T15:14:25Z

/cc @bn222
/cc @wizhaoredhat
/cc @SalDaniele

bn222 · 2023-03-20T19:46:02Z

As discussed, let's have validateNicModel return an error instead. Also, let's get rid of the side-effect way to communicate the result of functions. More precisely, we should at least address interfaceSelected and maybe even nodesSelected.

It seems to me that we should also refactor the for loop in the function dynamicValidateSriovNetworkNodePolicy. Some parts of it are nested 5 deep.

github-actions · 2023-03-29T02:15:38Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-03-29T03:10:17Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-03-29T03:11:53Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

wizhaoredhat · 2023-03-29T22:17:29Z

pkg/webhook/validate.go

 	}

 	// Check the vendor and device ID of the VF only if we are on a virtual environment
 	for key := range utils.PlatformMap {
 		if strings.Contains(strings.ToLower(node.Spec.ProviderID), strings.ToLower(key)) &&
 			selector.NetFilter != "" && selector.NetFilter == iface.NetFilter &&
 			sriovnetworkv1.IsVfSupportedModel(iface.Vendor, iface.DeviceID) {
-			return true
+			return nil
+		} else {


This is divergent logic. The "for" loop is trying to find something matching. It doesn't mean that if it doesn't match that it is an error.

So the new behaviour of the code you have implemented here is not equivalent to the original.

wizhaoredhat · 2023-03-29T22:21:30Z

/cc @pliurh

github-actions · 2023-04-06T00:58:09Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-04-11T08:06:42Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-04-11T08:09:50Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-04-11T08:15:00Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-04-11T08:17:48Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

wizhaoredhat · 2023-04-11T20:59:13Z

pkg/webhook/validate.go

-		if validateNicModel(&policy.Spec.NicSelector, &iface, node) {
-			interfaceSelected = true
+		err := validateNicModel(&policy.Spec.NicSelector, &iface, node)
+		if err != nil {


Err contains important information. But it is not passed down or shown to the user.

This can be resolved by logging and showing to the user without changing the behavior too much. Latest changes reflect that.

github-actions · 2023-04-13T05:40:13Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-04-13T06:07:30Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-04-28T18:36:24Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-04-28T18:38:55Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-04-28T18:41:55Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

pkg/webhook/validate.go

github-actions · 2023-05-09T19:56:01Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

pkg/webhook/validate.go

github-actions · 2023-05-23T02:41:51Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-05-23T16:11:18Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

pkg/webhook/validate.go

wizhaoredhat · 2023-05-23T22:37:47Z

pkg/webhook/validate_test.go

 }

 func TestValidatePolicyForNodeStateWithValidVFAndNetFilter(t *testing.T) {
-	interfaceSelected = false


Don't we still need this?

You removed interfaceSelected from the tests when it is still used.

github-actions · 2023-05-24T19:20:55Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

We use more expressive return types and also reduce cyclomatic complexity.

github-actions · 2023-05-24T19:24:17Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

vrindle · 2023-05-24T19:57:53Z

@bn222
@wizhaoredhat
@zshi-redhat

wizhaoredhat · 2023-05-24T20:20:30Z

pkg/webhook/validate_test.go

 	g.Expect(err).NotTo(HaveOccurred())
-	g.Expect(ok).To(Equal(true))
 	g.Expect(interfaceSelected).To(Equal(true))
 }


Should we add more tests for validateNicModel, since now it returns an error instead of a boolean?

what sort of tests are you looking to add? Imo, these functions test validateNicModel quite heavily.

validatePolicyForNodeState gets tested. But in err := validateNicModel(&policy.Spec.NicSelector, &iface, node), the err gets thrown away. I think it would be worth it to test validateNicModel indepedently.

I agree that we should test that out but maybe not in this PR. In my other PR I test this when the variable interfaceSelected is removed. Please see this line here for instance: https://github.com/k8snetworkplumbingwg/sriov-network-operator/pull/434/files#diff-ae978883f1787164fb6fa85b31c5fde6421f8a06d9c9f5f013fb72e6f21863a0R560 a
remove interfaceSelected anyways in a later PR and testing the return types for validateNicModel we don't have to add that to this PR.

wizhaoredhat · 2023-05-25T15:23:27Z

LGTM

bn222 · 2023-05-31T07:48:17Z

@adrianchiris

This is a pretty small change that doesn't actually change functionality. CI is green and we already have an LGTM from RH. Can you PTAL? Should be pretty quick.

Regards,
Balazs

adrianchiris · 2023-05-31T08:28:10Z

This is a pretty small change that doesn't actually change functionality. CI is green and we already have an LGTM from RH.

did not see an LGTM from maintainer, @bn222 @SchSeba you should approve :)
will take a look at this later this week.

bn222 · 2023-05-31T08:51:20Z

lgtm

adrianchiris · 2023-05-31T10:04:30Z

pkg/webhook/validate.go

 	glog.V(2).Infof("validatePolicyForNodeState(): validate policy %s for node %s.", policy.GetName(), state.GetName())
 	for _, iface := range state.Status.Interfaces {
-		if validateNicModel(&policy.Spec.NicSelector, &iface, node) {
+		err := validateNicModel(&policy.Spec.NicSelector, &iface, node)


nit:

if err := validateNicModel; err == nil { ... }

that said, its really a matter of taste here so im fine with keeping as is.

adrianchiris · 2023-05-31T10:45:04Z

Thx @vrindle for this improvement !

vrindle force-pushed the fix_error_message branch from 85b2dd1 to c6822a6 Compare March 29, 2023 02:15

vrindle force-pushed the fix_error_message branch from c6822a6 to f1bf6aa Compare March 29, 2023 03:10

vrindle force-pushed the fix_error_message branch from f1bf6aa to 1f9f03d Compare March 29, 2023 03:11

wizhaoredhat reviewed Mar 29, 2023

View reviewed changes

github-actions bot requested a review from pliurh March 29, 2023 22:21

vrindle force-pushed the fix_error_message branch from 1f9f03d to fda20b2 Compare April 6, 2023 00:57

vrindle force-pushed the fix_error_message branch from fda20b2 to aed33c3 Compare April 11, 2023 08:06

vrindle force-pushed the fix_error_message branch from aed33c3 to f7c30d8 Compare April 11, 2023 08:09

vrindle force-pushed the fix_error_message branch from f7c30d8 to 84eb8b7 Compare April 11, 2023 08:14

vrindle force-pushed the fix_error_message branch from 84eb8b7 to b4dfa71 Compare April 11, 2023 08:17

vrindle requested a review from wizhaoredhat April 11, 2023 20:42

wizhaoredhat reviewed Apr 11, 2023

View reviewed changes

vrindle force-pushed the fix_error_message branch from b4dfa71 to e06ddd2 Compare April 13, 2023 05:40

vrindle force-pushed the fix_error_message branch from e06ddd2 to d9c667c Compare April 13, 2023 06:07

vrindle requested a review from wizhaoredhat April 13, 2023 17:34

vrindle force-pushed the fix_error_message branch from f45ecda to a37c9c5 Compare April 28, 2023 18:38

vrindle force-pushed the fix_error_message branch from a37c9c5 to 5d94f17 Compare April 28, 2023 18:41

wizhaoredhat reviewed May 9, 2023

View reviewed changes

pkg/webhook/validate.go Outdated Show resolved Hide resolved

vrindle force-pushed the fix_error_message branch from 5d94f17 to 0fa4b93 Compare May 9, 2023 19:55

bn222 reviewed May 10, 2023

View reviewed changes

pkg/webhook/validate.go Outdated Show resolved Hide resolved

vrindle force-pushed the fix_error_message branch from 0fa4b93 to 025c5a3 Compare May 23, 2023 02:41

vrindle force-pushed the fix_error_message branch from 025c5a3 to ee63e61 Compare May 23, 2023 16:11

wizhaoredhat reviewed May 23, 2023

View reviewed changes

pkg/webhook/validate.go Show resolved Hide resolved

wizhaoredhat reviewed May 23, 2023

View reviewed changes

vrindle force-pushed the fix_error_message branch from ee63e61 to ba91e19 Compare May 24, 2023 19:20

General cleanup in validate.go

bd6efc0

We use more expressive return types and also reduce cyclomatic complexity.

vrindle force-pushed the fix_error_message branch from ba91e19 to bd6efc0 Compare May 24, 2023 19:24

wizhaoredhat reviewed May 24, 2023

View reviewed changes

adrianchiris approved these changes May 31, 2023

View reviewed changes

adrianchiris merged commit 6b81c4e into k8snetworkplumbingwg:master May 31, 2023

zeeke mentioned this pull request Jul 7, 2023

OCPBUGS-18409: [release-4.13] Allow creating policies that ignore NUMA topology openshift/sriov-network-operator#798

Merged

General cleanup in validate.go #422

General cleanup in validate.go #422

Conversation

vrindle commented Mar 20, 2023 • edited Loading

github-actions bot commented Mar 20, 2023

coveralls commented Mar 20, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5072635988

💛 - Coveralls

wizhaoredhat commented Mar 20, 2023

bn222 commented Mar 20, 2023

github-actions bot commented Mar 29, 2023

github-actions bot commented Mar 29, 2023

github-actions bot commented Mar 29, 2023

Choose a reason for hiding this comment

wizhaoredhat commented Mar 29, 2023

github-actions bot commented Apr 6, 2023

github-actions bot commented Apr 11, 2023

github-actions bot commented Apr 11, 2023

github-actions bot commented Apr 11, 2023

github-actions bot commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 13, 2023

github-actions bot commented Apr 13, 2023

github-actions bot commented Apr 28, 2023

github-actions bot commented Apr 28, 2023

github-actions bot commented Apr 28, 2023

github-actions bot commented May 9, 2023

github-actions bot commented May 23, 2023

github-actions bot commented May 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 24, 2023

github-actions bot commented May 24, 2023

vrindle commented May 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrindle May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wizhaoredhat commented May 25, 2023

bn222 commented May 31, 2023

adrianchiris commented May 31, 2023

bn222 commented May 31, 2023

Choose a reason for hiding this comment

adrianchiris commented May 31, 2023

vrindle commented Mar 20, 2023 •

edited

Loading

coveralls commented Mar 20, 2023 •

edited

Loading

vrindle May 25, 2023 •

edited

Loading