Remove overlapping tests with Prow

[Adirio]

Description

Prow and Travis are overlapping. This PR removes the e2e tests from Travis but leaves the local tests, as the OSX version isn't run by prow.

Motivation

#1344

[k8s-ci-robot]

Hi @Adirio. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

[Adirio]

@alexeldeib @mengqiy

[Adirio]

• Travis runs both tests with the latest version of k8s only and in both platforms.

Just a comment about this part of your proposal. E2E tests can't be performed in OSX.

About the issue you comment: not being able to see the result of e2e tests before /ok-to-test is set, I think that it basically removes the purpose of ok-to-test. It is a flag to prevent attacks, a reviewer that sets this flag is saying: "Ok, this PR can be tested. It may file, but it is not an attack". An example of an attack that is avoided with ok-to-test would be DoS (Denial of Service), submitting a big fake PR or a big ammount of fake PRs that keeps CI resources busy.

So I think the question we should debate is, which tests should we gate behind the ok-to-test flag.

Some things to bear in mind (correct me if I'm wrong):

• OSX related tests need to be run in Travis.
• ok-to-test flag only works for Prow tests.
• Can we control the flow of Prow tests the same way we do in Travis? Gating some tests behind others?

My opinion:

• scripts/verify.sh: linting. No need to gate it behind anything as it is lighweight and fast. Submitters being able to have this result before a reviewer sees the code means that the code will be cleaner when the reviewer inspects it. [TRAVIS]
• golden_test.sh: determines if the scaffolding has changed. It is not as lighweight or fast as linting but it isn't slow either. [ANY]
• test.sh: unitary tests for both kubebuilder and the scaffolded projects in testdata. Should be gated behind golden_test.sh so that we can claim that testdata is up to date. Similar to golden_test.sh in load and speed for linux, slower for osx. Needing osx support means: [TRAVIS]
• test_e2e.sh: performs e2e tests with different k8s versions using KinD. Heaviest and slowest tests, so the later they are run, the more tests that can fail before, meaning that if one test fails before it won't consume resources. [PROW]
• Coverage: low importance as of today, so lowest priority despite not needing that many resources. It basically runs unitary tests again on the main source packages and reports the coverage. Should only be gated. [TRAVIS]

If it wasn't for the osx requirement, I would move golden_test.sh and test.sh to Prow also as the second is already running the unitary tests.

[Adirio]

With your proposal, which is the current behavior, ok-to-test is useless. We are doing tests before the flag is set, all of them actually. Which is the purpose of setting ok-to-test if you already know the outcome of the tests? How does ok-to-test protect you from attacks if tests are performed without the flag?

I don't think the costs as you said are a real issue, as the curren trend has been towards using prow. The osx requirement is the main stopper IMO, so golden_test.sh and test.sh will have to stay in Travis for now. Remember that the free Travis CI is limited in the number of concurrent processes and it is unsafer. Imagine someone makes a PR where he changes .travis.yaml to include thousands of test combinations (easily doable just adding some OS, goversions and env variables combiantions). That would consume Travis resources for days.

[Adirio]

The /ok-to-test will trigger prow which will perform the e2e tests to ensure the compatibility with old versions for the Linux platform only.

That's basically nothing. Most of the tests would already be run without an aproval of a reviewer.

• As a reviewer, we can stop any Travis process

Not protecting something from an attack just because a certain user can manually block the attack is a recipe for a disaster in ciber-security.

• Also, the review process ensure that Travis and tests were not changed wrongly.

Not true, if you open this PRs Travis, you will see that it uses the Travis config from this PR, instead of using it from master. That may be another thing we may want to change, but in that case we would not know if a change to .travis.yaml file was successulf until after merging it to master and triggering Travis again for another PR.

[camilamacedo86]

Hi @Adirio,

Also, I forgot to share that Travis will kill a process that did not finish after X time as it has a lot of mechanisms to prevent scenarios as you described besides all be executed in isolate envs. So, you do not need to have concerns such as over an attack against it or if it is safe to be used. It is a commonly used and very mature project.

[Adirio]

Yes, and it is the goal. Would not great for who contribute needs waiting for manual intervention to check if their PR is passing or not in the tests. See that the needs of the flag ok to tests is just to save cost and do not trigger many times the prow unnecessary. (Travis is free but Prow is not)

Unitary tests can (and should) be run locally before PR as part of the development process. Coverage also executes unitary tests. Linting is also performed. The prupose of ok-to-test is to gate tests behind reviewer aproval. If all tests are going to be executed before, the way to go would be removing the need of ok-to-test once and for all.

Travis is an isolated environment and because of this is commonly used. If has any "attack" will be against the Travis which is a new process / env machine created for each round. So, I am quite sure that is pretty safe.

I'm not saying that they are going to break Travis, but they can overload it. DoS attack. Good PRs will not have resources to pass their Travis tests because Travis is busy with rogue PRs.

The Travis needs to be trigged from the PR since its purpose is to check the changes.

Travis should obviouslly use the code from PR, but we can probably enforce it to use the .travis.yaml file from master. Just that file. That way you can't change the behavior of Travis for your own PR. But thats a different topic, lets not discuss that here as it is just off-topic.

[DirectXMan12]

The intent of ok-to-test is that all tests run behind it, IIRC. Folks get to skip ok-to-test once they're org members, so it shouldn't be to much of a burden.

IMO, we should try to move most things to prow, since it's easier to do stuff like manually retrigger runs & prevent folks from abusing our lint scripts.

Plus, anything on prow gets run as part of merge, so the more we put there, the less likely we are to have scenarios like merges breaking lints.

[Adirio]

Travis:
Keep the golden/lint/coverage/unit/e2e test just for the latest k8s version for the supportable platforms.

I agree with some, disagree with others.

• Lint: leaving it in Travis helps with contributions before the ok-to-test flag is set, and reviewers should require contributors to fix style errors before setting the flag. Adding lint to Prow would ensure that it runs before merge.
• Coverage: setting it in Prow doesn't make sense IMO, as coverage is not (at least yet) part of the review-merge process.
• Golden test: contributors that do not submit regularly don't usually know that if the golden test fails, you should run golden_generate.sh, and a reviewer has to tell them about that. Contributors that do PR regularly and know this will probably belong to kubernetes-sigs so their tests will be run automatically. So I don't think we loose that much contributor-friendliness by moving this to Prow. So my choice would be to move this to Prow and remove it from Travis.
• Unit and e2e tests: definitely should be in Prow to be sure that we do not merge bugs. Removing them from Travis sounds like the thing to do as they are the heavy part and as @DirectXMan12 says, ok-to-test purpose is that all checks are run behind it. If we leave them in Travis they will be run without the ok-to-test flag, which is not desired. There is only one issue, Prow doesn't support osx unitary tests, so that will need to stay in Travis.

• If we move all for the prow means that contributors will need to wait for manual intervention to check if their changes passed

Exactly, that is the purpose of ok-to-test. If this behaviour is not wanted, we should just remove this flag from the project.

• Reviewers will be unable to ensure the basics before the flag ( as for example be sure that the testdata is properly updated )

Why would you need to run all the tests to know if you can set a flag that allows to run tests? It doesn't make sense.

• More fast CI results for contributors(no members) and reviewers.

They are not always faster. Prow tests may take a bit longer if they both start at the same time, but travis processes are limited, so when there are a couple PRs, which is pretty common, all processes will not start and the waiting time can be quite big.

• Prow do not allow us to ensure other platforms (mac and win)

True, and that seems to not be something temporal. It doesn't look as supporting other OS is coming soon.

So basically I would say:

• Prow: everything except other OS and coverage*1
• Travis: linting, other OS unit tests and coverage

*1 To make coverage part of the merge process, we should probably investigate how to tell coverall.io which PR it is coming from and all that stuff. Also, it would be needed develop a lot of tests for kubebuilder, which has almost no unit tests (which are the ones reporting coverage, e2e tests do not help with this).

[mengqiy]

Reviewers will be unable to ensure the basics before the flag ( as for example be sure that the testdata is properly updated )

IMO contributors are supposed to ensure tests are passing locally before submitting a PR.
We should have a detailed documentation in https://github.com/kubernetes-sigs/kubebuilder/blob/master/CONTRIBUTING.md to tell the contributors what tests to run locally before creating a PR.

Golden test: contributors that do not submit regularly don't usually know that if the golden test fails, you should run golden_generate.sh, and a reviewer has to tell them about that.

We instruct them in the failure message:

kubebuilder/golden_test.sh

Line 49 in 48ea4cb

header_text "Please make sure you have run ./generate_golden.sh if you have changed the scaffolding"

Plus, anything on prow gets run as part of merge, so the more we put there, the less likely we are to have scenarios like merges breaking lints.

This is a big advantage of Prow over Travis. Prow is smarter to figure out if a test result is considered to be out-of-date.

IIRC Prow currently supports Linux and Windows, but not MacOS. kubebuilder supports Linux and MacOS.
My opinion is to move everything but not test.sh and Coverage to Prow.

[Adirio]

IIRC Prow currently supports Linux and Windows, but not MacOS. kubebuilder supports Linux and MacOS.

Windows? I was told today that prow runs over k8s and that k8s doesn't run on native windows (docker on windows is basically a linux kernel with HyperV). But the point is clear, it doesn't support MacOS and kubebuilder does.

So this PR removes e2e tests from Travis as they are already in Prow, before moving the golden test we will need to implement it in Prow.

[mengqiy]

It seems Prow allow using Windows nodes in a k8s cluster.
I came across the testing config file of sig-windows: https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-windows/windows-gce.yaml

[mengqiy]

I created #1357 to track the contributor doc efforts

[camilamacedo86]

Hi @mengqiy, @Adirio

I am cool with the decision to move these tests to Prow.
However, I think that still important requirement that needs to be addressed over it so:

IMO contributors are supposed to ensure tests are passing locally before submitting a PR.

1. Allow contributors to test locally
   (Suggestion)

• make target to run the tests
• properly doc it in the CONTRIBUTION.MD
• ensure that the test runs in any platform ( currently, they work just in the Linux. )
  Issue tracked: Allow contributors to test locally #1364

2. Ensure the quality of the project in at least supported platforms ( Mac / Linux )
   To do it, I think we need just to run the golden script in both to ensure that commands work.
   Issue tracked: Ensure the kubebuilder works properly in the supported platform #1363

3. I think we could track a task for we start to run the tests in WIN as well in order to support it in the future.

IHMO: we need at least track this needs in an issue. WDYT? Is it make sense?

[camilamacedo86]

After the discussions over the topic. I think that it is ok to be merged since it is just removing the duplication of the tests. I mean, it is just removing from Travis the tests that are already done in Prow.

So, for me it is:
/lgtm

PS.: IHMO we could/should track the points discussed in new issues. Let me know if you agree with @mengqiy and @Adirio and if has something that I can do to help.

[camilamacedo86]

/lgtm

[mengqiy]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, mengqiy

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 [mengqiy]

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

[mengqiy]

It needs a decent amount of work to support it in windows.
And it has to be the next major release, since we have to make some breaking change to support windows. e.g. generated file should be .yml instead of .yaml.