Add go vet to the Makefile and fix its errors

[ronensc]

Issue link

What changes have been made

• Added go vet to the Makefile
• Fixed its errors

Verification steps

Checks

• I've made sure the tests are passing.
• Testing Strategy
  • Unit tests
  • Manual tests
  • Testing is not required for this change

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign asm582 for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[ronensc]

@anishasthana I think MCAD-CI is flaky as I don't think its failure is related to the changes in this PR. Could you please try rerunning it?

[ronensc]

Failed again. Same failing test "MCAD CPU Preemption Test"... 😢
https://github.com/project-codeflare/multi-cluster-app-dispatcher/actions/runs/6876210215/job/19098434211#step:8:722

It seems it is failing in other PRs as well.
https://github.com/project-codeflare/multi-cluster-app-dispatcher/actions/runs/7018959159/job/19095405569#step:8:726

https://github.com/project-codeflare/multi-cluster-app-dispatcher/actions/runs/7003208146/job/19048461449#step:8:725

[anishasthana]

Hmm @asm582 are you aware of any recent issues with CI?

[dgrove-oss]

I observed when porting the test suite to MCAD v2 that many of the tests only work as expected if the capacity of the cluster is calculated to be about 2 CPUs. If the cluster has more capacity, then the tests that expect certain AppWrappers to not be runnable or to be preempted because higher-priority AppWrappers will have taken all the available capacity fail. The failing test here is one of those. Looking at the logs, I see that the controller thinks the available capacity is about 6 CPUs

 The available capacity to dispatch appwrapper is cpu 5950.00, memory 32085303296.00, GPU 16 and time took to calculate is 8.462585ms

In the v2 test suite, I am redoing all of these tests to be based on a % of total CPU capacity instead of using hardwired resource values to avoid this fragility.

[asm582]

Thank you for the PR. I am curious, if we are doing %-based CPU calculation then are we diverging from the way Scheduler does accounting? and would that be ok?

[asm582]

Hmm @asm582 are you aware of any recent issues with CI?

I usually set 2 CPUs and 8 GB RAM as resource setting in podman or docker desktop and they would run to completion locally. I am not sure if we changed resource requirement in the CI system

[dgrove-oss]

To be clear, tests like this are what are fragile in my opinion: https://github.com/project-codeflare/multi-cluster-app-dispatcher/blob/main/test/e2e/queue.go#L97-L125 because they are using hardwired CPU requests (not CPU requests that are calculated dynamically by the test as a fraction of the available capacity on the actual cluster when it is run).

Here it creates an AppWrapper with a 1100 CPU request (2 pods at 550 each) and then assumes that if it creates a 854 CPU request (2 pods at 427 each..the function name is misleading) it won't fit. If docker is limited to 2 CPUs, then the worker nodes have 4000 CPU capacity, Kubernetes + the mcad operator take around 2100 CPU and the math works as expected. The slightest change in available resource (or in the resources used by the system pods) and the test doesn't work as expected.

[ronensc]

@dgrove-oss thanks for the detailed explanation of the root cause of the CI failure!
I noticed that these fragile tests were commented out temporarily in mcad v2. Should we adopt a similar approach here? Can this PR be approved even though the CI is currently failing?