Skip to content

Conversation

@sallyom
Copy link
Contributor

@sallyom sallyom commented Dec 19, 2018

This PR adds a check for kubeadmin user login. This check is a good indication that all is well in a cluster after the kubeadmin user is patched in (KubeAPIServerOperatorConfig).
@smarterclayton

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sallyom
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: russellb

If they are not already assigned, you can assign the PR to them by writing /assign @russellb in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 19, 2018
@sallyom sallyom changed the title use oc config context with oc login check in cluster-launch-installer templates use oc config set/use-context with kubeadmin login check in cluster-launch-installer templates Dec 19, 2018
Copy link
Contributor

Choose a reason for hiding this comment

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

be consistent in use of ${ ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see what value failing has here?

Copy link
Contributor

Choose a reason for hiding this comment

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

All the other steps are "waits", not fails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, made it an until..; do \ sleep 10

@sallyom sallyom force-pushed the add-kubeadmin-login-to-installer-templates branch 2 times, most recently from b0cadaf to 9d1cc69 Compare December 22, 2018 15:17
@sallyom sallyom force-pushed the add-kubeadmin-login-to-installer-templates branch from 9d1cc69 to 94ba816 Compare January 2, 2019 16:37
@sallyom sallyom force-pushed the add-kubeadmin-login-to-installer-templates branch from 94ba816 to 90f98d3 Compare January 3, 2019 15:39
done
# oh god the blood
sleep 180
# can remove this check once a proper test is added to origin e2e
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not write the e2e test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm doing that now. This ensures no breaking changes are merged b4 that test is in place (auth team is refactoring, configuring oauth, etc now). However, with how long this has taken to merge, might-as-well wait for the e2e, as i'm finally able to get to it now.

Copy link
Contributor Author

@sallyom sallyom Jan 3, 2019

Choose a reason for hiding this comment

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

This is, however, a good indicator that a cluster is healthy, ready to execute the e2e suite. Configuring the kubeadmin user requires a patch, takes a few minutes for the cluster to recover from that. Might want to keep this check in, but I'll close if not, since I'll have the e2e test done soonish.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't the installer handle those before handing off the cluster as "ready" ?

@wking

Copy link
Contributor

Choose a reason for hiding this comment

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

I am just very suspicious of any bash in here ..

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this feels like something that should be part of the installer, not post installer. Or if it shouldn't, we should have a good reason not to (like we don't want the password in the clear, but the private key is in the clear, so it's kind of a wash).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wking I don't see how this could be part of the installer?
a) Installer would run oc login and that would require oc be available?
or
b) Installer would have to vendor origin code to run the equivalent of oc login?
maybe I'm missing something, clue me in please if so.
I've opened a PR to add a smoke-4 test for kubeadmin login, instead: openshift/origin#21733

@sallyom
Copy link
Contributor Author

sallyom commented Jan 6, 2019

closing this in favor of openshift/origin#21733

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants