cmd/openshift-install/create: One shot console access #5336
Conversation
wking
force-pushed
the
one-shot-console-access
branch
from
59f666e
to
dca82df
Compare
| consoleRouteTimeout := 10 * time.Minute | ||
| logrus.Infof("Waiting up to %v for the openshift-console route to be created...", consoleRouteTimeout) | ||
| consoleRouteTimeout := 2 * time.Minute | ||
| logrus.Infof("Checking to see if there is a route at %s/%s...", consoleNamespace, consoleRouteName) |
There was a problem hiding this comment.
Do you think that there is any value is checking for the console ClusterOperator to determine whether the installer should expect the console route?
There was a problem hiding this comment.
That would backstop "console operator flubbed their Available handling and claimed Available=True before the route was ready". I'd consider that flub a pretty big console bug though. I guess that means I think it's not the installer's job to backstop the console operator, and that if the console folks are concerned about this potential console failure mode, they could add coverage to their console-specific e2e suite or the OCP-scoped e2e suite to ensure it doesn't happen.
But I'm not too committed to that separation of concerns, so if you'd like a ClusterOperator check as an installer-side backstop, I can add that in.
There was a problem hiding this comment.
I don't have a problem with the approach that you are taking, given that we are now stopping immediately when the console route is not found.
My only gripe is that we are then warning the user that there is no console in the cases where the console is not installed, but that is not relevant yet since we don't yet support not installing the console.
|
@wking: PR needs rebase. 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. |
openshift-ci
bot
added
the
needs-rebase
|
I can revive this if/when folks want it, just let me know. Until then, I'm not going to try to keep on top of rebases, so closing. |
The console may become optional [1], so teach the installer to handle its absence gracefully. We've waited on the console since way back in ff53523 (add logs at end of install for kubeadmin, consoleURL, 2018-12-06, openshift#806). Back then, install-complete timing was much less organized, and since e17ba3c (cmd: wait for the cluster to be initialized, 2019-01-25, openshift#1132) we've blocked on ClusterVersion going Available=True. So the current dependency chain is: 1. Console route admission blocks console operator from going Available=True in its ClusterOperator. 2. Console ClusterOperator blocks cluster-version operator from going Available=True in ClusterVersion. 3. ClusterVersion blocks installer's waitForInitializedCluster. So we no longer need to wait for the route to show up, and can fail fast if we get a clear IsNotFound. I'm keeping a bit of polling so we don't fail an install on a temporary network hiccup. We don't want to drop the console check entirely, because when it is found, we want: * To continue to log that access pathway on install-complete. * To continue to append the router CA to the kubeconfig. That latter point has been done since 4033577 (append router CA to cluster CA in kubeconfig, 2019-02-12, openshift#1242). The motivation in that commit message is not explicit, but the idea is to support folks who naively run 'oc login' with the kubeadmin kubeconfig [2] (despite that kubeconfig already having cluster-root access) when the console route's cert's CA happens to be something that the user's local trust store doesn't include by default. [1]: openshift/enhancements#922 [2]: openshift#1541 (comment)
wking
force-pushed
the
one-shot-console-access
branch
from
dca82df
to
b05f71a
Compare
|
Re-opened and rebased onto master with dca82df -> b05f71a now that openshift/console-operator#665 is failing on the lack of this change or something similar. |
openshift-merge-robot
removed
the
needs-rebase
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon 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 |
openshift-ci
bot
added
the
approved
|
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
The console may become optional (openshift/enhancements#922), so teach the installer to handle its absence gracefully.
We've waited on the console since way back in ff53523 (#806). Back then, install-complete timing was much less organized, and since e17ba3c (#1132) we've blocked on ClusterVersion going
Available=True. So the current dependency chain is:Available=Truein its ClusterOperator.Available=Truein ClusterVersion.waitForInitializedCluster.So we no longer need to wait for the route to show up, and can fail fast if we get a clear
IsNotFound. I'm keeping a bit of polling so we don't fail an install on a temporary network hiccup.We don't want to drop the console check entirely, because when it is found, we want:
That latter point has been done since 4033577 (#1242). The motication in that commit message is not explicit, but the idea is to support folks who [naively run
oc loginwith the kubeadmin kubeconfig][2] (despite that kubeconfig already having cluster-root access) when the console route's cert's CA happens to be something that the user's local trust store doesn't include by default.