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.

Sign up for GitHub

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

Jump to bottom

bug 1537857. Fix retrieving prometheus metrics #6903

Merged
merged 1 commit into from Feb 2, 2018
Merged

bug 1537857. Fix retrieving prometheus metrics #6903

merged 1 commit into from Feb 2, 2018

Conversation

jcantrill
Copy link
Contributor

@jcantrill jcantrill commented Jan 26, 2018 •
edited

This PR:

  • Generates a password file for use when retrieving metrics from elasticsearch

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 26, 2018
@jcantrill jcantrill mentioned this pull request Jan 26, 2018
Merged
ewolinetz
ewolinetz reviewed Jan 26, 2018
View reviewed changes
roles/openshift_logging_elasticsearch/templates/passwd.j2 Outdated
@@ -0,0 +1,2 @@
"{{_user_name}}":
Copy link
Contributor

Choose a reason for hiding this comment

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

_user_name and _user_passwd may be too generic of variable names (possibly affected by creep).
Could you make this a little more specific? Maybe just prefix it with logging_passwd_file

ewolinetz
ewolinetz reviewed Jan 26, 2018
View reviewed changes
roles/openshift_logging_elasticsearch/tasks/main.yaml
@@ -391,6 +403,7 @@
es_container_security_context: "{{ _es_containers.elasticsearch.securityContext if _es_containers is defined and 'elasticsearch' in _es_containers and 'securityContext' in _es_containers.elasticsearch else None }}"
deploy_type: "{{ openshift_logging_elasticsearch_deployment_type }}"
es_replicas: 1
basic_auth_passwd: "{{ _logging_metrics_proxy_passwd | b64decode }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

we cant leave this encoded here and decode it on our side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but that means its encoded in the DC, base64 encoded again in the request. We would need to double decode it during the check. I put the decode here so it wasnt plan text in the log file when running the installer. I wanted to avoid bzs that might come up.

ewolinetz
ewolinetz reviewed Jan 26, 2018
View reviewed changes
roles/openshift_logging_elasticsearch/templates/es.j2 Outdated
@@ -51,6 +51,7 @@ spec:
- -client-id={{openshift_logging_elasticsearch_prometheus_sa}}
- -client-secret-file=/var/run/secrets/kubernetes.io/serviceaccount/token
- -cookie-secret={{ 16 | lib_utils_oo_random_word | b64encode }}
- -basic-auth-password={{basic_auth_passwd | default('changeme')}}
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably don't need a default here if there's no way it can't be generated

@jcantrill jcantrill force-pushed the 1537857_fix_logging_prometheus branch from 79068e9 to 93814bd Compare January 26, 2018 21:46
@ewolinetz
Copy link
Contributor

/retest

@ewolinetz
Copy link
Contributor

/test logging

1 similar comment
@jcantrill
Copy link
Contributor Author

/test logging

@jcantrill
Copy link
Contributor Author

/retest

@ewolinetz
Copy link
Contributor

bot, retest this please

@ewolinetz
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2018
@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@jcantrill
Copy link
Contributor Author

/retest

@jcantrill
Copy link
Contributor Author

/test install
/test gcp
/test crio

@ewolinetz
Copy link
Contributor

bot, retest this please

@ewolinetz
Copy link
Contributor

/test gcp

@ewolinetz
Copy link
Contributor

/retest

@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@openshift-ci-robot
Copy link

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

Test name Commit Details Rerun command
ci/openshift-jenkins/install 93814bd link /test install
ci/openshift-jenkins/extended_conformance_install_crio 93814bd 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.

@sdodson sdodson merged commit fbdfa66 into openshift:master Feb 2, 2018
@sdodson
Copy link
Member

sdodson commented Feb 2, 2018

flake

@jcantrill jcantrill deleted the 1537857_fix_logging_prometheus branch February 2, 2018 13:50
@jcantrill
Copy link
Contributor Author

/cherrypick release-3.7

@openshift-cherrypick-robot
Copy link

@jcantrill: #6903 failed to apply on top of branch "release-3.7":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	roles/openshift_logging_elasticsearch/tasks/main.yaml
M	roles/openshift_logging_elasticsearch/templates/es.j2
Falling back to patching base and 3-way merge...
Auto-merging roles/openshift_logging_elasticsearch/templates/es.j2
CONFLICT (content): Merge conflict in roles/openshift_logging_elasticsearch/templates/es.j2
Auto-merging roles/openshift_logging_elasticsearch/tasks/main.yaml
Patch failed at 0001 bug 1537857. Fix retrieving prometheus metrics

In response to this:

/cherrypick release-3.7

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.

@jcantrill jcantrill mentioned this pull request Feb 2, 2018
Merged
openshift-merge-robot added a commit to openshift/origin-aggregated-logging that referenced this pull request Feb 6, 2018
@openshift-merge-robot
Merge pull request #920 from jcantrill/1537857_fix_prometheus
6c5c6a5 
Automatic merge from submit-queue.

bug 1537857. Fix retrieving prometheus metrics

This PR:

* bumps openshift-elasticsearch-plugin to fix retrieving metrics
* bumps the prometheus exporter to fix issue related to SG and plugin

Depends on: 
* fabric8io/openshift-elasticsearch-plugin#119
* https://github.com/fvvanholl/elasticsearch-prometheus-exporter/pull/82
* openshift/openshift-ansible#6903
openshift-merge-robot added a commit that referenced this pull request Feb 13, 2018
@openshift-merge-robot
Merge pull request #6998 from jcantrill/fix_prom_37
6e17701 
Automatic merge from submit-queue.

[3.7] Fix promethus for logging

3.7 backport of #6903 for https://bugzilla.redhat.com/show_bug.cgi?id=1510320
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ewolinetz ewolinetz ewolinetz left review comments

Assignees

@ewolinetz ewolinetz

Labels
affects_3.7 lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jcantrill @ewolinetz @openshift-merge-robot @openshift-ci-robot @sdodson @openshift-cherrypick-robot