Add IS constraint operator #5587

timcharper · 2017-10-09T23:17:13Z

JIRA Issues: MARATHON_EE-1667

mesosphere-ci

I'm building your change at jenkins-marathon-pipelines-PR-5587-1.

mesosphere-ci

✔ Build of #5587 completed successfully.

See details at jenkins-marathon-pipelines-PR-5587-1.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

"url": "https://s3.amazonaws.com/downloads.mesosphere.io/marathon/snapshots/marathon-1.6.0-pre-31-g4302502.tgz",
"sha1": "696ff561009affd7ddeda9299780d476fa243196"

＼\ ٩( ᐛ )و /／

ichernetsky · 2017-10-10T00:45:36Z

src/test/scala/mesosphere/mesos/ConstraintsTest.scala

+
+  "IN operator" should {
+    "require that the offer value be in the comma delimited list of values" in {
+      makeOffer("host1") should meetConstraint(hostnameField, IN, "host1,host2")


I am worried that values might have commas in them. In this case, we would need to support escaping and/or strings inside value strings of constraint definitions. Although I would vote for supports of arrays of strings as constraint values. In this case the constraint in an app definition could look like ["some-field", "IN", ["value1", "value2"]. What do you think?

I like what Ivan is proposing! :)

I thought of this; This would break API compatibility. If you put a comma in your value then you are awful. We can escape the comma for v2 API api and then allow the array syntax for v3.

Please forgive me my ignorance, but I do not see how it would break the user-facing API compatibility. Isn't it about addition/extension only? Are you referring to the case when Java/Python/etc clients would fail if their receive an array value, and not a string one?

It would change the type, specified here:

https://github.com/mesosphere/marathon/blob/v1.6.0-pre/docs/docs/rest-api/public/api/v2/types/constraint.raml#L20-L29

The specification is that constraints are a 2-3 item array of strings. Putting a non-string in there breaks the API.

I was thinking of supporting strings and arrays: arrays for IN and strings for everything else.

I understood this :)

@ichernetsky did you mean to write this earlier?

["some-field", "IN", "value1", "value2"]

(sans the additional [)?

@jdef mentioned this as a possibility and it made me wonder if perhaps this was what you were trying to suggest :)

No, I was suggesting ["field", "IN", ["value1", "value2"]].

alenkacz

Just a general question - who and when updates the docs? Like this one https://mesosphere.github.io/marathon/docs/constraints.html

timcharper · 2017-10-10T20:45:12Z

Great question :) On the one hand, it'd probably be a good idea to update the docs with this PR. Suzanne usually does our docs.

On the other hand, no sense in writing docs until we fully settle on the behavior of these constraints.

alenkacz · 2017-10-10T20:46:57Z

@timcharper ok, thanks :)

then LGTM

mesosphere-ci

I'm building your change at jenkins-marathon-pipelines-PR-5587-2.

mesosphere-ci

✔ Build of #5587 completed successfully.

See details at jenkins-marathon-pipelines-PR-5587-2.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

"url": "https://s3.amazonaws.com/downloads.mesosphere.io/marathon/snapshots/marathon-1.6.0-pre-38-geed526c.tgz",
"sha1": "a140ce48e9e79c0eea51eab17a3d56ddb335ed3f"

＼\ ٩( ᐛ )و /／

meln1k

LGTM

jeschkies · 2017-10-11T14:02:59Z

src/main/proto/marathon.proto

@@ -26,6 +26,10 @@ message Constraint {
      UNLIKE = 4;
      // Field will be grouped by field. Value specifies the maximum size of each group.
      MAX_PER = 5;
+      // Field must be the specified value
+      IS = 6;


Our API is also defined in Protos?!

jeschkies · 2017-10-11T14:05:37Z

src/test/scala/mesosphere/mesos/ConstraintsTest.scala

+
+    "trims whitespace after the commas (but not before)" in {
+      makeOffer("host1") should meetConstraint(hostnameField, IN, "host1, host2")
+      makeOffer("host1") should meetConstraint(hostnameField, IN, "host1 , host2")


Isn't here a whitespace before the comma? host1 is not equal to host1[ ]. What am I missing?

The separator is a comma optionally surrounded by whitespaces.

err lol the test description is a lie 😢

I don't believe Mesos constraints what values can be specified on an agent attribute value. A user could potentially put trailing or leading spaces in the value. That would be an awful thing to do, but, none-the-less, possible.

I think it is more likely that people accidentally put a space in comma-delimited list.

Yes, in agreement here.

@vinodkone corrected me; spaces are explicitly not allowed in attribute values, and they are removed by Mesos.

timcharper · 2017-10-14T00:10:14Z

Waiting for this proposal to be accepted by DCOS SDK / Storage Services team:

https://docs.google.com/document/d/e/2PACX-1vSFvPol0pcHC2Web7EaNU0oSDS5wrOWSgFcmuslYBtISV2NB2JZ_D-B4wpWy_Vutaf08m2LX6WZVy6s/pub

JIRA Issues: MARATHON_EE-1667

- Reject set/range values from IS constraints - Accept only set values for IN constraints - Reduce test verbosity

timcharper · 2017-10-31T06:07:46Z

Proposal is accepted and change has been updated to match proposal

mesosphere-ci

I'm building your change at jenkins-marathon-pipelines-PR-5587-3.

mesosphere-ci

✔ Build of #5587 completed successfully.

See details at jenkins-marathon-pipelines-PR-5587-3.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

"url": "https://s3.amazonaws.com/downloads.mesosphere.io/marathon/snapshots/marathon-1.6.0-pre-115-g254168f.tgz",
"sha1": "4c2d677483212af6cfe5bce9897afc255f58900c"

You can run system integration test changes of this PR against Marathon
master by tirggering this Jenkins job with the Pull_Request_id 5587.
The job then reports back to this PR.

＼\ ٩( ᐛ )و /／

Documentation

mesosphere-ci

I'm building your change at jenkins-marathon-pipelines-PR-5587-4.

mesosphere-ci

I'm building your change at jenkins-marathon-pipelines-PR-5587-5.

mesosphere-ci

I'm building your change at jenkins-marathon-pipelines-PR-5587-6.

mesosphere-ci

✔ Build of #5587 completed successfully.

See details at jenkins-marathon-pipelines-PR-5587-4.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

"url": "https://s3.amazonaws.com/downloads.mesosphere.io/marathon/snapshots/marathon-1.6.0-pre-116-gcd129c2.tgz",
"sha1": "1881d93f62c1fe8dbcd43dc4a34b42f604b1f327"

You can run system integration test changes of this PR against Marathon
master by tirggering this Jenkins job with the Pull_Request_id 5587.
The job then reports back to this PR.

＼\ ٩( ᐛ )و /／

mesosphere-ci

✔ Build of #5587 completed successfully.

See details at jenkins-marathon-pipelines-PR-5587-5.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

"url": "https://s3.amazonaws.com/downloads.mesosphere.io/marathon/snapshots/marathon-1.6.0-pre-117-g85dd4af.tgz",
"sha1": "257cd31f926d0956d9da7a8e4db1e180d1820508"

You can run system integration test changes of this PR against Marathon
master by tirggering this Jenkins job with the Pull_Request_id 5587.
The job then reports back to this PR.

＼\ ٩( ᐛ )و /／

mesosphere-ci

✔ Build of #5587 completed successfully.

See details at jenkins-marathon-pipelines-PR-5587-6.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

"url": "https://s3.amazonaws.com/downloads.mesosphere.io/marathon/snapshots/marathon-1.6.0-pre-118-gc35fe74.tgz",
"sha1": "3b39ab10929cdf100ea909907236e495dd5b62b7"

You can run system integration test changes of this PR against Marathon
master by tirggering this Jenkins job with the Pull_Request_id 5587.
The job then reports back to this PR.

＼\ ٩( ᐛ )و /／

mesosphere-ci

I'm building your change at jenkins-marathon-pipelines-PR-5587-7.

mesosphere-ci

I'm building your change at jenkins-marathon-pipelines-PR-5587-8.

mesosphere-ci

✔ Build of #5587 completed successfully.

See details at jenkins-marathon-pipelines-PR-5587-7.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

"url": "https://s3.amazonaws.com/downloads.mesosphere.io/marathon/snapshots/marathon-1.6.0-pre-119-gf1ca7e7.tgz",
"sha1": "a399477fc29e3993e5ceebaa71dfcf8da1fc0db8"

You can run system integration test changes of this PR against Marathon
master by tirggering this Jenkins job with the Pull_Request_id 5587.
The job then reports back to this PR.

＼\ ٩( ᐛ )و /／

mesosphere-ci

✔ Build of #5587 completed successfully.

See details at jenkins-marathon-pipelines-PR-5587-8.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

"url": "https://s3.amazonaws.com/downloads.mesosphere.io/marathon/snapshots/marathon-1.6.0-pre-120-g86f7168.tgz",
"sha1": "1772832233f6db5a56586b196701a57e43e9ce58"

You can run system integration test changes of this PR against Marathon
master by tirggering this Jenkins job with the Pull_Request_id 5587.
The job then reports back to this PR.

＼\ ٩( ᐛ )و /／

mesosphere-ci

I'm building your change at jenkins-marathon-pipelines-PR-5587-9.

mesosphere-ci

✔ Build of #5587 completed successfully.

See details at jenkins-marathon-pipelines-PR-5587-9.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

"url": "https://s3.amazonaws.com/downloads.mesosphere.io/marathon/snapshots/marathon-1.6.0-pre-123-g7fc6036.tgz",
"sha1": "f65fd33a3ab33e8f6a9a4ac9fc450ad75f6b560d"

You can run system integration test changes of this PR against Marathon
master by tirggering this Jenkins job with the Pull_Request_id 5587.
The job then reports back to this PR.

＼\ ٩( ᐛ )و /／

mesosphere-ci

I'm building your change at jenkins-marathon-pipelines-PR-5587-10.

mesosphere-ci

✔ Build of #5587 completed successfully.

See details at jenkins-marathon-pipelines-PR-5587-10.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

"url": "https://s3.amazonaws.com/downloads.mesosphere.io/marathon/snapshots/marathon-1.6.0-pre-125-g193bf8b.tgz",
"sha1": "457e50923cb2c4fdc7e7c72c473495776c5d404a"

You can run system integration test changes of this PR against Marathon
master by tirggering this Jenkins job with the Pull_Request_id 5587.
The job then reports back to this PR.

＼\ ٩( ᐛ )و /／

sascala

a couple tiny comments. Otherwise, LGTM!

sascala · 2017-11-01T22:02:41Z

docs/docs/constraints.md

@@ -102,6 +102,8 @@ $ curl -X POST -H "Content-type: application/json" localhost:8080/v2/apps -d '{
  }'
 ```

+Note, if the attribute in question is a scalar, it is rounded to the nearest thousandth using the half-even rounding strategy; zeroes after the decimal are dropped.


**Note:** If...

sascala · 2017-11-01T22:03:24Z

docs/docs/constraints.md

+
+#### Comparing scalar values
+
+When comparing scalars, the value is compared to the nearest thousandth (using half-even rounding strategy)


^Period at the end of the sentence.

timcharper · 2017-11-01T22:10:12Z

Thanks! Merged

timcharper requested review from ichernetsky and alenkacz October 9, 2017 23:17

mesosphere-ci suggested changes Oct 9, 2017

View reviewed changes

mesosphere-ci approved these changes Oct 9, 2017

View reviewed changes

ichernetsky reviewed Oct 10, 2017

View reviewed changes

alenkacz reviewed Oct 10, 2017

View reviewed changes

alenkacz approved these changes Oct 10, 2017

View reviewed changes

mesosphere-ci suggested changes Oct 11, 2017

View reviewed changes

mesosphere-ci approved these changes Oct 11, 2017

View reviewed changes

meln1k approved these changes Oct 11, 2017

View reviewed changes

jeschkies reviewed Oct 11, 2017

View reviewed changes

timcharper changed the title ~~Add IN and IS constraint operator~~ Add IN and IS constraint operator [DO NOT MERGE] Oct 12, 2017

timcharper changed the title ~~Add IN and IS constraint operator [DO NOT MERGE]~~ Add IN and IS constraint operator [DO NOT MERGE YET] Oct 12, 2017

wavesoft added the hold merge label Oct 13, 2017

meichstedt removed the hold merge label Oct 17, 2017

ichernetsky approved these changes Oct 18, 2017

View reviewed changes

Tim Harper added 3 commits October 30, 2017 00:12

Add IN and IS constraint operator

9c56a04

JIRA Issues: MARATHON_EE-1667

Less lies

d102815

Update IN and IS constraints to agreed upon spec

c84c556

- Reject set/range values from IS constraints - Accept only set values for IN constraints - Reduce test verbosity

timcharper force-pushed the tharper/add-in-and-is-constraint branch from 4302502 to c84c556 Compare October 31, 2017 06:06

timcharper changed the title ~~Add IN and IS constraint operator [DO NOT MERGE YET]~~ Add IN and IS constraint operator Oct 31, 2017

timcharper changed the title ~~Add IN and IS constraint operator~~ Add IN and IS constraint operators Oct 31, 2017

mesosphere-ci suggested changes Oct 31, 2017

View reviewed changes

mesosphere-ci approved these changes Oct 31, 2017

View reviewed changes

Match scalars by value, not by format (within a specified precision)

aad2ecd

Documentation

mesosphere-ci suggested changes Oct 31, 2017

View reviewed changes

lets do @hostname in another commit

53a8e13

mesosphere-ci suggested changes Oct 31, 2017

View reviewed changes

note on validation

53b9b7c

mesosphere-ci suggested changes Oct 31, 2017

View reviewed changes

mesosphere-ci approved these changes Oct 31, 2017

View reviewed changes

merge makeInstanceWithHost and makeInstance methods

0368b41

mesosphere-ci suggested changes Oct 31, 2017

View reviewed changes

fix test descriptions

552b53b

mesosphere-ci suggested changes Oct 31, 2017

View reviewed changes

mesosphere-ci approved these changes Oct 31, 2017

View reviewed changes

remove IN operator

e178e78

mesosphere-ci suggested changes Nov 1, 2017

View reviewed changes

ichernetsky approved these changes Nov 1, 2017

View reviewed changes

mesosphere-ci approved these changes Nov 1, 2017

View reviewed changes

timcharper changed the title ~~Add IN and IS constraint operators~~ Add IS constraint operators Nov 1, 2017

timcharper changed the title ~~Add IS constraint operators~~ Add IS constraint operator Nov 1, 2017

clarification on rounding

0b43829

mesosphere-ci suggested changes Nov 1, 2017

View reviewed changes

mesosphere-ci approved these changes Nov 1, 2017

View reviewed changes

sascala approved these changes Nov 1, 2017

View reviewed changes

timcharper closed this Nov 1, 2017

timcharper deleted the tharper/add-in-and-is-constraint branch November 27, 2017 22:10


		#### Comparing scalar values

		When comparing scalars, the value is compared to the nearest thousandth (using half-even rounding strategy)

Add IS constraint operator #5587

Add IS constraint operator #5587

Conversation

timcharper commented Oct 9, 2017

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timcharper Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenkacz left a comment

Choose a reason for hiding this comment

timcharper commented Oct 10, 2017

alenkacz commented Oct 10, 2017

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

meln1k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timcharper commented Oct 14, 2017

timcharper commented Oct 31, 2017

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

mesosphere-ci left a comment

Choose a reason for hiding this comment

sascala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timcharper commented Nov 1, 2017

timcharper Oct 10, 2017 •

edited

Loading