Skip to content
This repository was archived by the owner on Jan 27, 2021. It is now read-only.

don't patch deployments anymore #47

Merged
krancour merged 2 commits into from
Oct 23, 2019
Merged

don't patch deployments anymore #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
221cbf4
don't patch deployments anymore
krancour Oct 22, 2019
7eeb667
doc fixes
krancour Oct 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 23 additions & 9 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,20 @@ development.__

## How it works

For Osiris-enabled deployments, Osiris automatically instruments application
pods with a __metrics-collecting proxy__ deployed as a sidecar container.
Various types of Kubernetes resources can be Osiris-enabled using an annotation.

For any Osiris-enabled deployment that is _already_ scaled to a configurable
minimum number of replicas (one, by default), the __zeroscaler__ component
continuously analyzes metrics from each of that deployment's pods. When the
Osiris-enabled pods are automatically instrumented with a __metrics-collecting
proxy__ deployed as a sidecar container.

Osiris-enabled deployments (if _already_ scaled to a configurable minimum number
of replicas-- one by default) automatically have metrics from their pods
continuously scraped and analyzed by the __zeroscaler__ component. When the
aggregated metrics reveal that all of the deployment's pods are idling, the
zeroscaler scales the deployment to zero replicas.

Under normal circumstances, scaling a deployment to zero replicas poses a
problem: any services that select pods from that deployment (and only that
deployment) would lose all of their endpoints and become permanently
deployment) would lose _all_ of their endpoints and become permanently
unavailable. Osiris-enabled services, however, have their endpoints managed by
the Osiris __endpoints controller__ (instead of Kubernetes' built-in endpoints
controller). The Osiris endpoints controller will automatically add Osiris
Expand Down Expand Up @@ -138,10 +140,16 @@ spec:
metadata:
labels:
app: nginx
annotations:
osiris.deislabs.io/enabled: "true"
Copy link
Contributor

@vbehar vbehar Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@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/injectProxy annotation 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 ;-)

Copy link
Contributor Author

@krancour krancour Oct 23, 2019

Choose a reason for hiding this comment

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

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.

# ...
# ...
```

Note that the template for the pod _also_ uses an annotation to enable Osiris--
in this case, it enables the metrics-collecting proxy sidecar container on all
of the deployment's pods.

In Kubernetes, there is no direct relationship between deployments and services.
Deployments manage pods and services may select pods managed by one or more
deployments. Rather than attempt to infer relationships between deployments and
Expand Down Expand Up @@ -176,18 +184,24 @@ The following table lists the supported annotations for Kubernetes `Deployments`

| 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 the zeroscaler component to scrape and analyze metrics from the deployment's pods and scale the deployment to zero when idle. Allowed values: `y`, `yes`, `true`, `on`, `1`. | _no value_ (= disabled) |
| `osiris.deislabs.io/minReplicas` | The minimum number of replicas to set on the deployment when Osiris will scale up. If you set `2`, Osiris will scale the deployment from `0` to `2` replicas directly. Osiris won't collect metrics from deployments which have more than `minReplicas` replicas - to avoid useless collections of metrics. | `1` |
| `osiris.deislabs.io/metricsCheckInterval` | The interval in which Osiris would repeatedly track the pod http request metrics. The value is the number of seconds of the interval. Note that this value override the global value defined by the `zeroscaler.metricsCheckInterval` Helm value. | _value of the `zeroscaler.metricsCheckInterval` Helm value_ |

#### Pod Annotations

| Annotation | Description | Default |
| ---------- | ----------- | ------- |
| `osiris.deislabs.io/enabled` | Enable the metrics collecting proxy sidecar container to be injected into this pod. Allowed values: `y`, `yes`, `true`, `on`, `1`. | _no value_ (= disabled) |

#### Service Annotations

The following table lists the supported annotations for Kubernetes `Services` and their default values.

| Annotation | Description | Default |
| ---------- | ----------- | ------- |
| `osiris.deislabs.io/enabled` | Enable Osiris for this service. Allowed values: `y`, `yes`, `true`, `on`, `1`. | _no value_ (= disabled) |
| `osiris.deislabs.io/deployment` | Name of the deployment which is behind this service. This is required to map the service with its deployment. | _no value_ |
| `osiris.deislabs.io/enabled` | Enable this service's endpoints to be managed by the Osiris endpoints controller. Allowed values: `y`, `yes`, `true`, `on`, `1`. | _no value_ (= disabled) |
| `osiris.deislabs.io/deployment` | Name of the deployment which is behind this service. This is _required_ to map the service with its deployment. | _no value_ |
| `osiris.deislabs.io/loadBalancerHostname` | Map requests coming from a specific hostname to this service. Note that if you have multiple hostnames, you can set them with different annotations, using `osiris.deislabs.io/loadBalancerHostname-1`, `osiris.deislabs.io/loadBalancerHostname-2`, ... | _no value_ |
| `osiris.deislabs.io/ingressHostname` | Map requests coming from a specific hostname to this service. If you use an ingress in front of your service, this is required to create a link between the ingress and the service. Note that if you have multiple hostnames, you can set them with different annotations, using `osiris.deislabs.io/ingressHostname-1`, `osiris.deislabs.io/ingressHostname-2`, ... | _no value_ |
| `osiris.deislabs.io/ingressDefaultPort` | Custom service port when the request comes from an ingress. Default behaviour if there are more than 1 port on the service, is to look for a port named `http`, and fallback to the port `80`. Set this if you have multiple ports and using a non-standard port with a non-standard name. | _no value_ |
Expand Down
2 changes: 2 additions & 0 deletions example/hello-osiris.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ spec:
metadata:
labels:
app: hello-osiris
annotations:
osiris.deislabs.io/enabled: "true"
spec:
containers:
- name: hello-osiris
Expand Down
3 changes: 2 additions & 1 deletion pkg/kubernetes/osiris.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@ import (
)

const (
osirisEnabledAnnotationName = "osiris.deislabs.io/enabled"
metricsCheckIntervalAnnotationName = "osiris.deislabs.io/metricsCheckInterval"
)

// ResourceIsOsirisEnabled checks the annotations to see if the
// kube resource is enabled for osiris or not.
func ResourceIsOsirisEnabled(annotations map[string]string) bool {
enabled, ok := annotations["osiris.deislabs.io/enabled"]
enabled, ok := annotations[osirisEnabledAnnotationName]
if !ok {
return false
}
Expand Down
18 changes: 8 additions & 10 deletions pkg/kubernetes/osiris_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,50 +16,48 @@ func TestResourceIsOsirisEnabled(t *testing.T) {
{
name: "map with osiris enabled entry and value 1",
annotations: map[string]string{
"osiris.deislabs.io/enabled": "1",
osirisEnabledAnnotationName: "1",
},
expectedResult: true,
},
{
name: "map with osiris enabled entry and value true",
annotations: map[string]string{
"osiris.deislabs.io/enabled": "true",
osirisEnabledAnnotationName: "true",
},
expectedResult: true,
},
{
name: "map with osiris enabled entry and value on",
annotations: map[string]string{
"osiris.deislabs.io/enabled": "on",
osirisEnabledAnnotationName: "on",
},
expectedResult: true,
},
{
name: "map with osiris enabled entry and value y",
annotations: map[string]string{
"osiris.deislabs.io/enabled": "y",
osirisEnabledAnnotationName: "y",
},
expectedResult: true,
},
{
name: "map with osiris enabled entry and value yes",
annotations: map[string]string{
"osiris.deislabs.io/enabled": "yes",
osirisEnabledAnnotationName: "yes",
},
expectedResult: true,
},
{
name: "map with no osiris enabled entry ",
annotations: map[string]string{
"osiris.deislabs.io/notenabled": "yes",
},
name: "map with no osiris enabled entry",
annotations: map[string]string{},
expectedResult: false,
},

{
name: "map with osiris enabled entry and invalid value",
annotations: map[string]string{
"osiris.deislabs.io/enabled": "yee",
osirisEnabledAnnotationName: "yee",
},
expectedResult: false,
},
Expand Down
84 changes: 0 additions & 84 deletions pkg/metrics/proxy/injector/deployment_patch.go
View file
Open in desktop

This file was deleted.

5 changes: 1 addition & 4 deletions pkg/metrics/proxy/injector/injector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,7 @@ func (i *injector) Run(ctx context.Context) {
}()

glog.Infof(
"Proxy injector is listening on %s, patching Osiris-enabled "+
"deployments and pods",
"Proxy injector is listening on %s, patching Osiris-enabled pods",
i.srv.Addr,
)
err := i.srv.ListenAndServeTLS(i.config.TLSCertFile, i.config.TLSKeyFile)
Expand Down Expand Up @@ -113,8 +112,6 @@ func (i *injector) handleRequest(w http.ResponseWriter, r *http.Request) {
glog.Errorf("Can't decode body: %v", err)
} else {
switch ar.Request.Kind.Kind {
case "Deployment":
patchOps, err = i.getDeploymentPatchOperations(&ar)
case "Pod":
patchOps, err = i.getPodPatchOperations(&ar)
default:
Expand Down