HomeMesosphereNo notifications. 4 unresolved issues.

Changed tty configuration from (rows,cols) to (true|false)
ClosedAll Users

Authored by unterstein on Jun 23 2017, 12:41 PM.

Details

Summary

TTY functionality was originally implemented as configuration of a tuple (cols,rows), but mesos does not use this configuration.
Mesos is using this configuration internally when tty is first launched.

Test Plan

sbt test

Diff Detail

Repository
rMARATHON marathon
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
unterstein created this revision.Jun 23 2017, 12:41 PM
ichernetsky accepted this revision.Jun 23 2017, 12:43 PM
This revision is now accepted and ready to land.Jun 23 2017, 12:43 PM
jenkins accepted this revision.Jun 23 2017, 12:57 PM
✔ Build of 3616 completed jenkins-public-marathon-phabricator-345.

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://downloads.mesosphere.io/marathon/snapshots/marathon-1.5.0-SNAPSHOT-611-g99d1539.tgz",
"sha1": "4d953aea9dde051d0c2b47b35d62fcaaf63f7a49"

\\ ٩( ᐛ )و //

klueska added inline comments.
docs/docs/rest-api/public/api/v2/types/app.raml
315

Should be 'allocated' not 'enabled'

docs/docs/rest-api/public/api/v2/types/podContainer.raml
120

Should be 'allocated' not 'enabled'

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
415

I don't know scala ,but does making a variable an 'Option[Boolean]' mean that it can take on 3 values? None, true, false?

So long as "None" maps to false for the default value, I think this is fine. I'm just curious why we need it to be of type 'Option' in this case.

src/test/scala/mesosphere/marathon/state/AppDefinitionTest.scala
311

A better command to run would be:

if [ -t 0 ] ; then echo "true"; else echo "false"; fi

This will print "true" if a tty has been allocated, and "false" otherwise.

timcharper accepted this revision.Jun 23 2017, 5:40 PM

I left some comments; please review. Looks fundamentally good to me.

Question: will we ever specify window size for mesos in the future?

src/main/scala/mesosphere/marathon/core/pod/MesosContainer.scala
22

Does this need to be an Option[Boolean]? Any reason to not just assign a default?

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
415

None allows it to be unspecified; I'm not sure if there is a solid reason to be unspecified, though. It seems like we can just default it to false and always specify it.

We have a data structure patching layer which should use an option here to indicate that the prior value is to be used (unpatched). Johannes, what are your thoughts?

unterstein added inline comments.Jun 23 2017, 10:25 PM
src/main/scala/mesosphere/marathon/core/pod/MesosContainer.scala
22

hmmm...yes, in the internal model we can go to false. I tried to go for simple boolean in the first place, but then I was punished by our tests that I changed the responding JSON, so I decided to use Option[Boolean] in all places. But yeah, I will change this to simple boolean again 👍

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
415

We need to go for Option[Boolean] in RAML layer, because we need to be able to render JSONs without tty field, if no tty was configured from the user. Therefore I went for Option[Boolean] = None for the default option, which is mostly used during serialization to or from raml.

src/test/scala/mesosphere/marathon/state/AppDefinitionTest.scala
311

This unit test will never deploy the application. We test only the serialization round trip here ;)

We should add a system integration test to do this! @klueska is tty option in current dc/os testing/master available so that I can add a system integration test for this functionality?

klueska added inline comments.Jun 23 2017, 10:29 PM
src/test/scala/mesosphere/marathon/state/AppDefinitionTest.scala
311

Yes. Support for this has been in Mesos since DC/OS 1.9, so it should just work (fingers crossed).

jeschkies added inline comments.Jun 26 2017, 12:01 PM
docs/docs/rest-api/public/api/v2/types/app.raml
313–315

Is this a breaking change to the API?

unterstein added inline comments.Jun 26 2017, 2:56 PM
docs/docs/rest-api/public/api/v2/types/app.raml
313–315

This was introduced within 1.5 and was not part of any RC yet, therefore I would not consider this as breaking change. What do you think?

unterstein updated this revision to Diff 3627.Jun 26 2017, 7:33 PM
unterstein marked 3 inline comments as done.
  • adapted feedback
jenkins accepted this revision.Jun 26 2017, 7:55 PM
✔ Build of 3627 completed jenkins-public-marathon-phabricator-355.

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://downloads.mesosphere.io/marathon/snapshots/marathon-1.5.0-SNAPSHOT-611-g99d1539.tgz",
"sha1": "f1df14704919da123d6a92939fbf6b2dfd6c7086"

\\ ٩( ᐛ )و //

This revision was automatically updated to reflect the committed changes.