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.

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

cluster up support for N-1 clusters #17338

Merged
merged 2 commits into from
Dec 6, 2017

Conversation

bparees
Copy link
Contributor

@bparees bparees commented Nov 15, 2017

switching TSB to its own binary and making sure cluster up can still launch the TSB on older clusters.

supplements/replaces #16800

@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 15, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees

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-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2017
@bparees
Copy link
Contributor Author

bparees commented Nov 16, 2017

/unassign @stevekuznetsov @mfojtik
/assign @csrwng @deads2k

guessing the cluster up tests are going to need some tweaking before this all works right.

@bparees bparees changed the title cluster up support for N-1 clusters [WIP] cluster up support for N-1 clusters Nov 16, 2017
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 16, 2017
@bparees
Copy link
Contributor Author

bparees commented Nov 16, 2017

@deads2k and we believe this doesn't need an ansible change because ansible is just instantiating the TSB template, so it gets the change "for free" right?

@deads2k
Copy link
Contributor

deads2k commented Nov 16, 2017

@deads2k and we believe this doesn't need an ansible change because ansible is just instantiating the TSB template, so it gets the change "for free" right?

@sdodson question to you.

@sdodson
Copy link
Member

sdodson commented Nov 16, 2017

We still need to get the CI jobs in place to automatically sync content from origin to openshift-ansible, but yes that's the intention. I'll see if we can get that in place soon.

Rather than current/previous would it be easier to to just include vX.Y versioned templates?

@bparees
Copy link
Contributor Author

bparees commented Nov 16, 2017

Rather than current/previous would it be easier to to just include vX.Y versioned templates?

we are only supporting N-1, so current/previous makes more sense to me than renaming dirs and changing code every release. This way we just drop the right templates in the right dirs and the rest just works.

@bparees bparees force-pushed the tsb_clusterup branch 2 times, most recently from 9075af7 to e30d327 Compare November 16, 2017 23:56
@bparees
Copy link
Contributor Author

bparees commented Nov 17, 2017

/retest

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2017
@bparees bparees force-pushed the tsb_clusterup branch 2 times, most recently from 37a57a7 to 3961765 Compare November 19, 2017 00:09
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2017
@bparees bparees force-pushed the tsb_clusterup branch 3 times, most recently from 14b7211 to c550fe4 Compare November 20, 2017 20:57
@bparees
Copy link
Contributor Author

bparees commented Nov 30, 2017

/retest

@deads2k
Copy link
Contributor

deads2k commented Nov 30, 2017

/hold

I'd like to land this after we branch for 3.8. I think the last two pulls we need are #17516 and #17357 . I'm double checking now.

@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 Nov 30, 2017
@bparees
Copy link
Contributor Author

bparees commented Nov 30, 2017

/retest

@deads2k
Copy link
Contributor

deads2k commented Dec 1, 2017

/hold cancel
/retest

@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 Dec 1, 2017
@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.

@bparees
Copy link
Contributor Author

bparees commented Dec 4, 2017

/retest

@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2017
@bparees
Copy link
Contributor Author

bparees commented Dec 4, 2017

@deads2k the new image is failing because the routing doesn't appear to be setup properly:

  <*client.ServerError | 0xc42154ad20>: {
        StatusCode: 500,
        Description: "no matches for /, Kind=Route",
    }
    Internal Server Error: no matches for /, Kind=Route
I1204 20:55:58.536279       1 wrap.go:42] GET /healthz: (5.840178ms) 200 [[kube-probe/1.8] 10.128.0.1:38144]
I1204 20:56:00.824457       1 bind.go:138] Template service broker: Bind: instanceID aadda50d-d92c-402d-bd29-5ed2095aad2c, bindingID fadb6a68-dba5-4138-b9a5-424c79e154a7
I1204 20:56:00.941671       1 route.go:31] Service broker: call to /brokers/template.openshift.io returned no matches for /, Kind=Route
I1204 20:56:00.941996       1 wrap.go:42] PUT /brokers/template.openshift.io/v2/service_instances/aadda50d-d92c-402d-bd29-5ed2095aad2c/service_bindings/fadb6a68-dba5-4138-b9a5-424c79e154a7: (119.958549ms) 500

https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/17338/test_pull_request_origin_extended_conformance_install/3458/

I can't help but suspect this is related to your part of this change...

_ "k8s.io/kubernetes/pkg/api/install"
_ "k8s.io/kubernetes/pkg/apis/autoscaling/install"
_ "k8s.io/kubernetes/pkg/apis/batch/install"
_ "k8s.io/kubernetes/pkg/apis/extensions/install"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deads2k fyi this was necessary for the TSB to be able to do restmapping successfully. And it seems utterly unsustainable.

@bparees bparees added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2017
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2017
@bparees bparees added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2017
@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 5, 2017

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

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_conformance_install_update d572e4b link /test extended_conformance_install_update
ci/openshift-jenkins/extended_conformance_crio 5d9b047 link /test crio

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.

@bparees
Copy link
Contributor Author

bparees commented Dec 5, 2017

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit afeda2c into openshift:master Dec 6, 2017
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

I realize this has already merged, but I had a couple comments after rebasing #17575

// brought up matches the client binary being used. This needs to
// be updated each release.
func clusterVersionIsCurrent(v semver.Version) bool {
return v.GT(openshiftVersion37)
Copy link
Member

Choose a reason for hiding this comment

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

Won't this return true for 3.7.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, good catch, can you address this in your PR? :)

"--loglevel=2" # can't be empty, so pass something benign

# Test the code being delivered
"--loglevel=2 --version=${ORIGIN_COMMIT}"
Copy link
Member

Choose a reason for hiding this comment

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

ORIGIN_COMMIT shouldn't be used before a default is set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, thanks. i didn't notice since i was only running this in the CI env that always set it. I see you've addressed it in your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.