don't patch deployments anymore #47
Conversation
Signed-off-by: Kent Rancourt <kent.rancourt@microsoft.com>
Codecov Report
@@ Coverage Diff @@
## master #47 +/- ##
==========================================
- Coverage 59.09% 58.77% -0.32%
==========================================
Files 11 11
Lines 638 638
==========================================
- Hits 377 375 -2
- Misses 232 234 +2
Partials 29 29
Continue to review full report at Codecov.
|
README.md
Outdated
| | Annotation | Description | Default | | ||
| | ---------- | ----------- | ------- | | ||
| | `osiris.deislabs.io/enabled` | Enable Osiris for this deployment. Allowed values: `y`, `yes`, `true`, `on`, `1`. | _no value_ (= disabled) | | ||
| | `osiris.deislabs.io/enabled` | Enable Osiris for this deployment. Allowed values: `y`, `yes`, `true`, `on`, `1`. | _no value_ (= disabled). Enabling this ensures the zeroscaler component will scrape and analyze metrics from the deployment's pods. | |
There was a problem hiding this comment.
shouldn't this new sentence be written in the description column, instead of the default one?
There was a problem hiding this comment.
It sure should. Nice catch.
| labels: | ||
| app: nginx | ||
| annotations: | ||
| osiris.deislabs.io/enabled: "true" |
There was a problem hiding this comment.
@krancour I'm wondering if we shouldn't give this annotation a different name, to avoid confusion with the one on the deployment. This one is only used to inject the proxy, so maybe osiris.deislabs.io/injectProxy: "true" ?
I'm saying that because I have something else I'd like to add to osiris: a proxy-less mode. The proxy is only used to collect metrics, but we could imagine different implementations of the metrics collector, with one collecting metrics from a prometheus endpoint. For people already using prometheus, this would have the following benefits:
- complete control over how a request is counted, ie no need to use the
ignoredPaths, and if needed requests can be ignored based on different input: user-agent, source IP, ... - allow the use of another tool that inject a transparent proxy as a sidecar container, like a service mesh.
I imagine using a different metrics collector could be done by:
- not adding the
osiris.deislabs.io/injectProxyannotation on the pods - adding a new annotation on the deployment to enable the prometheus metrics collector, and specify the port, endpoint, metrics names, ...
I'll create a ticket to detail this proposal. so all that just to say that maybe we could use a more-specific name for this annotation ;-)
There was a problem hiding this comment.
New feature aside, I am on board with renaming this annotation (and for that matter, renaming osiris.deislabs.io/enabled in the other places it is used as well) with names that are more unambiguously indicative of the specific behaviors that are being added.
I do, however, think that should happen in a follow-up PR. In the even that renaming some annotations proves controversial, it will be easier to undo.
Following deislabs#47 here, let's rename the annotation used to inject the proxy in the pods: - previous annotation key: `osiris.deislabs.io/enabled` - new annotation key: `osiris.deislabs.io/injectProxy` so that we can avoid confusion between the deployments and pods annotations
backport deislabs#47 and deislabs#53
4 participants
This is a pre-requisite for #38.
This PR simplifies the function of the proxy injector. Instead of looking for an annotation on the deployment and copying that annotation down to the deployment's pod spec template, only to later act on it, users are now required to annotate their deployment's pod spec templates directly.
While this is a breaking change (although it should be noted that Osiris has not had even a single release yet, nevermind a GA one) and a minor inconvenience, this produces much, much more reliable behavior (because there is a likely circumstances in which the proxy injector will not intercept changes to a deployment; see detailed explanation here).
Since #38 is set to expand the configuration options for pods and would be affected by this same issue, it is advisable to get this straightened out first, as #38 will emulate whatever approach we utilize here.
cc @vbehar I would love your 👀 on this if you don't mind.