Serve metrics under a dedicated port #48

linki · 2018-02-28T14:31:40Z

etcd v3.3 introduced a new flag to allow serving /metrics and /health under a different port than e.g. /v2/keys. This allows us to protect etcd's data via firewall rules but still let monitoring tools to access the monitoring information.

See feature request in etcd repo: etcd-io/etcd#8060.
The implementation landed in v3.3: etcd-io/etcd#8242

This PR instructs etcd to serve metrics and health under the additonal port 2381 unconditionally when the used etcd binary is >=v3.3.x. However, if not explicitely set in the senza.yaml this port won't be mapped to the outside and therefore isn't accessible. It doesn't expose more information than anything under 2379 already does. See our intended usage here: zalando-incubator/kubernetes-on-aws#879.

Note: ~~This fails when bundled with etcd lower than v3.3~~. Furthermore, serving this additional endpoint unconditionally should be safe but might not be desired. It keeps the implementation very simple, though. LMKWYT

/cc @CyberDem0n @aermakov-zalando @mikkeloscar

mikkeloscar · 2018-02-28T14:36:32Z

lgtm

CyberDem0n · 2018-02-28T15:37:27Z

etcd.py

@@ -253,6 +255,8 @@ def etcd_arguments(self, data_dir, initial_cluster, cluster_state):
            self.peer_url,
            '-listen-client-urls',
            'http://0.0.0.0:{}'.format(self.client_port),
+            '-listen-metrics-urls',


It breaks compatibility with old etcd, because the same script is used for different images containing different version.
Moreover, it even breaks upgrade 3.2 => 3.3.

Those arguments should be applied conditionally, i.e. only if etcd version is >= 3.3.
You can get version from environment: ETCDVERSION.

linki · 2018-03-01T11:21:48Z

@CyberDem0n Thanks.

It now looks at the ETCDVERSION env variable and only adds the -listen-metrics-urls flag when the version is greater than or equal to v3.3.x

aermakov-zalando · 2018-03-01T11:23:17Z

etcd.py

+        # this section handles etcd version specific flags
+        etcdversion = os.environ.get('ETCDVERSION')
+        if etcdversion:
+            etcdversion = list(map(lambda x: int(x), etcdversion.split('.')))


etcdversion = [int(x) for x in etcdversion.split('.')]

You could also use tuples which are immutable (tuple(int(x) for x in etcdversion.split('.'))), but don't forget to change the comparison to match.

Thanks, I changed it.

aermakov-zalando · 2018-03-01T11:24:19Z

etcd.py

+        if etcdversion:
+            etcdversion = list(map(lambda x: int(x), etcdversion.split('.')))
+            # don't try to add additonal flags if we encounter an unexpected version format
+            if len(etcdversion) == 3:


Not sure if this is worth it TBH. If they decide to do a 3.3.1.2, this will suddenly break instead of continuing. I'd just check that the length is >= 2, or even drop the check altogether.

aermakov-zalando · 2018-03-01T11:24:55Z

etcd.py

+            # don't try to add additonal flags if we encounter an unexpected version format
+            if len(etcdversion) == 3:
+                # etcd >= v3.3: serve metrics on an additonal port
+                if etcdversion[0] >= 3 and etcdversion[1] >= 3:


etcdversion >= [3, 3]

szuecs · 2018-03-05T11:58:33Z

@CyberDem0n can we get this merged?

szuecs · 2018-03-05T16:36:35Z

👍

CyberDem0n · 2018-03-06T07:42:33Z

etcd.py

@@ -263,6 +266,20 @@ def etcd_arguments(self, data_dir, initial_cluster, cluster_state):
            cluster_state
        ]

+        # this section handles etcd version specific flags
+        etcdversion = os.environ.get('ETCDVERSION')


There is still one corner case left, it breaks upgrade from 3.2 to 3.3 :(
During the upgrade first it starts the old binary in order to get data, than it stops it and starts the new binary.
Current change will try to apply -listen-metrics-urls to the old binary and fail if the old version is 3.2

In order to solve it, let's introduce one more argument to this method, run_old and pass its value from https://github.com/linki/stups-etcd-cluster/blob/0a914150be28b93ff77b56747b0cf09a010138dc/etcd.py#L474:

return self.me.etcd_arguments(self.DATA_DIR, peers, cluster_state, self.run_old)

And the line 270 has to be changed to:

etcdversion = os.environ.get('ETCDVERSION_PREV' if run_old else 'ETCDVERSION')

CyberDem0n · 2018-03-06T14:27:49Z

etcd.py

@@ -241,8 +243,9 @@ def delete_member(self, member):
        self.adjust_security_groups('revoke_ingress', member)
        return result

-    def etcd_arguments(self, data_dir, initial_cluster, cluster_state):
-        return [
+    def etcd_arguments(self, data_dir, initial_cluster, cluster_state, run_old)):


- def etcd_arguments(self, data_dir, initial_cluster, cluster_state, run_old)): + def etcd_arguments(self, data_dir, initial_cluster, cluster_state, run_old):

this is the reason why tests are failing

damn, sorry about that.

CyberDem0n · 2018-03-06T14:43:15Z

👍

linki · 2018-03-06T14:44:13Z

@CyberDem0n let me quickly test on our cluster that it still works as expected before merging.

linki · 2018-03-06T14:46:57Z

@CyberDem0n ok, works with 3.3.

CyberDem0n · 2018-03-06T14:49:01Z

I need a second 👍

linki · 2018-03-07T13:51:32Z

@CyberDem0n Did it work for you as well?

CyberDem0n · 2018-03-07T14:54:36Z

I didn't tried. In order to build image it needs to be merged and we need a second approval for that

mikkeloscar · 2018-03-07T14:56:45Z

👍

feat: serve metrics under a dedicated port

16f2411

linki mentioned this pull request Feb 28, 2018

Expose etcd metrics under a separate port zalando-incubator/kubernetes-on-aws#879

Merged

1 task

CyberDem0n reviewed Feb 28, 2018

View reviewed changes

fix: enable metrics flag only when etcd is v3.3+

7ab8bde

aermakov-zalando reviewed Mar 1, 2018

View reviewed changes

fix: cleanup detection of etcd version

0a91415

linki force-pushed the master branch from 252b986 to 0a91415 Compare March 1, 2018 13:00

CyberDem0n reviewed Mar 6, 2018

View reviewed changes

fix: ensure that correct flags are used when upgrading as well

d8e6c10

CyberDem0n reviewed Mar 6, 2018

View reviewed changes

fix: remove superflous closing parenthesis

974f733

CyberDem0n merged commit 6ec11bd into zalando-stups:master Mar 7, 2018

serathius mentioned this pull request Jun 27, 2018

Expose system component ports to the cluster kubernetes-sigs/cluster-api#358

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serve metrics under a dedicated port #48

Serve metrics under a dedicated port #48

linki commented Feb 28, 2018 •

edited

Loading

mikkeloscar commented Feb 28, 2018

CyberDem0n Feb 28, 2018

linki commented Mar 1, 2018

aermakov-zalando Mar 1, 2018 •

edited

Loading

linki Mar 1, 2018

aermakov-zalando Mar 1, 2018 •

edited

Loading

aermakov-zalando Mar 1, 2018

szuecs commented Mar 5, 2018

szuecs commented Mar 5, 2018

CyberDem0n Mar 6, 2018

CyberDem0n Mar 6, 2018

linki Mar 6, 2018

CyberDem0n commented Mar 6, 2018

linki commented Mar 6, 2018 •

edited

Loading

linki commented Mar 6, 2018

CyberDem0n commented Mar 6, 2018

linki commented Mar 7, 2018

CyberDem0n commented Mar 7, 2018

mikkeloscar commented Mar 7, 2018

Serve metrics under a dedicated port #48

Serve metrics under a dedicated port #48

Conversation

linki commented Feb 28, 2018 • edited Loading

mikkeloscar commented Feb 28, 2018

CyberDem0n Feb 28, 2018

Choose a reason for hiding this comment

linki commented Mar 1, 2018

aermakov-zalando Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

linki Mar 1, 2018

Choose a reason for hiding this comment

aermakov-zalando Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

aermakov-zalando Mar 1, 2018

Choose a reason for hiding this comment

szuecs commented Mar 5, 2018

szuecs commented Mar 5, 2018

CyberDem0n Mar 6, 2018

Choose a reason for hiding this comment

CyberDem0n Mar 6, 2018

Choose a reason for hiding this comment

linki Mar 6, 2018

Choose a reason for hiding this comment

CyberDem0n commented Mar 6, 2018

linki commented Mar 6, 2018 • edited Loading

linki commented Mar 6, 2018

CyberDem0n commented Mar 6, 2018

linki commented Mar 7, 2018

CyberDem0n commented Mar 7, 2018

mikkeloscar commented Mar 7, 2018

linki commented Feb 28, 2018 •

edited

Loading

aermakov-zalando Mar 1, 2018 •

edited

Loading

aermakov-zalando Mar 1, 2018 •

edited

Loading

linki commented Mar 6, 2018 •

edited

Loading