search

allegro / marathon-consul

Integrates Marathon apps with Consul service discovery.
Apache License 2.0
191 stars 33 forks source link

Fixes #164 | Check if port index fits in bounds #166

Closed janisz closed 7 years ago

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 25f46ce585c7bfe322a27eb39091d2185ad600a5 on bugfix/#164/check_ports_bounds into on master.

janisz commented 7 years ago

This happens when you deployed service and then update it with different number of ports but you stopped deployment or you deployment has not finished yet.

Below are the steps to reproduce it:

* `PUT /v2/apps` ```json [{ "id": "bug/164", "cmd": "python -m SimpleHTTPServer $PORT0", "cpus": 0.1, "mem": 32, "disk": 0, "instances": 1, "healthChecks": [ { "gracePeriodSeconds": 3, "intervalSeconds": 60, "timeoutSeconds": 20, "maxConsecutiveFailures": 3, "portIndex": 0, "path": "/", "protocol": "HTTP", "ignoreHttp1xx": false } ], "labels": { "consul": "name" }, "portDefinitions": [ { "protocol": "tcp" } ] }] ``` * `PUT /v2/apps` ```json [{ "id": "bug/164", "cmd": "fail", "cpus": 0.1, "mem": 32, "disk": 0, "instances": 1, "healthChecks": [ { "gracePeriodSeconds": 3, "intervalSeconds": 60, "timeoutSeconds": 20, "maxConsecutiveFailures": 3, "portIndex": 0, "path": "/", "protocol": "HTTP", "ignoreHttp1xx": false } ], "labels": { "consul": "name" }, "portDefinitions": [ { "protocol": "tcp" }, { "protocol": "tcp" } ] }] ``` * `GET /v2/apps/bug/164?embed=app.tasks` ```json { "app":{ "id":"/bug/164", "ports":[ 10005, 10006 ], "portDefinitions":[ { "port":10005, "protocol":"tcp" }, { "port":10006, "protocol":"tcp" } ], "tasks":[ { "state":"TASK_RUNNING", "ports":[ 31018 ], "id":"bug_164.adaf2c2c-e88d-11e6-ae5c-a44e31765720", "appId":"/bug/164" } ] } } ```
ojagodzinski commented 7 years ago

Can service be processed anyway, despite the inconsistency between app definition and running task? Totally 'ignoring' such cases may not be the best idea.

janisz commented 7 years ago

I concur. If port counts differs between task and app we can check version and if it differs to we can skip registration of whole task. This approach could work but does not prevent situation when port definitions totally changed but port counts stays the same.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.01%) to 90.903% when pulling 6647e8e005632a0c1c4649f313f3946195e71f59 on bugfix/#164/check_ports_bounds into 73e66724bbeabd537b90490fd3d7aae5e363416d on master.