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

added readiness check to LDAP server pod #5064

Merged

Conversation

stevekuznetsov
Copy link
Contributor

Implemented an exec readiness probe, with the following check:

$ bash -c "ldapsearch -x -b dc=example,dc=com | grep organization"

This should catch the organization in the dc=example,dc=com as well as all of the organizationalUnit entries we have. This currently never returns a true ... cc @liggitt @deads2k thoughts appreciated

@stevekuznetsov
Copy link
Contributor Author

I have a feeling this is due to me not understanding exec fully ... but let's be honest that man page was terrifying to look at and I came out dumber than I went in

@stevekuznetsov stevekuznetsov mentioned this pull request Oct 15, 2015
27 tasks
@stevekuznetsov stevekuznetsov force-pushed the skuznets/ldap-readiness branch 2 times, most recently from af75ee7 to af37153 Compare October 21, 2015 12:38
@stevekuznetsov
Copy link
Contributor Author

@liggitt @deads2k PTAL

},
"readinessProbe": {
"exec": {
"command": ["bash", "-c", "\"ldapsearch -x -b dc=example,dc=com | grep organization\""]
Copy link
Contributor

Choose a reason for hiding this comment

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

surprised the quoting on the third arg is needed... is it not treated as a single arg to bash?

@openshift-bot openshift-bot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 22, 2015
@smarterclayton
Copy link
Contributor

Status of this?

@stevekuznetsov
Copy link
Contributor Author

@smarterclayton In-flight. I'll have bandwidth for this once prune-groups is finished. Until then we have a sleep in the test suite ... is that long wait what you're not liking at the moment?

@smarterclayton
Copy link
Contributor

Just asking as I was going through the older PRs

On Oct 29, 2015, at 8:12 AM, Steve Kuznetsov notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton In-flight. I'll have
bandwidth for this once prune-groups is finished. Until then we have a sleep
in the test suite ... is that long wait what you're not liking at the
moment?


Reply to this email directly or view it on GitHub
#5064 (comment).

@stevekuznetsov stevekuznetsov changed the title [wip] added readiness check to LDAP server pod added readiness check to LDAP server pod Dec 21, 2015
@stevekuznetsov
Copy link
Contributor Author

The check now runs ldapsearch -x -b dc=example,dc=com and will fail if nothing has been initialized with error 32, succeed once server is ready. @deads2k @liggitt PTAL

@deads2k
Copy link
Contributor

deads2k commented Dec 21, 2015

Will this be checked by the extended test? If so, run it and I'll merge on a pass.

@stevekuznetsov stevekuznetsov force-pushed the skuznets/ldap-readiness branch 2 times, most recently from 98d7c9c to b486b06 Compare December 21, 2015 18:03
@stevekuznetsov
Copy link
Contributor Author

@deads2k it should be checked, yes

[testonlyextended:ldap_groups]

@stevekuznetsov
Copy link
Contributor Author

I guess that syntax doesn't work. OK.
@gabemontero I've seen you use similar before, how do I tell Jenkins to just run one extended suite?

[test][extended:ldap_groups]

@gabemontero
Copy link
Contributor

@stevekuznetsov hey - don't want to put the precise string (cause it will kick a build off), but it is "testonlyextended" in between '[' and ']', followed by another '[' ']' pairing, with "core" and then the focus you want (i.e. ldap in this case I would imagine) in between parens.

If that doesn't make sense, just ping me in IRC.

@stevekuznetsov
Copy link
Contributor Author

Very unclear how valid_all_ldap_sync_delete_prune file got written as a .txt, when #6323 merged with a green extended run... I distinctly remember fixing this problem in that pull.

@stevekuznetsov
Copy link
Contributor Author

flake on TestAuthorizationRestrictedAccessForProjectAdmins here:

E1222 02:32:20.109854   20090 create_dockercfg_secrets.go:109] secrets "builder-token-9onls" already exists
--- FAIL: TestAuthorizationRestrictedAccessForProjectAdmins-4 (12.84s)
    authorization_test.go:45: unexpected error: namespaces "hammer-project" already exists

#6173
re[test]

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2015
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 5dd76bf

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 23, 2015
@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/8088/) (Extended Tests: ldap_groups)

@stevekuznetsov
Copy link
Contributor Author

@deads2k @liggitt got green

@liggitt
Copy link
Contributor

liggitt commented Dec 23, 2015

LGTM, [merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4474/) (Image: devenv-rhel7_3031)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 5dd76bf

openshift-bot pushed a commit that referenced this pull request Dec 23, 2015
@openshift-bot openshift-bot merged commit 6bd3bad into openshift:master Dec 23, 2015
@stevekuznetsov stevekuznetsov deleted the skuznets/ldap-readiness branch January 8, 2016 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants