-
Notifications
You must be signed in to change notification settings - Fork 183
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
Use env for prometheus metrics namespace #676
Conversation
It's strong candidate for backporting. Should I cherry-pick that on the release-1.0/0.17? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for one comment about autogenerated files.
<match prometheus.metrics.apiserver**> | ||
@type sumologic | ||
@id sumologic.endpoint.metrics.apiserver | ||
sumo_client "k8s_1.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is deleted?
Would you mind rebasing on current origin/master and deleting autogenerated commits (so that they can be regenerated once more)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something with rebase goes wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
solved in #672
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<<<<<<< HEAD
=======
above is more interesting
da735c1
to
f5fbbe2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, assuming you tested locally and the prometheus config loaded correctly with the thanos sidecar replacing all the environment variables
Yeah, it works, but I re-tested it with random collector name and I will update app label selector also |
I believe we should create new labels for matching the apps for prometheus, because now it won't be possible to generate it same way like in kubernetes templates: |
I set the label for default configuration as static value. |
*/}} | ||
{{- define "sumologic.labels.app" -}} | ||
{{- template "sumologic.fullname" . }} | ||
{{- printf "collection-sumologic" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any upgrade risks with this change and someone who has used a different release/namespace? I believe labels are immutable and one thing I could see is helm upgrade requiring a --force
flag. If so, it would be good to document in 1.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it requires --force
flag during upgrade
Error: UPGRADE FAILED: StatefulSet.apps "falling-zorse-sumologic-fluentd-events" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', and 'updateStrategy' are forbidden && StatefulSet.apps "falling-zorse-sumologic-fluentd-metrics" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', and 'updateStrategy' are forbidden && StatefulSet.apps "falling-zorse-sumologic-fluentd-logs" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', and 'updateStrategy' are forbidden
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the template is used for all app labels, it shouldn't affect anything (not sure about changed servicemonitors definitions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make sure we add a release note with this change and error and flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created #680 to not forget about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Closing due to #685 -> more clean and safe way |
Description
Get namespace dynamically
Testing performed