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

Add prometheus-to-sd-exporter to metadata-proxy addon; bump to v0.1.4 #54653

Merged
merged 1 commit into from
Nov 3, 2017
Merged

Add prometheus-to-sd-exporter to metadata-proxy addon; bump to v0.1.4 #54653

merged 1 commit into from
Nov 3, 2017

Conversation

ikehz
Copy link
Contributor

@ikehz ikehz commented Oct 26, 2017

What this PR does / why we need it: Add metrics exporters to the metadata-proxy addon for GCE. Work toward #8867.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 26, 2017
@ikehz ikehz mentioned this pull request Oct 26, 2017
Merged
@ikehz
Copy link
Contributor Author

ikehz commented Oct 26, 2017

Block on GoogleCloudPlatform/k8s-metadata-proxy#5.

ikehz
ikehz commented Oct 26, 2017
View reviewed changes
cluster/addons/metadata-proxy/gce/metadata-proxy.yaml Outdated
@@ -40,13 +42,22 @@ spec:
limits:
memory: "32Mi"
cpu: "50m"
# BEGIN_PROMETHEUS_TO_SD
- name: prometheus-to-sd-exporter
image: gcr.io/google_containers/prometheus-to-sd:v0.1.2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@loburm Should this be v0.2.2?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please use 0.2.2 (as I remember 0.1.2 even don't have --source flag). Also it may have sense to add pod_name and pod_namespace, see #54635

Just pay attention that in future we will change pod name to pod id (currently not all kubernetes downward api support it)

@ikehz ikehz mentioned this pull request Oct 26, 2017
Merged
@ikehz
Copy link
Contributor Author

ikehz commented Oct 26, 2017

/assign @crassirostris

@ikehz ikehz mentioned this pull request Oct 26, 2017
Closed
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 26, 2017
@cjwagner
Copy link
Member

/retest

1 similar comment
@cjwagner
Copy link
Member

/retest

loburm
loburm reviewed Oct 27, 2017
View reviewed changes
cluster/addons/metadata-proxy/gce/metadata-proxy.yaml Outdated
image: gcr.io/google_containers/prometheus-to-sd:v0.2.2
command:
- /monitor
- --stackdriver-prefix={{ prometheus_to_sd_prefix }}/addons/metadata_proxy
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need to add metadata_proxy as a prefix. Metric name is built as:
prefix/component_name/(metric_name without component_name)

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; how does it know the component_name?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's taken from the first part of --source flag. So if you specify multiple sources, each one is going to have a separate stackdriver prefix.

loburm
loburm reviewed Oct 27, 2017
View reviewed changes
cluster/gce/gci/configure-helper.sh Outdated
@@ -1864,6 +1864,8 @@ EOF
fi
if [[ "${ENABLE_METADATA_PROXY:-}" == "simple" ]]; then
setup-addon-manifests "addons" "metadata-proxy/gce"
local -r metadata_proxy_yaml="${dst_dir}/metadata-proxy/gce/metadata-proxy.yaml"
update-prometheus-to-sd-parameters ${metadata_proxy_yaml}
Copy link
Contributor

Choose a reason for hiding this comment

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

The same is required for the corresponding file in container-linux dir.

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.

crassirostris
crassirostris reviewed Oct 27, 2017
View reviewed changes
Copy link

@crassirostris crassirostris left a comment

Choose a reason for hiding this comment

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

SGTM, but would wait until @loburm lgtms

cluster/addons/metadata-proxy/gce/metadata-proxy.yaml Outdated
- name: config-volume
configMap:
name: metadata-proxy-config
- name: ssl-certs
Copy link

Choose a reason for hiding this comment

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

Please remove this volume, this is not a requirement for prom-to-sd and will be removed across all of K8s soon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay! Done.

@ikehz
Copy link
Contributor Author

ikehz commented Oct 30, 2017

@loburm ptal.

@ikehz
Copy link
Contributor Author

ikehz commented Oct 30, 2017

(Tests aren't passing because v0.1.4 doesn't exist yet (xref GoogleCloudPlatform/k8s-metadata-proxy#5). Waiting on #52744.

@loburm
Copy link
Contributor

loburm commented Oct 31, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2017
@ikehz
Copy link
Contributor Author

ikehz commented Oct 31, 2017

/assign @cjcullen

@ikehz
Copy link
Contributor Author

ikehz commented Oct 31, 2017

https://github.com/GoogleCloudPlatform/k8s-metadata-proxy/releases/tag/v0.1.4 now exists.

@ikehz
Copy link
Contributor Author

ikehz commented Oct 31, 2017

/retest

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2017
@ikehz
Copy link
Contributor Author

ikehz commented Oct 31, 2017

Addressed @cjcullen's comments, plus another rebase.

@ikehz ikehz closed this Oct 31, 2017
@ikehz ikehz reopened this Oct 31, 2017
@ikehz
Copy link
Contributor Author

ikehz commented Oct 31, 2017

Gah, didn't mean to close.

@cjcullen
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2017
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2017
@ikehz
Copy link
Contributor Author

ikehz commented Nov 2, 2017

Added change in e2e image, which was causing e2e failures. Ptal.

@loburm
Copy link
Contributor

loburm commented Nov 2, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2017
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 3, 2017
Add prometheus-to-sd-exporter to metadata-proxy addon; bump to proxy …
be8aaf9 
…to v0.1.4 and e2e to v0.0.2; remove configmag
@mikedanese
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjcullen, ihmccreery, loburm, mikedanese

Associated issue: 8867

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

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2017
@k8s-github-robot
Copy link

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

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 3, 2017 •
edited

@ihmccreery: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-device-plugin-gpu be8aaf9 link /test pull-kubernetes-e2e-gce-device-plugin-gpu

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.

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 2a40c48 into kubernetes:master Nov 3, 2017
@shyamjvs shyamjvs mentioned this pull request Nov 14, 2017
Closed
@shyamjvs
Copy link
Member

This PR seems to cause a scalability regression - #55695.

@shyamjvs shyamjvs mentioned this pull request Nov 14, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crassirostris crassirostris crassirostris left review comments

@cjcullen cjcullen cjcullen left review comments

@loburm loburm loburm left review comments

Assignees

@crassirostris crassirostris

@mikedanese mikedanese

@cjcullen cjcullen

@loburm loburm

@Q-Lee Q-Lee

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@ikehz @cjwagner @loburm @cjcullen @mikedanese @k8s-github-robot @k8s-ci-robot @shyamjvs @crassirostris @Q-Lee