Improved platform tests #1291
Improved platform tests #1291
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
| The major issue with splitting the test binaries to be built from two separate | ||
| repositories is that developers wishing to run kubernetes tests will have to | ||
| build the binary manually, from a separate repository and ensure the binary | ||
| is available during test runs. |
There was a problem hiding this comment.
or just pull down the latest tests image.
There was a problem hiding this comment.
developers wishing to run kubernetes tests will have to
build the binary manually, from a separate repository
@bparees it's not clear to me how it affects the suite kubernetes/conformance currently included in the openshift-tests binary when running:
openshift-tests run kubernetes/conformance [args...]
Will the suite kubernetes/conformance keeps available without additional steps or we'll need to run it from the upstream way (building/downloading the binary [, using sonobuoy's embedded k8s suites], etc) ?
There was a problem hiding this comment.
really a question for @soltysh at this point. The current implementation includes the k8s tests in the openshift-tests image and in a separate k8s image, so you can run them from either location (i.e. just having openshift-tests is sufficient to run the k8s tests, though the version of the k8s tests may vary between the two images).
But that is currently done by vendoring the k8s tests into origin, which ultimately we want to stop doing. So i think the end solution looks more like:
separate test images for different tests/suites/components/areas
an aggregated image (openshift-tests) that is built by pulling all those separate images together
and only the aggregated image ships in the payload, and it's all that is needed to run all the tests. That would maintain the current end user experience.
but that is not what has been built/delivered so far, out of a need for expediency.
There was a problem hiding this comment.
@mtulio there are 2 possible ways with the current approach:
- run
openshift-testsas you did before, which will try to pull the appropriatek8s-testsbinary from the release image, and it will use that binary for the test execution; - use
OPENSHIFT_SKIP_EXTERNAL_TESTS=trueenv variable, when invokingopenshift-testswhich will use the embedded version of k8s conformance tests;
We are guaranteeing that the latter approach will also work for cases when pulling the binary from release is not possible for various reasons. When it comes to k8s conformance (which don't change between minor versions) that shouldn't be a problem for you.
Have a look at Risks and Mitigations section, if you don't find answers there, lemme know I'll gladly add whatever details are required.
|
@elmiko @rvanderp3 @mtulio @bostrt @lobziik @julienlim FYI, since this will affect the VCSP work and certification tool. You'll want to work with @soltysh to make sure someone from your team is on the reviewer list for this enhancement. |
| ### Non-Goals | ||
|
|
||
| 1. Abandon existing tests. | ||
| 2. Change the available functionality of `openshift-tests`. |
There was a problem hiding this comment.
IIUC it is not a goal to change the current content of openshift-tests suites, mainly the conformance ones (openshift/conformance and kubernetes/conformance), is that correct?
My concern is the e2e tests from kubernetes/K8s suite are in openshift/conformance, this suite is a base suite used to validate external clusters by partners on VCSP program - and we understand that the e2e tests in this suite cover all needed to validate an OCP installation.
$ ./openshift-tests run openshift/conformance --dry-run |grep -F "$(./openshift-tests run kubernetes/conformance --dry-run |tail -n1)"
"[sig-storage] Subpath Atomic writer volumes should support subpaths with secret pod [Excluded:WindowsDocker] [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]"
$ ./openshift-tests run openshift/conformance --dry-run |grep -F "$(./openshift-tests run kubernetes/conformance --dry-run |head -n1)"
"[sig-api-machinery] AdmissionWebhook [Privileged:ClusterAdmin] listing mutating webhooks should work [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]"
$ ./openshift-tests run openshift/conformance --dry-run |grep -F "$(./openshift-tests run kubernetes/conformance --dry-run |shuf |head -n1)"
"[sig-api-machinery] Watchers should observe an object deletion if it stops meeting the requirements of the selector [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]"
There was a problem hiding this comment.
everything you said is true, i'm not clear what the concern is though?
we can meet the goal of having k8s tests run as part of the openshift/conformance suite w/o including the k8s tests in the openshift-tests binary.
There was a problem hiding this comment.
we can meet the goal of having k8s tests run as part of the openshift/conformance suite w/o including the k8s tests in the openshift-tests binary.
That's what I would like to make sure I got correctly to track changes on the VCSP program/OPCT.
Overall the two steps in this proposal will bring a huge benefit.
There was a problem hiding this comment.
@mtulio correct, we will ensure the current workloads continue working, at least wrt k8s conformance tests.
|
adding myself to cc list for this pr, i have not had a chance to fully digest it yet though. |
elmiko
left a comment
elmiko
left a comment
There was a problem hiding this comment.
this makes sense to me and generally seems like a good idea.
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
|
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten enhancement proposals close after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Reopen the proposal by commenting /close |
|
@openshift-bot: Closed this PR. DetailsIn response to this:
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. |
soltysh
force-pushed
the
test_improvements
branch
2 times, most recently
from
bb05711 to
1ae56a3
Compare
soltysh
force-pushed
the
test_improvements
branch
from
239f7ca to
49b77b4
Compare
soltysh
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| 2. Remove the requirement that all test code lives in a single repository. | ||
| 3. Maintain `openshift-tests` API for running tests. | ||
| 4. Ensure that kubernetes tests are still vendored in `openshift-tests` binary, | ||
| to allow for a fallback when the release payload image is not reachable. |
There was a problem hiding this comment.
I see this one as temporary....once we start having all teams moving their e2e tests into their own repos, we're just going to have to accept that the test images need to be available or the test job fails.
and we're not going to want to maintain this vendoring indefinitely just to give us a fallback (not to mention that falling back is risky since now you're not necessarily running the tests you thought you were)
There was a problem hiding this comment.
For some cases the fallback will be a must, microshift comes to my mind immediately, since they don't provide release image. I'm not sure about hypershift, either. So at least for k8s-related tests I think that might be longer than just temporary. We'll need to re-assess this at a later point in time. That's why I've explicitly called out k8s tests only for backwards compatibility.
There was a problem hiding this comment.
I've added this to "Even more future work" section, there's still work about upgrade tests, so this is a good future thing to look at .
There was a problem hiding this comment.
if nothing else, fallback needs to be an opt-in behavior (do not fallback to another version of the test code silently or by default).
but this is another reason why producing a single image that contains all the test binaries would be a better solution in my mind.
cc @dhellmann since he can better speak to what microshift does today or would want to support in the future.
There was a problem hiding this comment.
Yeah, that explicit fallback is being tracked in openshift/origin#28000 which will eventually allow us to drop the fallback we currently have.
|
|
||
| This proposal might lead to unnecessary proliferation of external tests binaries, | ||
| which might cause problems when the release image is not available, or for local | ||
| development. |
There was a problem hiding this comment.
hard to imagine a situation where the release image is not available to someone who's trying to run tests
There was a problem hiding this comment.
i'll have to defer to @dhellmann on how they manage this today (where they get the openshift-tests binary from, how they distribute it to customers) on the implications there, but in my mind it's another argument for why we're better off having a single test image that contains all the test binaries. Microshift can then consume/reference that image.
There was a problem hiding this comment.
There is no release payload for MicroShift. It looks like the CI job that uses openshift-tests pulls it from an image built as part of the CI system? https://github.com/openshift/release/blob/master/ci-operator/config/openshift/microshift/openshift-microshift-main.yaml#L47
@pacevedom, @pmtk , or @copejon can any of you explain where that binary comes from?
We do not ship the openshift-tests binary as part of MicroShift.
There was a problem hiding this comment.
Following the breadcrumbs it's: inputs.test-bin which is
test-bin:
name: "4.14"
namespace: ocp
tag: tests
Which is imported: Tagging ocp/4.14:tests into pipeline:test-bin.
I think it's this image config
tl;dr: we just take it from a promoted image
There was a problem hiding this comment.
the workflow where someone is trying to test outside of our CI system and has no release payload?
if they aren't having to modify the tests themselves, they could still use the test binaries from a recent payload image(since @soltysh's current proposal is that there would be multiple test images, one per binary, i defer to him on exactly how a consumer/client identifies all the images needed, pulls them, and extracts all the binaries, but that's something that CI will need to be doing too and i expect it will be relatively straightforward and handled by the openshift-tests binary itself)
if they are modifying the tests locally, then i'd expect them to be able to:
- build/get a copy of the openshift-tests wrapper binary
- invoke it while pointing it at their local externalized test binary for their component
(it would only run the tests that are built into openshift-tests plus the tests in their binary, but usually when people are doing this they are already narrowing the set of tests down to a few specific ones anyway).
Do those two options cover your use cases?
There was a problem hiding this comment.
Number 2 is covered in risks sections, where I'm talking about possibility to use locally available binaries instead of pulling them from release. It's not implemented, but it's rather simple. I'll add it to future work section.
There was a problem hiding this comment.
The only downside is you need to know which binaries you care about, but in case of microshift it's rather simple b/c you'll most likely care about k8s mostly.
There was a problem hiding this comment.
Off the top of my head, MicroShift would need tests for Routes, SCCs, and oc, as well. Our current goal is to make the entire suite "work" (pass or skip), so we can add a job to ensure new tests also work on MicroShift (pass or skip). When the suite is split up, we could avoid doing that for the entire suite and focus on the parts that are most important.
If there's going to be a way to use local binaries for the tests, we should be able to come up with a way to get the "right" binaries, so I think my concerns would be covered by adding a bit of detail to the future work section as you suggest.
There was a problem hiding this comment.
Based on Fabio's comment we'll stick with in-binary k8s tests for now, and we'll slowly work our way out in the future, remembering about cases like CSI and Microshift.
@elmiko thx for that, but I think k8s is in better situation, since they could just export the framework for easier consumption, and the fact that they are directly invoking ginkgo they don't require the same sophistication when splitting the tests into separate binaries, since you can easily treat each one as a separate unit (they can be combined, but I'd probably discourage that). Our case is a bit different in that we have the set of monitors which are probing the cluster throughout the entire test run, and only then we execute single tests. Upstream k8s doesn't have that functionality, which makes their tests' execution a bit simpler 😉 |
| and minimize time when we start running newest kubernetes tests. | ||
| 2. Remove the requirement that all test code lives in a single repository. | ||
| 3. Maintain `openshift-tests` API for running tests. | ||
| 4. Ensure that kubernetes tests are still vendored in `openshift-tests` binary, |
There was a problem hiding this comment.
IMO this is a must for now, but I'm not sure if it's worth to keep this goal in the long-term. The fallback argument is important during the first stages, but eventually it's all about maintainability winning the game.
bertinatto
left a comment
bertinatto
left a comment
There was a problem hiding this comment.
LGTM.
I went through this enhancement and except for nits, I don't have anything to add.
I propose we merge this and work out the remaining details in separate changes.
soltysh
added
the
tide/merge-method-squash
| the kubernetes tests vendored since they don't provide release image from which | ||
| we can extract the kubernetes test binaries. | ||
|
|
||
| Re-evaluate [alternative approach](#build-time-assembly---aggregating-binaries) |
There was a problem hiding this comment.
It's probably worth starting this conversation with testplatform in 4.15 to understand what capabilities our CI build system provides and how difficult this actually is long term. I'd really like to see us not have vendored tests. That will discourage separation beyond a few well known cases.
|
We have a significant improvement already merged. I'm ok landing this as a description, but we need to avoid losing sight of future improvements since we're still stuck vendoring at the moment. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@soltysh: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
| To achieve the first step of the proposal, we need to: | ||
| 1. Provide a library for building `<binary>-tests` which exposes two commands: | ||
| - `list` - responsible for listing tests in JSON format; | ||
| - `run` - run a single test, returning results in ginkgo compatible format; |
There was a problem hiding this comment.
This could be run-test, consistently with the main openshift-tests command. Having a compatible API potentially enables interesting future evolutions. For example, external tests may eventually grow some independence from openshift-tests for debugging purposes, and implement other bits of openshift-tests' API.
There was a problem hiding this comment.
That's a good catch, thank you. The actual implementation is using run-test, see https://github.com/openshift/kubernetes/blob/0e0d15b865ffc36177dc8770b4723dc14476a630/openshift-hack/cmd/k8s-tests/k8s-tests.go#L64 Fix in #1547
14 participants
/assign @deads2k