HomeMesosphereNo notifications. 4 unresolved issues.

add Unreachablestrategy to AppUpdate and apply
ClosedAll Users

Authored by timcharper on Dec 9 2016, 12:56 AM.

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.
Changes from before your most recent comment are hidden. Show Older Changes
jeschkies added a subscriber: jeschkies.EditedDec 9 2016, 11:49 AM

Thanks for your work. This fixes #4810, doesn't it?

Is the update not specified in the RAML? Is it just the App?

Fun fact is that changing the UnreachableStrategy in the current implementation is neither an upgrade nor a scale change – we don't want any instance to be restart, nor to launch an additional one. But @aquamatthias is right, it must bei either of those, otherwise it's not persisted.

timcharper updated this revision to Diff 1186.Dec 9 2016, 4:41 PM

recognize unreachableStrategy as an upgrade change

I think the probability of this change regressing in the future is low. I don't feel like a test is required. If I am outvoted then I will write one.

timcharper retitled this revision from WIP: add Unreachablestrategy to AppUpdate and apply to add Unreachablestrategy to AppUpdate and apply.Dec 9 2016, 4:43 PM
meichstedt requested changes to this revision.Dec 9 2016, 4:43 PM
meichstedt added a reviewer: meichstedt.

As @aquamatthias mentioned, the unreachableStrategy must be checked for changes in AppDefinition.isUpgrade – otherwise an AppUpdate that changes only the unreachableStrategy will not be considered a change.

It's probably best to start with an additional test case in AppsResourceTest (and later also in PodsResource) that verifies that a runSpec's unreachableStrategy can be updated.

Refer to DeploymentPlan.dependencyOrderedSteps – since changing the unreachableStrategy is neither a scaleChange nor needsRestart, there will be no deploymentStep and the change will not be persisted. To not restart all instances when changing the strategy, you should either treat it as a Some(ScaleApplication(...)) (not so nice), or dig into the code and see how you can persist the runSpec w/o creating a deployment.

This revision now requires changes to proceed.Dec 9 2016, 4:43 PM
meichstedt accepted this revision.Dec 9 2016, 5:14 PM

Oh I see.

This revision is now accepted and ready to land.Dec 9 2016, 5:14 PM

Seems like there’s an additional thing needed for it to persist to zookeeper

jdef accepted this revision.Dec 9 2016, 5:33 PM
timcharper updated this revision to Diff 1197.Dec 9 2016, 9:12 PM

Add tests

jdef added inline comments.Dec 9 2016, 9:19 PM
src/main/proto/marathon.proto
117

interesting that I don't see killSelection in here. @jeschkies ?

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

also don't see killSelection here @jeschkies

timcharper updated this revision to Diff 1198.Dec 9 2016, 10:51 PM

le latest

This revision was automatically updated to reflect the committed changes.