Fix ports read/write races in neg tests

[panslava]

In general, I want to make all unit tests pass with -race, this is 1 step towards

Current approach with ports variable in neg tests has race, cause we use global variable, and we modify it from different tests

/assign swetharepakula
/assign cezarygerard
/cc code-elinka
/cc aojea

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[k8s-ci-robot]

@panslava: GitHub didn't allow me to request PR reviews from the following users: code-elinka.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

In general, I want to make all unit tests pass with -race, this is 1 step towards

Current approach with ports variable in neg tests has race, cause we use global variable, and we modify it from different tests

/assign swetharepakula
/assign cezarygerard
/cc code-elinka
/cc aojea

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/test-infra repository.

[aojea]

why not removing t.Parallel() from the tests that share the variable?

[panslava]

why not removing t.Parallel() from the tests that share the variable?

Do you think it is better? I feel like running tests in parallel can be good to find some weird errors (like these global variables, but not in real code), that we would never found if we ran everything single threaded

[aojea]

Do you think it is better? I feel like running tests in parallel can be good to find some weird errors (like these global variables, but not in real code), that we would never found if we ran everything single threaded

in this case is a design choice , if you have a global variable to share in multiple test is a conscious decision, what is important though is to run with -race , we should always enable the race detector, t.Parallel() is more an optimisation to me ... but I can be wrong on this, of course

[panslava]

Do you think it is better? I feel like running tests in parallel can be good to find some weird errors (like these global variables, but not in real code), that we would never found if we ran everything single threaded

in this case is a design choice , if you have a global variable to share in multiple test is a conscious decision, what is important though is to run with -race , we should always enable the race detector, t.Parallel() is more an optimisation to me ... but I can be wrong on this, of course

Tbh this tests were written not by me, but this global variable is actually not intended to be shared between tests (in the way that there is no shared logic based on this variable), as I understand, this is just some useful “const”

[aojea]

And are the test modifying the variable?

[panslava]

And are the test modifying the variable?

They do modify, but only for their own usage, like if they understand default const is not enough, they will append to it something extra

ingress-gce/pkg/neg/controller_test.go

Line 1771 in c6d2645

ports = append(

[aojea]

Oh, that is wrong thenz, it will pollute all the other tests

[panslava]

Oh, that is wrong thenz, it will pollute all the other tests

So that’s what I am trying to fix in this PR, at first, I am taking a copy of this global ports, and then modify only copy

[aojea]

What do you think about using a function instead?
It is more intuitive and less error prone to avoid similar issues in the future

func servicePorts() []apiv1.ServicePort

[panslava]

What do you think about using a function instead? It is more intuitive and less error prone to avoid similar issues in the future

func servicePorts() []apiv1.ServicePort

thanks! good suggestion! changed

[aojea]

/lgtm

Thanks

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, panslava

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [panslava]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[aojea]

why don't you send a PR enabling the race detector and adding a commit per fix?

this way is easier to review and to fix

[panslava]

@aojea the problem is, there are other tests that are failing race detector, so I can not enable it yet... and other tests are harder to fix (at least not as obvious)

[panslava]

or you suggesting something else? Maybe I didn't understand completely the suggestion

[aojea]

#1838