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

Run openshift aggregated behind kube #18652

Merged
merged 3 commits into from
Mar 8, 2018
Merged

Run openshift aggregated behind kube #18652

merged 3 commits into from
Mar 8, 2018

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Feb 16, 2018 •
edited
Loading

This is ready for review. I've split some bits out into separate pulls to make it easier to review. It separates openshift-apiserver and kube-apiserver and successfully produces a working cluster by aggregating openshift-apiserver using cluster up (on my machine).

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 16, 2018
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 16, 2018
@openshift-merge-robot openshift-merge-robot added the vendor-update Touching vendor dir or related files label Feb 16, 2018
@deads2k deads2k force-pushed the hypershift-02-openshift-apiserver branch 5 times, most recently from f56a1bd to d5135a6 Compare February 19, 2018 19:08
@deads2k deads2k added this to the 3.10.0 milestone Feb 19, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Feb 20, 2018

@diegs this is where my head is at.

@deads2k deads2k force-pushed the hypershift-02-openshift-apiserver branch 2 times, most recently from e535466 to 0560aaf Compare February 22, 2018 20:38
@deads2k
Copy link
Contributor Author

deads2k commented Feb 26, 2018

/retest

@deads2k deads2k force-pushed the hypershift-02-openshift-apiserver branch from 0560aaf to 5cce750 Compare February 26, 2018 20:30
@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.

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

@deads2k deads2k force-pushed the hypershift-02-openshift-apiserver branch 3 times, most recently from cf26d46 to df19af4 Compare February 28, 2018 16:29
@deads2k
Copy link
Contributor Author

deads2k commented Feb 28, 2018

/retest

@deads2k deads2k force-pushed the hypershift-02-openshift-apiserver branch 2 times, most recently from fa5fa67 to 5861580 Compare March 1, 2018 13:37
@deads2k
Copy link
Contributor Author

deads2k commented Mar 1, 2018

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Mar 1, 2018

ah, I need hypershift in an image before service-catalog will pass.

@deads2k
Copy link
Contributor Author

deads2k commented Mar 1, 2018

/test end_to_end

@deads2k
Copy link
Contributor Author

deads2k commented Mar 1, 2018

/retest

1 similar comment
@deads2k
Copy link
Contributor Author

deads2k commented Mar 1, 2018

/retest

@deads2k deads2k force-pushed the hypershift-02-openshift-apiserver branch from 9b38e05 to 2d89a7c Compare March 1, 2018 19:47
@deads2k
Copy link
Contributor Author

deads2k commented Mar 1, 2018

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Mar 8, 2018

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Mar 8, 2018

/hold

got a wrong commit in here somehow

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2018
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2018
@deads2k deads2k force-pushed the hypershift-02-openshift-apiserver branch from 77db27b to 40c590b Compare March 8, 2018 12:59
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Mar 8, 2018

/hold cancel

fixed

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2018
mfojtik
mfojtik reviewed Mar 8, 2018
View reviewed changes
pkg/oc/bootstrap/clusterup/staticpods/load.go Outdated
@@ -9,7 +9,7 @@ import (
"github.com/openshift/origin/pkg/oc/bootstrap"
)

func substitute(in string, replacements map[string]string) string {
func Substitute(in string, replacements map[string]string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

why making this public?

soltysh
soltysh reviewed Mar 8, 2018
View reviewed changes
vendor/k8s.io/kubernetes/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go Outdated
@@ -284,6 +284,12 @@ func (s *APIAggregator) AddAPIService(apiService *apiregistration.APIService) er
s.GenericAPIServer.Handler.NonGoRestfulMux.Handle(proxyPath, proxyHandler)
s.GenericAPIServer.Handler.NonGoRestfulMux.UnlistedHandlePrefix(proxyPath+"/", proxyHandler)

enableOapiHack := true
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of this?

Copy link
Contributor

Choose a reason for hiding this comment

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

was asking @deads2k the same... this is to proxy securely to /oapi as there is no way to register that in the apiservice API.... so this is fine and we should remove this when we deprecate /oapi...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I get that, what I was asking why we need this true variable which is checked right in the next line?

@deads2k deads2k force-pushed the hypershift-02-openshift-apiserver branch from 40c590b to 8a8283e Compare March 8, 2018 15:22
@deads2k
Copy link
Contributor Author

deads2k commented Mar 8, 2018

comments addressed.

@jpeeler
Copy link

jpeeler commented Mar 8, 2018 •
edited
Loading

/test service-catalog
Job is building all the origin images now.

@deads2k
Copy link
Contributor Author

deads2k commented Mar 8, 2018

Job is building all the origin images now.

@jpeeler Excellent! Thanks.

@soltysh
Copy link
Contributor

soltysh commented Mar 8, 2018

I have only one question, but overall this lgtm.

@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2018
@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/extended_builds 729d2f7 link /test extended_builds
ci/openshift-jenkins/extended_networking_minimal 8a8283e link /test extended_networking_minimal
ci/openshift-jenkins/service-catalog 8a8283e link /test service-catalog

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
Copy link
Contributor Author

deads2k commented Mar 8, 2018

/hold

SC is still having trouble. passes in "normal" run though.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Mar 8, 2018

https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/18652/test_pull_request_origin_service_catalog/194/ appears to be failing because the origin image does not include UPSTREAM: <carry>: aggregator to proxy oapi to apps.openshift.io server from this pull. I can reproduce the problem if I run with current --version=latest.

It fails on an invocation like oc cluster up --version=latest --service-catalog. We can see oc cluster up --service-catalog --loglevel=5 --server-loglevel=5 --version=7eb90cc work in this test here https://storage.googleapis.com/origin-ci-test/pr-logs/pull/18652/test_pull_request_origin_extended_clusterup/530/build-log.txt when run with the images produced by this pull.

@jpeeler
Copy link

jpeeler commented Mar 8, 2018

Merging this from a service catalog standpoint is fine. The image usage is not properly sorted out yet.

@deads2k
Copy link
Contributor Author

deads2k commented Mar 8, 2018

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2018
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 18778, 18709, 18876, 18897, 18652).

@openshift-merge-robot openshift-merge-robot merged commit 6160a5e into openshift:master Mar 8, 2018
@deads2k deads2k deleted the hypershift-02-openshift-apiserver branch July 3, 2018 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfojtik mfojtik mfojtik left review comments

@soltysh soltysh soltysh approved these changes

@csrwng csrwng Awaiting requested review from csrwng

@mrogers950 mrogers950 Awaiting requested review from mrogers950

Assignees
No one assigned
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. vendor-update Touching vendor dir or related files
Projects
None yet
Milestone
v3.10
Development

Successfully merging this pull request may close these issues.
7 participants
@deads2k @openshift-ci-robot @jpeeler @soltysh @openshift-merge-robot @mfojtik @openshift-bot