-
Notifications
You must be signed in to change notification settings - Fork 86
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
feat: move Prometheus flags to config #1003
Conversation
a9a57d5
to
0642ae6
Compare
76cc01d
to
00843b0
Compare
05609ee
to
1a0cd72
Compare
114a4e1
to
6561000
Compare
6561000
to
c4fe925
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.
One question, otherwise good, thanks!
p := path.Join(secretsDir, pathForSelector(r.opts.PublicNamespace, &monitoringv1.SecretOrConfigMap{Secret: spec.Credentials})) | ||
flags = append(flags, fmt.Sprintf("--export.credentials-file=%q", p)) | ||
} | ||
setContainerExtraArgs(ds.Spec.Template.Spec.Containers, CollectorPrometheusContainerName, "") |
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.
Hm.. why we do this (and GET and UPDATE) if we only set EXTRA_ARGS to empty. Do you want to make it empty explicitly or what?
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 we want to make it explicitly empty. The problem is that customers will permanently have EXTRA_ARGs set without this (or until there's a DaemonSet update).
We continue supporting EXTRA_ARGs on Prometheus in addition to configurations for backwards-compatibility, so if you have both set, behavior may be misleading.
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.
We continue supporting EXTRA_ARGs on Prometheus in addition to configurations for backwards-compatibility, so if you have both set, behavior may be misleading.
Why not atomic change with GMP version? (operator + new perms + collector failing on extra args?). I think for self install logic, we might want to consider safe route for EXTRA_ARGS (I think it's fine to fail on those from certain fork version), but def for managed we could switch atomically, no? What am I missing?
I guess the plan was to best-effort (A) get this running (use no RBAC needed form, except reset part) for a bit to reset their envvars, then (B) remove this UPDATE and RBAC perms. Plus try to have (A) to be on all clusters to reset envvars properly (month? all regular channel?)
Long term we will have to NOT only ask for flags / vars but also for configmap to debug ANYWAY, so why not avoiding A phase?
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.
That's a good idea and thinking through, it should work. Do you want it 1 PR or would merging each PR separately still be okay? The PR to remove the UPDATE perms is available. If you merge it now, it will merge into this branch.
c4fe925
to
c1698a4
Compare
c1698a4
to
379e5e7
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.
Thanks, let's go!
Pre-req: GoogleCloudPlatform/prometheus#179Uses the new config settings instead of CLI flags for prometheus, and makes DaemonSet UPDATE permissions optional.