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

Support web console image for cluster up #17575

Merged
merged 2 commits into from
Jan 10, 2018

Conversation

spadgett
Copy link
Member

@spadgett spadgett commented Dec 4, 2017

https://trello.com/c/9oaUh8xP

Install and run the web console server from a template during cluster up. Requires the proxy from #17862 to actually use the console server.

@deads2k @jwforres

@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 Dec 4, 2017
@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 4, 2017
@jwforres
Copy link
Member

jwforres commented Dec 4, 2017

@bparees
Copy link
Contributor

bparees commented Dec 4, 2017

please add a test (or enhance an existing test) in clusterup.sh that confirms the web console is working.

name: openshift-web-console
annotations:
openshift.io/display-name: OpenShift Web Console
description: The API server for the OpenShift web console.
Copy link
Member

Choose a reason for hiding this comment

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

remove "API"

@spadgett spadgett force-pushed the console-cluster-up branch 2 times, most recently from 4503b9d to a9d4110 Compare December 4, 2017 17:04
@spadgett
Copy link
Member Author

spadgett commented Dec 4, 2017

please add a test (or enhance an existing test) in clusterup.sh that confirms the web console is working.

Thanks, I added a basic test for the console.

function os::test::extended::clusterup::verify_console () {
os::cmd::expect_success "oc login -u system:admin"
os::cmd::expect_success_and_text "oc get svc -n openshift-web-console" "webconsole"
os::cmd::try_until_text "oc get endpoints webconsole -o jsonpath='{ .subsets[*].ports[?(@.name==\"https\")].port }' -n openshift-web-console" "8443" $(( 10*minute )) 1
Copy link
Contributor

Choose a reason for hiding this comment

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

indenting

@spadgett spadgett force-pushed the console-cluster-up branch 2 times, most recently from 310a478 to 089f736 Compare December 4, 2017 18:19
@spadgett
Copy link
Member Author

spadgett commented Dec 5, 2017

/retest

@spadgett
Copy link
Member Author

spadgett commented Dec 5, 2017

I believe the cluster up tests are failing because there's no v3.9.0-alpha.0 console image.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2017
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2017
@spadgett
Copy link
Member Author

spadgett commented Dec 6, 2017

Rebased for #17338

@bparees For now, I special cased the console tests not to run N-1 since it is only for 3.9. We might have the same problem again down the road when we add new features. Let me know if you can think of a cleaner way to handle it.

// for the web console
consoleTemplateLocations = map[string]string{
"web console server template": "install/origin-web-console/console-template.yaml",
}
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason you didn't just put this in the "internalCurrentTemplateLocations" array?
(the comments about it only being used w/ the service catalog is requested are inaccurate, you can clean them up :) )

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now the template is only imported on 3.9+ clusters, which is why it's in a separate map. We could always import it, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I see internalCurrentTemplateLocations only installs in 3.9. I'll update.

@@ -1040,6 +1094,10 @@ func useAnsible(v semver.Version) bool {
return v.GTE(openshiftVersion36)
}

func useWebConsoleTemplate(v semver.Version) bool {
return v.GTE(openshiftVersion39)
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use v.GT(openshiftVersion37) instead to avoid introducing yet another version number constant.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not clear to me looking at the doc if that will return true for 3.7.1?

https://godoc.org/github.com/blang/semver#Version.GT

I might need to make this greater than v3.9.0-alpha.0 to avoid the extended test failures.

Copy link
Contributor

Choose a reason for hiding this comment

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

it should return true for 3.7.1.

Copy link
Member Author

Choose a reason for hiding this comment

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

it should return true for 3.7.1.

Cluster up will fail if it tries to use the web console image for 3.7.1 since the image doesn't exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok well all the "current/previous" logic is impacted by that. so it should all be updated to check for 3.9.

if [ "$test" == "console" ]; then
cleanup_func=$("os::test::extended::clusterup::cleanup_func" "${test}")
# trap "${cleanup_func}; os::test::extended::clusterup::junit_cleanup" EXIT
os::test::extended::clusterup::run_test "${test}" "--loglevel=2"
Copy link
Contributor

Choose a reason for hiding this comment

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

pass the --version here instead of hardcoding it in your test. that way when someone removes this special case things will "just work" the way they're supposed to.

@spadgett spadgett force-pushed the console-cluster-up branch 6 times, most recently from 37586fa to fe38017 Compare December 13, 2017 15:03
@spadgett spadgett force-pushed the console-cluster-up branch 2 times, most recently from 5813b35 to d3c9541 Compare December 13, 2017 16:51
path: /healthz
port: 8443
scheme: HTTPS
nodeSelector: "${{NODE_SELECTOR}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

What do double curlies do?

Copy link
Member Author

Choose a reason for hiding this comment

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

It lets you use values that aren't strings.

https://docs.openshift.org/latest/dev_guide/templates.html#writing-parameters

When using the ${{PARAMETER_NAME}} syntax only a single parameter reference is allowed and leading/trailing characters are not permitted. The resulting value will be unquoted unless, after substitution is performed, the result is not a valid json object. If the result is not a valid json value, the resulting value will be quoted and treated as a standard string.

volumes:
- name: serving-cert
secret:
defaultMode: 420
Copy link
Contributor

Choose a reason for hiding this comment

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

Not 0440?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or 0400?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I've switched it to 0400, which works.

It looks like this was copied from the TSB template. I haven't changed that one.

@deads2k
Copy link
Contributor

deads2k commented Jan 8, 2018

Still a WIP?

@spadgett spadgett changed the title [WIP] support web console image for cluster up Support web console image for cluster up Jan 8, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2018
@spadgett spadgett force-pushed the console-cluster-up branch from 7611157 to caba0dc Compare January 8, 2018 14:14
@spadgett
Copy link
Member Author

spadgett commented Jan 8, 2018

@deads2k thanks, updated

@spadgett
Copy link
Member Author

spadgett commented Jan 8, 2018

/retest

1 similar comment
@spadgett
Copy link
Member Author

spadgett commented Jan 8, 2018

/retest

@deads2k
Copy link
Contributor

deads2k commented Jan 8, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, spadgett

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 8, 2018
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2018
@spadgett
Copy link
Member Author

spadgett commented Jan 8, 2018

@deads2k The e2e tests were failing because there no was console image tagged with the origin commit for cluster up. I have a PR open in aos-cd-jobs to build the image for the e2e tests like the image registry container image.

openshift-eng/aos-cd-jobs#982

I added a second commit to this PR that tags origin-web-console:latest as the origin commit, for the moment anyway.

@stevekuznetsov FYI

@spadgett spadgett added the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2018
@spadgett
Copy link
Member Author

spadgett commented Jan 9, 2018

flake #17529

/retest

spadgett added a commit to spadgett/openshift-ansible that referenced this pull request Jan 9, 2018
Update the web console template based on changes in
openshift/origin#17575
spadgett added a commit to spadgett/openshift-ansible that referenced this pull request Jan 9, 2018
Update the web console template based on changes in

openshift/origin#17575
@spadgett
Copy link
Member Author

spadgett commented Jan 9, 2018

/test unit

@spadgett
Copy link
Member Author

spadgett commented Jan 9, 2018

/retest

@spadgett
Copy link
Member Author

spadgett commented Jan 9, 2018

It looks like the cluster failed to provision in the conformance gce tests

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit a027e96 into openshift:master Jan 10, 2018
@spadgett spadgett deleted the console-cluster-up branch January 11, 2018 01:10
openshift-merge-robot added a commit to openshift/openshift-ansible that referenced this pull request Jan 11, 2018
Automatic merge from submit-queue.

Update web console template

Update the web console template based on changes in

openshift/origin#17575

/assign @sdodson 
@deads2k fyi
leseb pushed a commit to leseb/openshift-ansible that referenced this pull request Dec 14, 2018
Update the web console template based on changes in

openshift/origin#17575
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.

6 participants