Exhaustive constraint migration
ClosedAll Users
Actions

Authored by ichernetsky on Aug 11 2017, 1:36 AM.

Details

Reviewers

kensipe
zen-dog
unterstein
timcharper
jdef

Commits

rMARATHONf4d3ff88ff97: Exhaustive constraint migration

JIRA Issues

JIRA MARATHON-4802 Add migration to fix invalid constraints

Summary

The existing migration code doesn't take into account all the possible ways
constraints created using prior Marathon versions, can be invalid. This diff
fixes it.

Loosening CLUSTER constraint because it was improperly tightened;
a value-less cluster constraint causes all tasks to be placed on the same
host as the first task.

Test Plan

sbt test

Diff Detail

Repository

rMARATHON marathon

Branch

ic/1.3-constraint-migration

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 3281
Build 6260: Marathon (revised)	Jenkins
Build 6259: arc lint + arc unit

ichernetsky created this revision.Aug 11 2017, 1:36 AM

Herald added a subscriber: marathon-dev. · View Herald TranscriptAug 11 2017, 1:36 AM

src/main/scala/mesosphere/marathon/state/Migration.scala
628	why the mixture of info/warn levels? shouldn't we just pick a level and be consistent?
631	latest docs say that CLUSTER doesn't require a value. code seems to agree: https://github.com/mesosphere/marathon/blob/releases/1.3/src/main/scala/mesosphere/mesos/Constraints.scala#L112
673	CLUSTER does not require a value
src/test/scala/mesosphere/marathon/state/MigrationTo_1_3_5Test.scala
53	CLUSTER does not require value

This revision now requires changes to proceed.Aug 11 2017, 3:17 PM

src/main/scala/mesosphere/marathon/state/Migration.scala
628	I chose `info`for constraints which need to be slightly adjusted (like removing an unused value). And I think `warn` is more appropriate when constraints get dropped, because they are invalid.
631	It depends on where to look for CLUSTER validation. In 1.3 it is considered to be invalid according to https://github.com/mesosphere/marathon/blob/releases/1.3/src/main/scala/mesosphere/marathon/state/AppDefinition.scala#L717 But I think this rule can be relaxed, so that a value is not required, as it is currently on 1.4 and master branches. What do you think?
673	Please refer to my response above.
src/test/scala/mesosphere/marathon/state/MigrationTo_1_3_5Test.scala
53	Please refer to my response above.

ichernetsky requested review of this revision.Aug 11 2017, 5:26 PM

src/main/scala/mesosphere/marathon/state/Migration.scala
628	let's pick one level and stick with it. I propose that any change to a user object be logged as a warning
631	To me, that looks like a bug in the validation logic for 1.3. Even the validation in 1.4 looks wrong: https://github.com/mesosphere/marathon/blob/releases/1.4/src/main/scala/mesosphere/marathon/state/AppDefinition.scala#L616 I think master has it right: https://github.com/mesosphere/marathon/blob/a5eff497221d2b7382a14e9884c1d1e5aa31a580/src/main/scala/mesosphere/marathon/api/v2/validation/SchedulingValidation.scala#L64

Address @jdef's comments

CLUSTER constraint doesn't require a value

ichernetsky marked 4 inline comments as done.Aug 11 2017, 6:13 PM

src/main/scala/mesosphere/marathon/state/Migration.scala
611	why are we turning format and scalastyle on and back off back to back? perhaps you are wanting to keep it as a function declaration but these macros are file level... we should remove lines 607-611

This revision now requires changes to proceed.Aug 11 2017, 6:33 PM

also wondering .. for people that have already migrated to 1.4 ... when they migrate to 1.5 they won't go through the constraint sanitation here (because this patch is written against the 1.3.x branch).
should the changes in this patch for constraint sanitation in migration be forward-ported to 1.4.x in order to facilitate a smoother upgrade to 1.5? (if so, it implies that we want users to be on the latest, patched 1.4.x release before upgrading to 1.5 in order to pick up the constraint sanitization)

src/main/scala/mesosphere/marathon/state/Migration.scala
611	I explicitly disable them where I need them disabled. They don't need to be disabled in between the methods. Especially, I would like to avoid a situation when a new function is added in between them later on without making sure that the effect of this `format/scalatyle` directives is limited to places in the code where they should be.

@jdef, as far as I can tell, upgrades skipping any version are not supported. This is what I heard from Matthias. @meichstedt, could you please clarify it here?

I'm specifically talking about the case where a user has already upgraded to 1.4.x, prior to this change landing in 1.3.x; they won't see the sanitation this patch introduces because it targets 1.3, right?

That's correct. Users of 1.1 are supposed to upgrade to 1.3 first, before upgrading to 1.4 Users of 1.4 do not need this sanitation, because the constraint validation in it is much better.

We shouldn't relax the CLUSTER constraint in this diff. Otherwise lgtm

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
716	I don't think we should mix migration with relaxation of the `CLUSTER` constraint. Especially since 1.4 still require `CLUSTER` to have a value. Do it in an extra diff if you want.

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
716	Good point. I can adjust the 1.4 in this regard, but on the other hand, I won't be always the case that a user upgrades the latest 1.3 version to the latest 1.4. Therefore I should bring the `CLUSTER` migration logic that was there initially. What do you think, @jdef?

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
716	I remember our upgrade policy being currently to upgrade from 1.3.x to 1.4x to 1.5.. where `x` is always the latest one. I think relaxing `CLUSTER` constraints should be done in 1.4 and backported to 1.3 in a separate effort.

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
716	https://phabricator.mesosphere.com/D986

Lgtm provided you revert the changes that are done in D986.

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
716	Cool. Revert this change here then?
src/test/scala/mesosphere/marathon/api/validation/RunSpecValidatorTest.scala
575	... and here?

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
716	I am not sure why, if the changes in D986 are for 1.4, and these changes are for 1.3.

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
716	Because this diff conflates 2 independent problems (migration and `CLUSTER` constraint) and the second one is not even mentioned in the diff description. I know it's more work this way but one diff should solve one problem.

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
716	I absolutely disagree that they are independent, because if not relaxing the constraint on `CLUSTER`,there should be additional clauses in the corresponding function to do `CLUSTER` constraint migration. Which means, that even if another diff to relax the constraint is created, the migration function would have to be adjusted too to take into account the constraint relaxation. And this proves that they are not independent. In addition to that it will require re-testing of the current diff, and testing a new diff, => at least 2 hours of additional work for no obvious to me reason.

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
716	I see. Ok then.

jdef resigned from this revision.Aug 23 2017, 9:30 PM

lgtm

This revision is now accepted and ready to land.Aug 23 2017, 10:32 PM

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
716	It isn't clear to me at all why we are relaxing this constraint; why not just solve it with a migration?
src/main/scala/mesosphere/marathon/state/Migration.scala
594	How do we currently interpret `"*"`?
611	I question the value of the cyclomatic complexity checker; it is too sensitive for functions with large pattern matching statements

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
716	/Users/tim/src/m8e/marathon-1.4/src/main/scala/mesosphere/mesos/Constraints.scala 97 \| case Operator.CLUSTER => 98 \| // Hostname must match or be empty 99 \| (value.isEmpty \|\| value == offer.getHostname) && 100 \| // All running tasks must have the same hostname as the one in the offer 101 \| allPlaced.forall(_.hostname == offer.getHostname) 102 \| case _ => false Oh geez it actually did have a meaning!!! 😨

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

716

/Users/tim/src/m8e/marathon-1.4/src/main/scala/mesosphere/mesos/Constraints.scala

  97 |         case Operator.CLUSTER =>
  98 |           // Hostname must match or be empty
  99 |           (value.isEmpty || value == offer.getHostname) &&
 100 |             // All running tasks must have the same hostname as the one in the offer
 101 |             allPlaced.forall(_.hostname == offer.getHostname)
 102 |         case _ => false

Oh geez it actually did have a meaning!!! 😨

ichernetsky edited the summary of this revision. (Show Details)Aug 23 2017, 11:05 PM

src/main/scala/mesosphere/marathon/state/AppDefinition.scala
716	Yeah, it works this way in all Marathon versions starting from 1.3 onwards
src/main/scala/mesosphere/marathon/state/Migration.scala
594	As an improper value. `LIKE` and `UNLIKE` operators must have regexes as their values.

ichernetsky closed this revision.Aug 23 2017, 11:25 PM

Revision Contents

			Path	Packages
M			src/main/scala/mesosphere/marathon/state/AppDefinition.scala (6 lines)
M			src/main/scala/mesosphere/marathon/state/Migration.scala (73 lines)
M			src/test/scala/mesosphere/marathon/api/validation/RunSpecValidatorTest.scala (4 lines)
M			src/test/scala/mesosphere/marathon/state/MigrationTo_1_3_5Test.scala (74 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	3989	acc2618		Aug 11 2017, 1:36 AM	★	★
Diff 2	3998	acc2618	- Address @jdef's comments	Aug 11 2017, 6:07 PM	★	★
Diff 3	3999	acc2618	- CLUSTER constraint doesn't require a value	Aug 11 2017, 6:12 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
5e49b16ae443	e7a60eff5428	e8b2ceb6eff1	Ivan Chernetsky	CLUSTER constraint doesn't require a value	Aug 11 2017, 6:12 PM
e8b2ceb6eff1	19e9eb7cb4f7	002b14624f60	Ivan Chernetsky	Address @jdef's comments	Aug 11 2017, 6:05 PM
002b14624f60	fc361159a981	acc261835097	Ivan Chernetsky	Exhaustive constraint migration (Show More…)	Aug 10 2017, 11:44 PM

Diff 3999

View Options

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

Show First 20 Lines • Show All 707 Lines • ▼ Show 20 Line(s)
708	708		} else {
709	709		c.getOperator match {
710	710		case UNIQUE =>
711	711		if (c.hasValue && c.getValue.nonEmpty) {
712	712		Failure(Set(RuleViolation(c, "Value specified but not used", None)))
713	713		} else {
714	714		Success
715	715		}
716	716		case CLUSTER =>
		zen-dogUnsubmitted Not Done I don't think we should mix migration with relaxation of the `CLUSTER` constraint. Especially since 1.4 still require `CLUSTER` to have a value. Do it in an extra diff if you want.
		ichernetskyAuthorUnsubmitted Not Done Good point. I can adjust the 1.4 in this regard, but on the other hand, I won't be always the case that a user upgrades the latest 1.3 version to the latest 1.4. Therefore I should bring the `CLUSTER` migration logic that was there initially. What do you think, @jdef?
		zen-dogUnsubmitted Done I remember our upgrade policy being currently to upgrade from 1.3.x to 1.4x to 1.5.. where `x` is always the latest one. I think relaxing `CLUSTER` constraints should be done in 1.4 and backported to 1.3 in a separate effort.
		ichernetskyAuthorUnsubmitted Not Done https://phabricator.mesosphere.com/D986
		zen-dogUnsubmitted Not Done Cool. Revert this change here then?
		ichernetskyAuthorUnsubmitted Not Done I am not sure why, if the changes in D986 are for 1.4, and these changes are for 1.3.
		zen-dogUnsubmitted Not Done Because this diff conflates 2 independent problems (migration and `CLUSTER` constraint) and the second one is not even mentioned in the diff description. I know it's more work this way but one diff should solve one problem.
		ichernetskyAuthorUnsubmitted Not Done I absolutely disagree that they are independent, because if not relaxing the constraint on `CLUSTER`,there should be additional clauses in the corresponding function to do `CLUSTER` constraint migration. Which means, that even if another diff to relax the constraint is created, the migration function would have to be adjusted too to take into account the constraint relaxation. And this proves that they are not independent. In addition to that it will require re-testing of the current diff, and testing a new diff, => at least 2 hours of additional work for no obvious to me reason.
		zen-dogUnsubmitted Not Done I see. Ok then.
		timcharperUnsubmitted Not Done It isn't clear to me at all why we are relaxing this constraint; why not just solve it with a migration?
		timcharperUnsubmitted Not Done /Users/tim/src/m8e/marathon-1.4/src/main/scala/mesosphere/mesos/Constraints.scala 97 \| case Operator.CLUSTER => 98 \| // Hostname must match or be empty 99 \| (value.isEmpty \|\| value == offer.getHostname) && 100 \| // All running tasks must have the same hostname as the one in the offer 101 \| allPlaced.forall(_.hostname == offer.getHostname) 102 \| case _ => false Oh geez it actually did have a meaning!!! 😨
		ichernetskyAuthorUnsubmitted Not Done Yeah, it works this way in all Marathon versions starting from 1.3 onwards
717			if (c.hasValue && c.getValue.nonEmpty) {
718	717		Success
719			} else {
720			Failure(Set(RuleViolation(c, "Missing value", None)))
721			}
722	718		case GROUP_BY =>
723	719		if (!c.hasValue \|\| (c.hasValue && c.getValue.nonEmpty && Try(c.getValue.toInt).isSuccess)) {
724	720		Success
725	721		} else {
726	722		Failure(Set(RuleViolation(
727	723		c,
728	724		"Value was specified but is not a number",
729	725		Some("GROUP_BY may either have no value or an integer value"))))
▲ Show 20 Lines • Show All 79 Lines • Show Last 20 Lines

View Options

src/main/scala/mesosphere/marathon/state/Migration.scala

Show First 20 Lines • Show All 560 Lines • ▼ Show 20 Line(s)
561	561		log.info("Finished 1.2 migration")
562	562		}
563	563		}
564	564
565	565		class MigrationTo_1_3_5(appRepository: AppRepository, groupRepository: GroupRepository,
566	566		deploymentRepository: DeploymentRepository) {
567	567		private[this] val log = LoggerFactory.getLogger(getClass)
568	568
569			private def isBrokenConstraint(constraint: Constraint): Boolean = {
570			(constraint.getOperator == Constraint.Operator.LIKE \|\|
571			constraint.getOperator == Constraint.Operator.UNLIKE) &&
572			(constraint.hasValue && Try(Pattern.compile(constraint.getValue)).isFailure)
573			}
574
575			private def fixConstraints(app: AppDefinition): AppDefinition = {
576			val newConstraints: Set[Constraint] = app.constraints.flatMap { constraint =>
	569		// scalastyle:off cyclomatic.complexity
	570		// format: OFF
	571		private def fixConstraint(appId: PathId, constraint: Constraint): Option[Constraint] = {
577	572		constraint.getOperator match {
578			case Constraint.Operator.LIKE \| Constraint.Operator.UNLIKE if isBrokenConstraint(constraint) =>
579			// we know we have a value and the pattern doesn't compile already.
580			if (constraint.getValue == "*") {
581			Some(constraint.toBuilder.setValue(".*").build())
582			} else {
583			log.warn(s"Removing invalid constraint $constraint from ${app.id}")
	573		case Constraint.Operator.UNIQUE
	574		if constraint.hasValue =>
	575		log.warn(s"Removing the value of $constraint in $appId app")
	576		Option(constraint.toBuilder.clearValue().build())
	577
	578		case Constraint.Operator.GROUP_BY
	579		if constraint.hasValue && constraint.getValue.isEmpty =>
	580		log.warn(s"Removing the empty value of $constraint in $appId app")
	581		Option(constraint.toBuilder.clearValue().build())
	582
	583		case Constraint.Operator.GROUP_BY
	584		if constraint.hasValue && Try(constraint.getValue.toInt).isFailure =>
	585		log.warn(s"Removing the invalid constraint $constraint from $appId app")
584	586		None
	587
	588		case Constraint.Operator.LIKE \| Constraint.Operator.UNLIKE
	589		if !constraint.hasValue =>
	590		log.warn(s"Removing the invalid constraint $constraint from $appId app")
	591		None
	592
	593		case Constraint.Operator.LIKE \| Constraint.Operator.UNLIKE
	594		if constraint.hasValue && constraint.getValue == "*" =>
		timcharperUnsubmitted Not Done How do we currently interpret `"*"`?
		ichernetskyAuthorUnsubmitted Not Done As an improper value. `LIKE` and `UNLIKE` operators must have regexes as their values.
	595		log.warn(s"""Replacing the value "" of $constraint with "." in $appId app""")
	596		Option(constraint.toBuilder.setValue(".*").build())
	597
	598		case Constraint.Operator.LIKE \| Constraint.Operator.UNLIKE
	599		if constraint.hasValue && Try(Pattern.compile(constraint.getValue)).isFailure =>
	600		log.warn(s"Removing the invalid constraint $constraint from $appId app")
	601		None
	602
	603		case _ =>
	604		Option(constraint)
585	605		}
586			case _ => Some(constraint)
587	606		}
588			}(collection.breakOut)
	607		// format: ON
	608		// scalastyle:on cyclomatic.complexity
	609
	610		// scalastyle:off cyclomatic.complexity
	611		// format: OFF
		kensipeUnsubmitted Not Done why are we turning format and scalastyle on and back off back to back? perhaps you are wanting to keep it as a function declaration but these macros are file level... we should remove lines 607-611
		ichernetskyAuthorUnsubmitted Not Done I explicitly disable them where I need them disabled. They don't need to be disabled in between the methods. Especially, I would like to avoid a situation when a new function is added in between them later on without making sure that the effect of this `format/scalatyle` directives is limited to places in the code where they should be.
		timcharperUnsubmitted Not Done I question the value of the cyclomatic complexity checker; it is too sensitive for functions with large pattern matching statements
	612		private def isBrokenConstraint(constraint: Constraint): Boolean = constraint.getOperator match {
	613		case Constraint.Operator.UNIQUE
	614		if constraint.hasValue => true
	615		case Constraint.Operator.LIKE \| Constraint.Operator.UNLIKE
	616		if !constraint.hasValue => true
	617		case Constraint.Operator.GROUP_BY
	618		if constraint.hasValue && Try(constraint.getValue.toInt).isFailure => true
	619		case Constraint.Operator.LIKE \| Constraint.Operator.UNLIKE
	620		if constraint.hasValue && Try(Pattern.compile(constraint.getValue)).isFailure => true
	621		case _ => false
	622		}
	623		// format: ON
	624		// scalastyle:on cyclomatic.complexity
	625
	626		private def fixConstraints(app: AppDefinition): AppDefinition = {
	627		val newConstraints: Set[Constraint] = app.constraints.flatMap(fixConstraint(app.id, _))(collection.breakOut)
589	628		app.copy(constraints = newConstraints)
		jdefUnsubmitted Done why the mixture of info/warn levels? shouldn't we just pick a level and be consistent?
		ichernetskyAuthorUnsubmitted Not Done I chose `info`for constraints which need to be slightly adjusted (like removing an unused value). And I think `warn` is more appropriate when constraints get dropped, because they are invalid.
		jdefUnsubmitted Done let's pick one level and stick with it. I propose that any change to a user object be logged as a warning
590	629		}
591	630
592	631		private def migrateApps(): Future[Done] = async {
		jdefUnsubmitted Done latest docs say that CLUSTER doesn't require a value. code seems to agree: https://github.com/mesosphere/marathon/blob/releases/1.3/src/main/scala/mesosphere/mesos/Constraints.scala#L112
		ichernetskyAuthorUnsubmitted Not Done It depends on where to look for CLUSTER validation. In 1.3 it is considered to be invalid according to https://github.com/mesosphere/marathon/blob/releases/1.3/src/main/scala/mesosphere/marathon/state/AppDefinition.scala#L717 But I think this rule can be relaxed, so that a value is not required, as it is currently on 1.4 and master branches. What do you think?
		jdefUnsubmitted Done To me, that looks like a bug in the validation logic for 1.3. Even the validation in 1.4 looks wrong: https://github.com/mesosphere/marathon/blob/releases/1.4/src/main/scala/mesosphere/marathon/state/AppDefinition.scala#L616 I think master has it right: https://github.com/mesosphere/marathon/blob/a5eff497221d2b7382a14e9884c1d1e5aa31a580/src/main/scala/mesosphere/marathon/api/v2/validation/SchedulingValidation.scala#L64
593	632		val apps = await(appRepository.apps())
594	633		val brokenApps = apps.collect {
595	634		case app: AppDefinition if app.constraints.exists(isBrokenConstraint) => app
596	635		}
597	636		log.info(s"Fixing apps with invalid constraints: ${brokenApps.map(_.id).mkString(", ")}")
598	637		val futures = brokenApps.map(app => appRepository.store(app))
599	638		await(Future.sequence(futures))
600	639		Done
Show All 25 Lines
626	665		}
627	666
628	667		private def migratePlans(): Future[Done] = async {
629	668		val plans = await(deploymentRepository.all())
630	669		val badPlans = plans.collect {
631	670		case plan: DeploymentPlan if plan.original.transitiveApps.exists(app => app.constraints.exists(isBrokenConstraint)) \|\| // scalastyle:off
632	671		plan.target.transitiveApps.exists(app => app.constraints.exists(isBrokenConstraint)) => plan
633	672		}
634	673		val futures = badPlans.map { plan =>
		jdefUnsubmitted Not Done CLUSTER does not require a value
		ichernetskyAuthorUnsubmitted Not Done Please refer to my response above.
635	674		log.info(s"Fixing plan ${plan.id} as it contains apps with invalid " +
636	675		"constraints (even if they are not affected by the plan itself)")
637	676		deploymentRepository.store(plan.copy(original = fixRoot(plan.original), target = fixRoot(plan.target)))
638	677		}
639	678		await(Future.sequence(futures))
640	679		Done
641	680		}
642	681
▲ Show 20 Lines • Show All 44 Lines • Show Last 20 Lines

View Options

src/test/scala/mesosphere/marathon/api/validation/RunSpecValidatorTest.scala

Show First 20 Lines • Show All 565 Lines • ▼ Show 20 Line(s)
566	566
567	567		Given("A UNIQUE constraint with a value")
568	568		val appUniqueValue = app.copy(constraints = Set(unique.setValue("a").build()))
569	569		Then("validation fails")
570	570		validAppDefinition(appUniqueValue).isFailure shouldBe true
571	571
572	572		Given("A CLUSTER constraint without a value")
573	573		val appClusterNoValue = app.copy(constraints = Set(cluster.build()))
574			Then("validation fails")
575			validAppDefinition(appClusterNoValue).isFailure shouldBe true
	574		Then("validation succeeds")
	575		validAppDefinition(appClusterNoValue).isSuccess shouldBe true
		zen-dogUnsubmitted Not Done ... and here?
576	576
577	577		Given("A CLUSTER constraint with a value")
578	578		val appCluster = app.copy(constraints = Set(cluster.setValue("abc").build()))
579	579		Then("validation succeeds")
580	580		validAppDefinition(appCluster).isSuccess shouldBe true
581	581
582	582		Given("A GROUP_BY without a value")
583	583		val appGroupByNoValue = app.copy(constraints = Set(groupBy.build()))
▲ Show 20 Lines • Show All 251 Lines • Show Last 20 Lines

View Options

src/test/scala/mesosphere/marathon/state/MigrationTo_1_3_5Test.scala

Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Line(s)
43	43		}
44	44		}
45	45
46	46		"app has broken constraints" should {
47	47		val appRepo = mock[AppRepository]
48	48		val groupRepo = mock[GroupRepository]
49	49		val deployRepo = mock[DeploymentRepository]
50	50
51			"fix '*' regex's, remove bad regex's and preserve non-broken constraints" in {
	51		"fix malformed constraints, remove invalid ones and preserve valid ones" in {
52	52		val badApp = AppDefinition(id = PathId("/badApp"), constraints = Set(
	53		constraint("hostname", GROUP_BY, Some("str")),
		jdefUnsubmitted Not Done CLUSTER does not require value
		ichernetskyAuthorUnsubmitted Not Done Please refer to my response above.
	54		constraint("hostname", LIKE, None),
	55		constraint("hostname", LIKE, Some("+")), // not a valid regex
	56		constraint("hostname", UNLIKE, None),
	57		constraint("hostname", UNLIKE, Some("+")),
	58
	59		// valid constraints
	60		constraint("hostname", UNIQUE, None),
	61		constraint("hostname", CLUSTER, Some("host")),
	62		constraint("hostname", GROUP_BY, None),
	63		constraint("hostname", GROUP_BY, Some("3")),
	64		constraint("hostname", LIKE, Some("\\w+")),
	65		constraint("hostname", UNLIKE, Some("\\w+"))))
	66
	67		val adjustedApp = AppDefinition(id = PathId("/adjustedApp"), constraints = Set(
	68		constraint("hostname", UNIQUE, Some("")),
	69		constraint("hostname", UNIQUE, Some("not-used")),
	70		constraint("hostname", GROUP_BY, Some("")),
53	71		constraint("hostname", LIKE, Some("*")),
54	72		constraint("hostname", UNLIKE, Some("*")),
	73
	74		// valid constraints
	75		constraint("hostname", CLUSTER, Some("host")),
	76		constraint("hostname", GROUP_BY, None),
	77		constraint("hostname", GROUP_BY, Some("3")),
55	78		constraint("hostname", LIKE, Some("\\w+")),
56			constraint("hostname", UNLIKE, Some("\\w+")),
57			constraint("hostname", LIKE, Some("+")), // not a valid regex
58			constraint("hostname", UNLIKE, Some("+")),
59			constraint("hostname", GROUP_BY, None)))
	79		constraint("hostname", UNLIKE, Some("\\w+"))))
	80
60	81		val goodApp = AppDefinition(id = PathId("/goodApp"))
61			val root = Group(id = PathId("/"), apps = Map(badApp.id -> badApp, goodApp.id -> goodApp),
62			groups = Set(Group(id = PathId("/a"), apps = Map(PathId("/a/bad") -> badApp, PathId("/a/bad") -> goodApp)))
	82		val root = Group(
	83		id = PathId("/"),
	84		apps = Map(badApp.id -> badApp, adjustedApp.id -> adjustedApp, goodApp.id -> goodApp),
	85		groups = Set(Group(
	86		id = PathId("/a"),
	87		apps = Map(PathId("/a/bad") -> badApp, PathId("/a/adjusted") -> adjustedApp, PathId("/a/good") -> goodApp)))
63	88		)
64	89		val badPlan = DeploymentPlan(root, root)
65	90		val goodPlan = DeploymentPlan(Group.empty, Group.empty)
66	91
67			appRepo.apps() returns Future.successful(Iterable(badApp, goodApp))
	92		appRepo.apps() returns Future.successful(Iterable(badApp, adjustedApp, goodApp))
68	93		groupRepo.rootGroup() returns Future.successful(Some(root))
69	94		deployRepo.all() returns Future.successful(Seq(goodPlan, badPlan))
70	95
71	96		appRepo.store(any) returns Future.successful(goodApp)
72	97		groupRepo.store(any, any) returns Future.successful(root)
73	98		deployRepo.store(any) returns Future.successful(badPlan)
74	99
75	100		new MigrationTo_1_3_5(appRepo, groupRepo, deployRepo).migrate().futureValue
76	101
77			val fixedApp = AppDefinition(id = PathId("/badApp"), constraints = Set(
	102		val fixedBadApp = AppDefinition(id = PathId("/badApp"), constraints = Set(
	103		constraint("hostname", UNIQUE, None),
	104		constraint("hostname", CLUSTER, Some("host")),
	105		constraint("hostname", GROUP_BY, None),
	106		constraint("hostname", GROUP_BY, Some("3")),
	107		constraint("hostname", LIKE, Some("\\w+")),
	108		constraint("hostname", UNLIKE, Some("\\w+"))))
	109
	110		val fixedAdjustedApp = AppDefinition(id = PathId("/adjustedApp"), constraints = Set(
	111		// adjusted constraints
	112		constraint("hostname", UNIQUE, None),
78	113		constraint("hostname", LIKE, Some(".*")),
79	114		constraint("hostname", UNLIKE, Some(".*")),
	115
	116		// valid constraints
	117		constraint("hostname", CLUSTER, Some("host")),
	118		constraint("hostname", GROUP_BY, None),
	119		constraint("hostname", GROUP_BY, Some("3")),
80	120		constraint("hostname", LIKE, Some("\\w+")),
81			constraint("hostname", UNLIKE, Some("\\w+")),
82			constraint("hostname", GROUP_BY, None)))
83			val fixedRoot = Group(id = PathId("/"), apps = Map(badApp.id -> fixedApp, goodApp.id -> goodApp),
84			groups = Set(Group(id = PathId("/a"), apps = Map(PathId("/a/bad") -> fixedApp, PathId("/a/bad") -> goodApp),
	121		constraint("hostname", UNLIKE, Some("\\w+"))))
	122
	123		val fixedRoot = Group(
	124		id = PathId("/"),
	125		apps = Map(badApp.id -> fixedBadApp, adjustedApp.id -> fixedAdjustedApp, goodApp.id -> goodApp),
	126		groups = Set(Group(
	127		id = PathId("/a"),
	128		apps = Map(PathId("/a/bad") -> fixedBadApp, PathId("/a/adjusted") -> fixedAdjustedApp,
	129		PathId("/a/bad") -> goodApp),
85	130		version = root.version)),
86	131		version = root.version
87	132		)
88	133		val fixedPlan = badPlan.copy(original = fixedRoot, target = fixedRoot)
89	134
90	135		verify(appRepo).apps()
91	136		verify(groupRepo).rootGroup()
92	137		verify(deployRepo).all()
93			verify(appRepo).store(fixedApp)
	138		verify(appRepo).store(fixedBadApp)
	139		verify(appRepo).store(fixedAdjustedApp)
94	140		verify(groupRepo).store(GroupRepository.zkRootName, fixedRoot)
95	141		verify(deployRepo).store(fixedPlan)
96	142		noMoreInteractions(appRepo, groupRepo, deployRepo)
97	143		}
98	144		}
99	145		}
100	146		}

Exhaustive constraint migrationClosedAll UsersActions

Details

Diff Detail

Revision Contents

Diff 3999

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

src/main/scala/mesosphere/marathon/state/Migration.scala

src/test/scala/mesosphere/marathon/api/validation/RunSpecValidatorTest.scala

src/test/scala/mesosphere/marathon/state/MigrationTo_1_3_5Test.scala

Exhaustive constraint migration
ClosedAll Users
Actions