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.

Sign up for GitHub

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

Jump to bottom

image: add ImageTag to etcd test #24458

Merged
merged 1 commit into from Jan 28, 2020
Merged

image: add ImageTag to etcd test #24458

merged 1 commit into from Jan 28, 2020

Conversation

mfojtik
Copy link
Member

@mfojtik mfojtik commented Jan 27, 2020

This should address following failure:

fail [github.com/openshift/origin/test/extended/etcd/etcd_test_runner.go:72]: test failed:
no test data for image.openshift.io/v1, Kind=ImageTag.  Please add a test for your new type to etcdStorageData.
etcd data does not match the types we saw:
in etcd data but not seen:
[]
seen but not in etcd data:
[
	image.openshift.io/v1, Resource=imagetags

Found by @smarterclayton in https://testgrid.k8s.io/redhat-openshift-ocp-release-4.4-informing#release-openshift-ocp-installer-e2e-metal-serial-4.4

Question: Why was this test not failing before and we only found this on metal?

/cc @bparees
/cc @adambkaplan

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 27, 2020
@mfojtik
Copy link
Member Author

mfojtik commented Jan 27, 2020

/test release-openshift-ocp-installer-e2e-metal-serial-4.4

@mfojtik
Copy link
Member Author

mfojtik commented Jan 27, 2020

/test installer-e2e-metal-serial

@mfojtik
Copy link
Member Author

mfojtik commented Jan 27, 2020

/test e2e-metal-serial

@p0lyn0mial
Copy link
Contributor

it doesn't affect only metal, it affects API data in etcd should be stored at the correct location and version for all resources test.

The new resource was added in openshift/openshift-apiserver#51 and the test wasn't updated.

The test only runs as a part of a "serial test suite" for example e2e-aws-serial

@p0lyn0mial
Copy link
Contributor

Why openshift/openshift-apiserver#51 didn't run e2e-aws-serial?

@p0lyn0mial
Copy link
Contributor

/test e2e-aws-serial

@bertinatto
Copy link
Member

/retest

@bertinatto
Copy link
Member

bertinatto commented Jan 27, 2020 •
edited

This needs to merged manually, right? Otherwise e2e-aws-serial will keep failing?

Edit: I mean, after all tests pass (except for e2e-aws-serial).

@p0lyn0mial
Copy link
Contributor

@mfojtik it looks like the stub is broken - https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/24458/pull-ci-openshift-origin-master-e2e-aws-serial/12341

I think it should be {"metadata": {"name": "itag1"}, "spec": {"name": "tag"}, "status": {"tag":"itag1"}}

@p0lyn0mial
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2020
@p0lyn0mial
Copy link
Contributor

This needs to merged manually, right? Otherwise e2e-aws-serial will keep failing?

No, this pull should fix e2e-aws-serial (unless it has more issues)

@smarterclayton
Copy link
Contributor

lgtm as well

@bparees
Copy link
Contributor

bparees commented Jan 27, 2020

Why openshift/openshift-apiserver#51 didn't run e2e-aws-serial?

e2e-aws-serial isn't enabled on openshift-apiserver. sounds like it should be added?

@p0lyn0mial
Copy link
Contributor

e2e-aws-serial isn't enabled on openshift-apiserver. sounds like it should be added?

Yes, I'm going to add it - created a placeholder - https://bugzilla.redhat.com/show_bug.cgi?id=1795194 for it.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@smarterclayton
Copy link
Contributor

Actually, changing my LGTM. ImageTags are virtual, and won't be in etcd. They're basically the same as image stream tags.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2020
@mfojtik
Copy link
Member Author

mfojtik commented Jan 27, 2020

@smarterclayton alright, updated.

@smarterclayton
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@marun marun mentioned this pull request Jan 27, 2020
Merged
@marun
Copy link
Contributor

marun commented Jan 28, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

11 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@marun
Copy link
Contributor

marun commented Jan 28, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@marun
Copy link
Contributor

marun commented Jan 28, 2020 •
edited

This can't merge due to the failing oauth check in e2e-aws-serial resulting from incompatibility with bound tokens. Options to merge include:

  1. force merging this PR once all tests but the oauth check are passing
  2. force merging the fix for auth compatibility: UPSTREAM: <carry>: oauth-authn: add implicit audience support #24461
  3. reverting bound tokens: Revert 718+727: audience support because TokenReview API is broken with legacy SAs cluster-kube-apiserver-operator#734

I think 1 is suggested, and the decision whether to fix or revert bound token support handled separately.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@smarterclayton
Copy link
Contributor

Force merging, test passes

@smarterclayton smarterclayton merged commit d3552fb into openshift:master Jan 28, 2020
@openshift-ci-robot
Copy link

@mfojtik: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-gcp a4aa46f link /test e2e-gcp
ci/prow/e2e-aws-serial a4aa46f link /test e2e-aws-serial
ci/prow/e2e-aws-fips a4aa46f link /test e2e-aws-fips

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

2 similar comments
@openshift-ci-robot
Copy link

@mfojtik: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-gcp a4aa46f link /test e2e-gcp
ci/prow/e2e-aws-serial a4aa46f link /test e2e-aws-serial
ci/prow/e2e-aws-fips a4aa46f link /test e2e-aws-fips

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@openshift-ci-robot
Copy link

@mfojtik: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-gcp a4aa46f link /test e2e-gcp
ci/prow/e2e-aws-serial a4aa46f link /test e2e-aws-serial
ci/prow/e2e-aws-fips a4aa46f link /test e2e-aws-fips

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adambkaplan adambkaplan Awaiting requested review from adambkaplan

@bparees bparees Awaiting requested review from bparees

Assignees

@smarterclayton smarterclayton

@p0lyn0mial p0lyn0mial

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@mfojtik @p0lyn0mial @bertinatto @smarterclayton @bparees @openshift-bot @openshift-ci-robot @marun