Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

e2e: manifest url cache #71703

Closed
wants to merge 3 commits into from
Closed

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Dec 4, 2018

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Manually mirroring files from upstream URLs is error prone. It is hard
to verify during a review whether the files are actually in sync with
their origin, or even determine what that origin is.

On the other hand, directly downloading via URLs during a test or
build increases the risk of random failures in CI jobs.

This problem gets addressed by:

  • referencing test files via URL
  • registering those URLs in the test/e2e/e2e.test binary
    before tests run
  • loading the content of the files from a local cache
    (either via file or bindata)
  • adding tooling that updates resp. verifies the cached files

The URL cache is placed under test/e2e/testing-manifests even though
the files might not always be .yaml manifest files because that
directory already gets built into the binary via bindata.

The hack/verify-url-cache.sh is meant to be used in a (to be created)
Prow pre-submit job that triggers only when test/e2e gets
updated. That way the normal "verify" job will not be affected by
the (potentially flaky) download verification.

Registering normal files is also beneficial, because then the test
suite can check in advance whether the file is available. Otherwise a
complete test run is needed to detect a missing file, or the error
might not be detected at all when the test that uses it doesn't run.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 4, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pohly
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: gmarek

If they are not already assigned, you can assign the PR to them by writing /assign @gmarek in a comment when ready.

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:

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

@pohly
Copy link
Contributor Author

pohly commented Dec 4, 2018

/hold

kubernetes-retired/drivers#124 and #71667 need to be merged first.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2018
@pohly
Copy link
Contributor Author

pohly commented Dec 4, 2018

/cc @msau42

This is the proposal for referencing manifest in the upstream repos. Does this concept look right to you?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 23, 2018
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 23, 2018
@pohly
Copy link
Contributor Author

pohly commented Dec 23, 2018

/hold cancel

Although kubernetes-retired/drivers#124 was merged, we can't use the YAML files from that repo because they reference the wrong images. I changed the PR such that it only references those files via URL that are usable, i.e. the sidecar RBAC rules.

While working on the update it occurred to me that registering plain files is useful to detect missing files quickly and reliably, so I've added that.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 23, 2018
@pohly pohly changed the title WIP: e2e: manifest url cache e2e: manifest url cache Dec 23, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 23, 2018
@pohly
Copy link
Contributor Author

pohly commented Dec 28, 2018

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-e2e-gce

Tests might want to use there own options for specifying a file path,
while still using the abstract file access API. For example,
framework.CreateFromManifests might be used to create a mixture of
files under the repo root and from elsewhere. To support this,
absolute paths can now be given to the testfiles package and they will
be read directly.
Manually mirroring files from upstream URLs is error prone. It is hard
to verify during a review whether the files are actually in sync with
their origin, or even determine what that origin is.

On the other hand, directly downloading via URLs during a test or
build increases the risk of random failures in CI jobs.

This problem gets addressed by:
- referencing test files via URL
- registering those URLs in the test/e2e/e2e.test binary
  before tests run
- loading the content of the files from a local cache
  (either via file or bindata)
- adding tooling that updates resp. verifies the cached files

The URL cache is placed under test/e2e/testing-manifests even though
the files might not always be .yaml manifest files because that
directory already gets built into the binary via bindata.

The hack/verify-url-cache.sh is meant to be used in a (to be created)
Prow pre-submit job that triggers only when test/e2e gets
updated. That way the normal "verify" job will not be affected by
the (potentially flaky) download verification.

Registering normal files is also beneficial, because then the test
suite can check in advance whether the file is available. Otherwise a
complete test run is needed to detect a missing file, or the error
might not be detected at all when the test that uses it doesn't run.
Generated via hack/update-bazel.sh.
@pohly
Copy link
Contributor Author

pohly commented Jan 8, 2019

The feedback from SIG-testing in the meeting today was that these .yaml files shouldn't be needed by the E2E test suite at all. Instead, drivers should be installed as part of the test cluster setup. How that could work is open though, and I suspect that we'd still need to move .yaml files around.

As a result, I'm not sure whether I can get someone from SIG-testing to review this PR. They seem more interested in that long-term solution.

@k8s-ci-robot
Copy link
Contributor

@pohly: PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2019
@pohly
Copy link
Contributor Author

pohly commented Jan 10, 2019

The long-term direction is to move the driver installation into the test cluster setup phase. Therefore the approach with framework.testfiles is at best an interim solution. Therefore no-one was inerested enough to review and approve it and I am closing the PR.

SIG-storage will have to continue with manually keeping .yaml files in sync. Once it is clear where those files will be used in the long run (might still be in kubernetes/kubernetes, just in a different directory), some automatism for updating and verifying them can be introduced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants