Skip to content

Use custom script to invoke registry extended test suite#729

Closed
miminar wants to merge 2 commits into
openshift-eng:masterfrom
miminar:extended-image-registry-suite
Closed

Use custom script to invoke registry extended test suite#729
miminar wants to merge 2 commits into
openshift-eng:masterfrom
miminar:extended-image-registry-suite

Conversation

@miminar
Copy link
Copy Markdown

@miminar miminar commented Oct 13, 2017

The test/extended/core.sh script does not allow to properly focus only the desired tests to run.

This script contains the necessary bits from the core.sh scripts to setup the environment and run just the registry tests.

Once openshift/origin#16846 merges, the registry tests won't be runnable using the extended test scripts because most of them are run serially.

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 13, 2017
@miminar
Copy link
Copy Markdown
Author

miminar commented Oct 13, 2017

/assign @Kargakis
/cc @bparees

I haven't tested it. I'm not sure what I need to do to test it in CI before merging. I'd welcome any pointers.

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Oct 13, 2017

run hack/generate.sh and include those changes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not need merge_junit anymore. i'm trying to get it removed here:
https://github.com/openshift/origin/pull/16068/files

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Oct 13, 2017

and for something like this that isn't going to break anyone else, we can just test it live. i'm guessing you do not have access to create jobs on the jenkins server, however?

@bparees bparees self-assigned this Oct 13, 2017
@stevekuznetsov
Copy link
Copy Markdown
Contributor

What's the developer workflow to do this from an Origin checkout?

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Oct 13, 2017

@stevekuznetsov there isn't a good one. and i'm not happy about it, but go see openshift/origin#16846 for that discussion.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will fail to detect $OS_ROOT correctly since the script that runs is not under the Origin tree. I may have made that work in the past but I can't remember. You should test that this script, when saved in some part of the filesystem NOT in the Origin tree, works as expected.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I at least assume that $OS_ROOT is set?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, nothing sets it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miminar i'm holding merging/creating this based on @stevekuznetsov's response, but if i'm wrong let's talk tomorrow and i can set it up for you and we can see what happens.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the "$(pwd)" holds the truth. I've made "${OS_ROOT}" guessing a bit more robust in this script just in case...

@miminar miminar force-pushed the extended-image-registry-suite branch from ae35ec7 to a39fff5 Compare October 16, 2017 08:20
@miminar
Copy link
Copy Markdown
Author

miminar commented Oct 16, 2017

Comments addressed (hopefully).

and for something like this that isn't going to break anyone else, we can just test it live

OK, let's test it live.

@miminar miminar force-pushed the extended-image-registry-suite branch from a39fff5 to 32cf232 Compare October 16, 2017 10:10
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 16, 2017
@miminar
Copy link
Copy Markdown
Author

miminar commented Oct 18, 2017

I've created new test_pull_request_origin_extended_image_registry_custom jenkins job to try out the changes. I'll update this PR once I get it working.

Update: tested & updated

Michal Minář added 2 commits October 18, 2017 20:43
The `test/extended/core.sh` script does not allow to properly focus only
the desired tests to run.

This script contains the necessary bits from the `core.sh` scripts to
setup the environment and run just the registry tests.

Signed-off-by: Michal Minář <miminar@redhat.com>
Signed-off-by: Michal Minář <miminar@redhat.com>
@miminar miminar force-pushed the extended-image-registry-suite branch from 32cf232 to 7b9a497 Compare October 18, 2017 18:44
@miminar
Copy link
Copy Markdown
Author

miminar commented Oct 18, 2017

Any other comments?

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Oct 18, 2017

@miminar in light of openshift/origin#16919 is this still necessary?

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Oct 18, 2017

also i only see one [registry] test? (registry.go)

Are all the registry tests in that one file?

(there's also signature.go but that is all disabled)

@miminar
Copy link
Copy Markdown
Author

miminar commented Oct 19, 2017

$ ack --noheading --go '\[Feature:Image|\[registry\]' -- test/extended/
test/extended/images/hardprune.go:20:var _ = g.Describe("[Feature:ImagePrune][Serial] Image hard prune", func() {
test/extended/images/prune.go:31:var _ = g.Describe("[Feature:ImagePrune][Serial] Image prune", func() {
test/extended/images/resolve.go:14:var _ = g.Describe("[Feature:ImageLookup] Image policy", func() {
test/extended/imageapis/limitrange_admission.go:24:var _ = g.Describe("[Feature:ImageQuota][Serial] Image limit range", func() {
test/extended/imageapis/quota_admission.go:29:var _ = g.Describe("[Feature:ImageQuota][Serial] Image resource quota", func() {
test/extended/registry/registry.go:28:var _ = g.Describe("[Conformance][registry][migration][Serial] manifest migration from etcd to registry storage", func() {
test/extended/registry/signature.go:15:var _ = g.Describe("[imageapis][registry][Skipped] image signature workflow", func() {

Registry tests often begin with [Feature:Image as can be seen above.

The problem with the current regexp "\[Feature:Image|\[registry\].*?\[Serial\]" is that either [registry] followed by [Serial] will be matched or just [Feature:Image will be matched. Therefor several tests will be run twice:

  • [Conformance][registry][migration][Serial] manifest migration from etcd to registry storage" is matched both by FOCUS="\[Serial\].*?${t}" and FOCUS="${t}.*?\[Serial\]".
  • [Feature:ImageLookup] Image policy is matched both by FOCUS="${t}.*?\[Serial\]" and the parallel test suite.
    • the same applies to the other registry tests missing [Serial] tag.

The focus of this job could be rewritten to (\[Feature:Image|\[registry\]) but I still would like this job to explicitly pick up the particular tests to run. Run them just once and run them all serial - because it's much more easy to debug. Since this job is run only against registry related PRs, it shouldn't pose a problem. Debugging a failure with a registry log containing overlapping pushes from 3 different tests run in parallel is a task I'd like to avoid.

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Oct 19, 2017

@miminar how about we just consistently tag all our registry tests with [registry] or [image] and then you can set the focus to [registry] (or [image]) and be done with it?

title: "run extended tests"
repository: "origin"
script: |-
for pth in ${OS_ROOT:-} "$(pwd)" "$(dirname ${BASH_SOURCE})" "/data/src/github.com/openshift/origin"; do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the test env you can be certain Origin is at /data/src/github.com/openshift/origin

@miminar
Copy link
Copy Markdown
Author

miminar commented Oct 20, 2017

@miminar how about we just consistently tag all our registry tests with [registry] or [image] and then you can set the focus to [registry] (or [image]) and be done with it?

I'd prefer [registry]. But I'd still preserve [Feature:ImagePruning] tag for local testing.
Shall we address our desire to run registry tests serially by using [Serial] tag as well? We will eventually switch to parallel test runs when each registry test will spawn its own registry replica.

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Oct 20, 2017

I'd prefer [registry]. But I'd still preserve [Feature:ImagePruning] tag for local testing.

sure, you can have as many tags as you like.

Shall we address our desire to run registry tests serially by using [Serial] tag as well?

yes.

@miminar
Copy link
Copy Markdown
Author

miminar commented Oct 20, 2017

Closing in favor of openshift/origin#16967

@miminar miminar closed this Oct 20, 2017
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Oct 20, 2017
Automatic merge from submit-queue (batch tested with PRs 16912, 16931, 16939, 16967, 16978).

extended: annotated registry tests

To make them easily focus-able in CI.

Obsoletes openshift-eng/aos-cd-jobs#729
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants