chore: remove vendor modules from benchmark

[kushthedude]

Signed-off-by: Kush Trivedi kushthedude@gmail.com

What type of PR is this?

/area dependency
What this PR does / why we need it:

• This PR removes vendor modules for benchmark sub-module.
• We don't need vendor modules as even go modules dependency management.

Which issue(s) this PR fixes:

Refers #1099

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

Hi @kushthedude. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[kushthedude]

/assign @wojtek-t

[mm4tt]

Sorry, but we do we need the vendor dir? It's obsolete and redundant with the new go mod dep management system. In case of clusterloader2 we needed it for backward compatibility (travis presubmits), but it's not the case here. It's also not the case anymore for cl2, as we got rid of the travis presubmits.

We should also remove the vendor dir from clusterloader2 and other places (e.g. benchmarks)

[wojtek-t]

Sorry, but we do we need the vendor dir? It's obsolete and redundant with the new go mod dep management system. In case of clusterloader2 we needed it for backward compatibility (travis presubmits), but it's not the case here. It's also not the case anymore for cl2, as we got rid of the travis presubmits.

We should also remove the vendor dir from clusterloader2 and other places (e.g. benchmarks)

@mm4tt - That's not obvious to me - we have vendor/ in k/k repo and we don't have travis there at all.
There were other reasons for having it, and unless you know all of them, let's not remove them.

[mm4tt]

Sorry, but we do we need the vendor dir? It's obsolete and redundant with the new go mod dep management system. In case of clusterloader2 we needed it for backward compatibility (travis presubmits), but it's not the case here. It's also not the case anymore for cl2, as we got rid of the travis presubmits.

We should also remove the vendor dir from clusterloader2 and other places (e.g. benchmarks)

Matt Matejczyk - That's not obvious to me - we have vendor/ in k/k repo and we don't have travis there at all.
There were other reasons for having it, and unless you know all of them, let's not remove them.

I don't know what are the reasons for k/k repo. I'd guess it has something to do with the complicated build system we have for kubernetes. e.g. doing the magic for the /staging dir requires all files to be on disk.

But I know that the only reason for k/perf-tests to keep the vendor dir was the travis presubmit. Given that we don't have the travis presubmit anymore, we're free to remove it. And we should do it, as it'll get rid of the redundancy and make some operations easier, e.g. updating the dependencies will be a few line, easy-to-review change instead of a change with 1k+ modified files.

[wojtek-t]

But I know that the only reason for k/perf-tests to keep the vendor dir was the travis presubmit. Given that we don't have the travis presubmit anymore, we're free to remove it. And we should do it, as it'll get rid of the redundancy and make some operations easier, e.g. updating the dependencies will be a few line, easy-to-review change instead of a change with 1k+ modified files.

I guess I disagree with it. Yes - the PRs are smaller, but it also makes introducing some backdoors (e.g. security vulnerabilities) super easy, because noone will be reviewing this code any more...

[mm4tt]

But I know that the only reason for k/perf-tests to keep the vendor dir was the travis presubmit. Given that we don't have the travis presubmit anymore, we're free to remove it. And we should do it, as it'll get rid of the redundancy and make some operations easier, e.g. updating the dependencies will be a few line, easy-to-review change instead of a change with 1k+ modified files.

I guess I disagree with it. Yes - the PRs are smaller, but it also makes introducing some backdoors (e.g. security vulnerabilities) super easy, because noone will be reviewing this code any more...

I think it's quite the opposite. Consider bumping some k8s dep to the next minor, e.g.

k8s.io/apimachinery v0.18.0

to

k8s.io/apimachinery v0.19.0

Then the go mod change will be as above, a one liner. It's impossible to introduce any backdoors there as to do that someone would have to hack the k8s repo and place them there.

On the other hand, with vendor dir, it'll be hundreds of files, and it's super easy to smuggle something there - no one really reviews the thousand of lines "update vendor" changes.

The other argument is that having both vendor and go.mod file creates a huge redundancy in the system. It's easy to imagine a situation where someone updates one, but forget about the other. It can lead to some very hard to debug bugs.

Unless we don't have a strong reason to have both go.mod and vendor dir (and we don't) we shouldn't keep both.

[wojtek-t]

Then the go mod change will be as above, a one liner. It's impossible to introduce any backdoors there as to do that someone would have to hack the k8s repo and place them there.

We have tens of random dependencies, which I have no reason to believe into by default.

Unless we don't have a strong reason to have both go.mod and vendor dir (and we don't) we shouldn't keep both.

I'm actually on the opposite - until we fully understand why there are both in k/k, let's not remove.

[mm4tt]

Then the go mod change will be as above, a one liner. It's impossible to introduce any backdoors there as to do that someone would have to hack the k8s repo and place them there.

We have tens of random dependencies, which I have no reason to believe into by default.

Where do we have "tens of random dependencies"? We have 16 deps in the clusterloader2's go.mod, I don't see a single random one.

perf-tests/clusterloader2/go.mod

Line 54 in 7db196a

github.com/go-errors/errors v1.0.1

Unless we don't have a strong reason to have both go.mod and vendor dir (and we don't) we shouldn't keep both.

I'm actually on the opposite - until we fully understand why there are both in k/k, let's not remove.

I already explained one of the reasons. K/k is not a good example of typical go project when it comes to dependency management, we do a lot of things in our own quirky ways there. It doesn't make sense to look at it as an example.
Looking at the more typical repo, k/test-infra, the vendor dir has been removed there over a year ago - kubernetes/test-infra#14036

[wojtek-t]

I already explained one of the reasons. K/k is not a good example of typical go project when it comes to dependency management, we do a lot of things in our own quirky ways there. It doesn't make sense to look at it as an example.

One example of reason is not a good explanation - the fact that one reason doesn't work for us, doesn't mean that other reasons will not work.

That said:

Looking at the more typical repo, k/test-infra, the vendor dir has been removed there over a year ago - kubernetes/test-infra#14036

That seems like a much stronger reason to me. Conceptually perf-tests repo is quite similar to test-infra so that looks like a good argument to me.

[kushthedude]

Should I amend the PR to move away from vendor modules?

…

On Mon, 14 Sep, 2020, 18:09 Wojciech Tyczynski, ***@***.***> wrote: I already explained one of the reasons. K/k is not a good example of typical go project when it comes to dependency management, we do a lot of things in our own quirky ways there. It doesn't make sense to look at it as an example. One example of reason is not a good explanation - the fact that one reason doesn't work for us, doesn't mean that other reasons will not work. That said: Looking at the more typical repo, k/test-infra, the vendor dir has been removed there over a year ago - kubernetes/test-infra#14036 <kubernetes/test-infra#14036> That seems like a much stronger reason to me. Conceptually perf-tests repo is quite similar to test-infra so that looks like a good argument to me. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1463 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKQMTLVDOEEHSINSZ3UMJHLSFYFJ7ANCNFSM4RHNKZGA> .

[mm4tt]

Should I amend the PR to move away from vendor modules?

Yes :)

[kushthedude]

/cc @mm4tt @wojtek-t

[wojtek-t]

/ok-to-test

[kushthedude]

/retest

[mm4tt]

It's a huge change. Let's do them separately - cl2 & benchmark. It'll be a bit easier.

[kushthedude]

It's a huge change. Let's do them separately - cl2 & benchmark. It'll be a bit easier.

I agree, removing the commits related to cl2

[kushthedude]

@mm4tt I don't guess the test failure is due to the changes are done in the PR, it is unable to fetch a dependency from bitbucket 😕

[mm4tt]

Yeah, this is being addressed in #1482 as we speak.
Please wait for that PR to be merged then retest

[mm4tt]

/retest

[mm4tt]

The PR seems to be touching both cl2 and benchmark files. Please fix

[mm4tt]

Please also update the PR description

[kushthedude]

The PR seems to be touching both cl2 and benchmark files. Please fix

BUILD files are removed from cl2 because they were listed in .gitignore. If the following files are to be committed then should the gitignore be amended?

[wojtek-t]

BUILD files are removed from cl2 because they were listed in .gitignore. If the following files are to be committed then should the gitignore be amended?

Please don't mix them. It's fine to remove those but in a separate PR.

[kushthedude]

Got it, amending the PR

…

On Tue, 22 Sep, 2020, 11:17 Wojciech Tyczynski, ***@***.***> wrote: BUILD files are removed from cl2 because they were listed in .gitignore. If the following files are to be committed then should the gitignore be amended? Please don't mix them. It's fine to remove those but in a separate PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1463 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKQMTLR6QYERCDEMJ5DSB7DSHA3APANCNFSM4RHNKZGA> .

[kushthedude]

Fixed!

[kushthedude]

/cc @mm4tt @wojtek-t

[mm4tt]

/lgtm
/approve

Thanks!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kushthedude, mm4tt

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:

• benchmark/OWNERS [mm4tt]

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