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

UPSTREAM: 58293: fail earlier on discovery failures #18137

Closed
wants to merge 2 commits into from
Closed

UPSTREAM: 58293: fail earlier on discovery failures #18137

wants to merge 2 commits into from

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Jan 17, 2018

Picking a fail-early pull for when we're unable to lookup resource information from the apiserver.

/assign juanvallejo

@openshift-ci-robot
Copy link

[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.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2018
@openshift-merge-robot openshift-merge-robot added the vendor-update Touching vendor dir or related files label Jan 17, 2018
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 17, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Jan 17, 2018

/retest

4 similar comments
@deads2k
Copy link
Contributor Author

deads2k commented Jan 17, 2018

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Jan 17, 2018

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Jan 18, 2018

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Jan 18, 2018

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Jan 19, 2018

@juanvallejo this is exposing a problem in oc process and discovery mapper lookups. Can you chase it down?

@juanvallejo
Copy link
Contributor

@deads2k sure, taking a look

juanvallejo
juanvallejo reviewed Jan 19, 2018
View reviewed changes
vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util/factory_object_mapping.go
if err != nil {
glog.V(3).Infof("Unable to retrieve API resources, falling back to hardcoded types: %v", err)
return legacyscheme.Registry.RESTMapper(), legacyscheme.Scheme, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Error is caused here: when we run oc process expecting not to contact the server, returning a mapper for hardcoded types would suffice. With this patch, we are actually continuing on, and using a deferred discovery mapper down below.

@juanvallejo juanvallejo mentioned this pull request Jan 22, 2018
Closed
@deads2k deads2k closed this Jan 24, 2018
@deads2k deads2k deleted the cli-13-fail branch January 24, 2018 14:29
@deads2k deads2k restored the cli-13-fail branch January 24, 2018 14:29
@deads2k deads2k reopened this Jan 24, 2018
@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 Jan 31, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Jan 31, 2018

@juanvallejo see the second commit.

@juanvallejo
Copy link
Contributor

@deads2k opened PR upstream: kubernetes/kubernetes#59118

Some tests in /test cmd are failing with panics, looking..

Locally, oc label panics as well.

@openshift-ci-robot
Copy link

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

Test name Commit Details Rerun command
ci/openshift-jenkins/cmd d61acb1 link /test cmd
ci/openshift-jenkins/verify d61acb1 link /test verify
ci/openshift-jenkins/end_to_end d61acb1 link /test end_to_end
ci/openshift-jenkins/unit d61acb1 link /test unit
ci/openshift-jenkins/extended_conformance_install d61acb1 link /test extended_conformance_install
ci/openshift-jenkins/gcp d61acb1 link /test gcp

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.

@deads2k deads2k closed this Apr 20, 2018
@deads2k deads2k deleted the cli-13-fail branch July 3, 2018 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juanvallejo juanvallejo juanvallejo left review comments

@fabianofranz fabianofranz Awaiting requested review from fabianofranz

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@juanvallejo juanvallejo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. vendor-update Touching vendor dir or related files
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@deads2k @openshift-ci-robot @juanvallejo @openshift-merge-robot