Initial support for chronos 3.x for #33 by thekad · Pull Request #35 · asher/chronos-python

thekad · 2017-03-07T17:59:08Z

This is the initial draft I wrote for support of chronos >= 3.x
It tries to play nice with versions < 3.x as well and it doesn't keep you from
shooting yourself in the foot. I am undecided at which point you draw the line
in the sand and only allow valid values sent.
Tested vs chronos 2.4 and 3.0.x

This is the initial draft I wrote for support of chronos >= 3.x It tries to play nice with versions < 3.x as well and it doesn't keep you from shooting yourself in the foot. I am undecided at which point you draw the line in the sand and only allow valid values sent. Tested vs chronos 2.4 and 3.0.x

thekad · 2017-03-07T18:02:30Z

I had to skirt around the testing environment because it is not so easy to change, however I came up with the following docker-compose.yml:

version: "2"

services:
  zookeeper:
    image: zookeeper
    container_name: zookeeper
    environment:
      ZOO_MY_ID: 1
      ZK_INIT_LIMIT: 10
      ZK_SYNC_LIMIT: 5

  master:
    image: mesosphere/mesos-master:1.0.3
    container_name: mesos-master
    environment:
      MESOS_ZK: zk://zookeeper:2181/mesos
      MESOS_QUORUM: 1
      MESOS_CLUSTER: chronos-python
      MESOS_REGISTRY: in_memory
    ports:
      - 5050:5050
    depends_on:
      - zookeeper
    links:
      - zookeeper

  slave:
    image: mesosphere/mesos-slave:1.0.3
    container_name: mesos-slave
    pid: host
    environment:
      MESOS_MASTER: zk://zookeeper:2181/mesos
      MESOS_WORK_DIR: /tmp/mesos
      MESOS_PORT: 5051
      MESOS_RESOURCES: ports(*):[11000-11999]
      MESOS_CONTAINERIZERS: mesos
    ports:
      - 5051:5051
    volumes:
      - /sys/fs/cgroup:/sys/fs/cgroup
    depends_on:
      - zookeeper
    links:
      - zookeeper
      - master

  chronos2:
    image: mesosphere/chronos:chronos-2.4.0-0.1.20150828104228.ubuntu1404-mesos-0.27.0-0.2.190.ubuntu1404
    container_name: chronos2
    command: '/usr/bin/chronos run_jar --http_port 4400 --zk_hosts zookeeper:2181 --master zk://zookeeper:2181/mesos'
    ports:
      - 4402:4400
    depends_on:
      - zookeeper
      - master
    links:
      - zookeeper
      - master
      - slave

  chronos3:
    image: mesosphere/chronos:v3.0.2
    container_name: chronos3
    command: '--zk_hosts zookeeper:2181 --master zk://zookeeper:2181/mesos'
    environment:
      PORT0: 4400
      PORT1: 8080
    ports:
      - 4403:4400
    depends_on:
      - zookeeper
      - master
    links:
      - zookeeper
      - master
      - slave

Which allows you to spin up a chronos 2.4 on port 4402 and a chronos 3.0.2 on port 4403, then I could modify itests/steps/chronos_steps.py to point individually to either one. One of the test cases fails in 3.x (obviously) because new chronos doesn't accept spaces in the job name

thekad · 2017-03-07T18:39:45Z

And of course travis build failed because the test env is not updated. I didn't want to mess with it but my idea would be to default to test everything in docker under travis via docker-compose https://docs.travis-ci.com/user/docker/ and run the same tests vs either port, depending which version of chronos you want to test (the docker-compose port bits would need updating as well) what do you think @Rob-Johnson ?

solarkennedy · 2017-03-08T03:52:07Z

How about just append the env to include a new CHRONOSVERSION and travis will matrix it out? Why not mess with it? This to me seems like the most direct way to test this, and only requires one line added to the .travis.yaml file.

You could do docker compose, we use it in https://github.com/thefactory/marathon-python. I recommend avoiding the complexity if you can.

thekad · 2017-03-08T05:54:29Z

The whole test setup right now relies on debian packages being available so chronos can be installed. Mesosphere stopped building packages in v3.0 and only provide docker images, so that test infrastructure is a no-go from now on. However they have been providing docker images for quite some time, meaning we can just move the whole thing over to use docker images. Not saying it is the best way to go, but seems like options are limited (unless we wanna start building chronos debian packages ourselves)

…

-- ~kad

On Tue, Mar 7, 2017 at 9:52 PM, Kyle Anderson ***@***.***> wrote: How about just append the env to include a new CHRONOSVERSION and travis will matrix it out? Why *not* mess with it? This to me seems like the most direct way to test this, and only requires one line added to the .travis.yaml file. You could do docker compose, we use it in https://github.com/thefactory/ marathon-python. I recommend avoiding the complexity if you can. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#35 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC1Vl-9bbfw8RKvtLCqnL3uAXU-Cu4vks5rjiXngaJpZM4MV0ki> .

solarkennedy · 2017-03-11T05:09:06Z

Oh right, no debs from upstream. We'll docker it is I guess. Again the marathon-python does have some "ok" prior art.

Which is pretty fantastic

Now it is a matter of passing the appropriate docker node defined in the docker-compose.yml file. All chronos nodes will expose chronos on port 4400 so all client calls will be done to localhost port 4400. No need to figure out magically which port docker-compose is exposing or anything (at least in theory)

Instead of on various files

Taken straight from https://docs.travis-ci.com/user/docker/

Rob-Johnson · 2017-03-16T11:13:38Z

+        else:
+            if scheduler_version not in SCHEDULER_VERSIONS:
+                raise ChronosAPIError('Wrong scheduler_version provided')
+            self._prefix = "/%s" % (scheduler_version,)


I think you need to add empty-string '' to list list of acceptable SCHEDULER_VERSIONS, or allow None so people can keep using < 3

Rob-Johnson · 2017-03-16T11:24:24Z

                raise MissingFieldError("missing required field %s" % k)

+        # Chronos v3.x rejects job names with spaces in them
+        regex = re.compile(r'^[\w.-]+$')


what about < 3 though? this isn't really allowing for backwards compatibility - if I were you I'd let this happen and have clients deal with the failure (hopefully chronos spits out a reasonably informative error message?)

Rob-Johnson · 2017-03-16T11:25:16Z


 class ChronosJob(object):
    fields = [
-        "async",


same comment re < 3. You're not really allowing for backwards compat here.

This reverts commit 538d9bb.

This way we'll properly raise an exception when the API returns a non OK status code.

This way we can test on multiple versions again

as per https://docs.travis-ci.com/user/multi-os/

This way we can support more than 1 version

thekad · 2017-03-22T17:29:56Z

@Rob-Johnson So I worked out most of the compatibility issues, but I seem to have tripped the py.test tests with da54db4, I am raising an ChronosAPIError when the chronos api, well, returns an error. However:

tests/test_chronos_init.py::test_check_returns_raw_response_when_not_json FAILED
tests/test_chronos_init.py::test_check_does_not_log_error_when_content_type_is_not_json FAILED
tests/test_chronos_init.py::test_check_logs_error_when_invalid_json FAILED

These 3 tests make me think not raising an error on chronos api errors was actually intentional, do you still think that's a valid route and I should revert the exception raising? or should the tests be reworked?

Rob-Johnson · 2017-03-23T11:08:23Z

those tests are more for handling Chronos not consistently returning JSON in failure modes. Happy for you to rework them as you see fit, so long as we keep similar behaviour: that is, the client handles non-json responses appropriately.

Also the payload message might be too long as is

And also since we're throwing exceptions, make sure to catch them properly

thekad · 2017-03-23T21:41:18Z

@Rob-Johnson let me know what you think re: 1ba92a9 also I fixed the build and it now passes with 2.4.0 and 3.0.2 with both python 2.7 and 3.5.

Rob-Johnson

one last thing, but otherwise this looks good to me. want to take a glance over it @solarkennedy? one action point for us at Yelp is going to be adding api_version=None to our calls to create a Chronos client until we get our fork inline with upstream.

Rob-Johnson · 2017-03-24T15:24:03Z

 except ImportError:
    from urllib.parse import quote

+SCHEDULER_VERSIONS = ('v1',)


can we rename this (and all mentions of scheduler_version) to be API_VERSION instead? it just makes it a little clearer what this actually does.

@Rob-Johnson I thought about this, but since chronos appears to have multiple APIs and they are only versioning /scheduler/* (they don't prefix /metrics, for example) that's why I decided to use this variable. Happy to change it to whatever name makes the most sense, naming is hard 😊

how about.... scheduler_api_version?

😲 see this is exactly why you get paid the big bucks @ yelp, gonna do that.

Rob-Johnson · 2017-03-28T14:20:13Z

thanks @thekad !

Jorge Gallegos added 10 commits March 14, 2017 18:32

Make warnings throw an exception instead

a68659e

Line breaking < 120c and default to v1

ccc48a9

Chronos API removed PUT method here

538d9bb

Which is pretty fantastic

async is no longer a valid attribute

5adc62d

Should account for whatever prefix client has

1a3bc93

Put all the test requirements in a single file

c8a337c

Instead of on various files

Using the docker service provided by travis

2a4a3e7

Taken straight from https://docs.travis-ci.com/user/docker/

Update to chronos 3.0.2

de0188e

Update the docker images before building if needed

97e90d4

Rob-Johnson reviewed Mar 16, 2017

View reviewed changes

Jorge Gallegos added 7 commits March 22, 2017 09:59

Trying to bring back legacy support

1f91834

Revert "Chronos API removed PUT method here"

5a37d15

This reverts commit 538d9bb.

Working legacy support back in

c31e8ae

We need to raise a proper exception on http errors

da54db4

This way we'll properly raise an exception when the API returns a non OK status code.

Work in environment variables to control behavior

4795d0b

This way we can test on multiple versions again

Use travis' matrix support

103fb95

as per https://docs.travis-ci.com/user/multi-os/

Use behave's tag support to only run some tests

496aac7

This way we can support more than 1 version

Jorge Gallegos added 4 commits March 23, 2017 13:40

3xx are potentially good status codes

65cf3ac

Also the payload message might be too long as is

Adding a test for all known status codes

1ba92a9

And also since we're throwing exceptions, make sure to catch them properly

Small update to docs

34ee57f

I think this is causing travis failures

c378464

Jorge Gallegos added 3 commits March 23, 2017 15:04

For flake8's sake

3abaa15

Fixing typo in {env} and passing CHRONOSVERSION

f0c3405

Using the environment is kind of feeble I think

787ddd8

Rob-Johnson requested changes Mar 24, 2017

View reviewed changes

s/scheduler_version/scheduler_api_version/

2d598b2

Rob-Johnson approved these changes Mar 28, 2017

View reviewed changes

Rob-Johnson merged commit e531f47 into asher:master Mar 28, 2017

paddycarey mentioned this pull request Apr 20, 2017

Chronos 3.0.0 support #33

Closed

Conversation

thekad commented Mar 7, 2017

Uh oh!

thekad commented Mar 7, 2017

Uh oh!

thekad commented Mar 7, 2017

Uh oh!

solarkennedy commented Mar 8, 2017

Uh oh!

thekad commented Mar 8, 2017 via email

Uh oh!

solarkennedy commented Mar 11, 2017

Uh oh!

Rob-Johnson Mar 16, 2017

Choose a reason for hiding this comment

Uh oh!

Rob-Johnson Mar 16, 2017

Choose a reason for hiding this comment

Uh oh!

Rob-Johnson Mar 16, 2017

Choose a reason for hiding this comment

Uh oh!

thekad commented Mar 22, 2017

Uh oh!

Rob-Johnson commented Mar 23, 2017

Uh oh!

thekad commented Mar 23, 2017

Uh oh!

Rob-Johnson left a comment

Choose a reason for hiding this comment

Uh oh!

Rob-Johnson Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thekad Mar 24, 2017

Choose a reason for hiding this comment

Uh oh!

Rob-Johnson Mar 24, 2017

Choose a reason for hiding this comment

Uh oh!

thekad Mar 24, 2017

Choose a reason for hiding this comment

Uh oh!

Rob-Johnson commented Mar 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Rob-Johnson Mar 24, 2017 •

edited

Loading