Fixes #3723 - Fix validation of duplicate volume names #3737
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -115,24 +115,39 @@ private[impl] object DVDIProviderValidations extends ExternalVolumeValidations { | |
| // task instance across the entire cluster. | ||
| override lazy val rootGroup = new Validator[Group] { | ||
| override def apply(g: Group): Result = { | ||
| val appsByVolume: Map[String, Set[PathId]] = | ||
| g.transitiveApps | ||
| .flatMap { app => namesOfMatchingVolumes(app).map(_ -> app.id) } | ||
| .groupBy { case (volumeName, _) => volumeName } | ||
| .mapValues(_.map { case (volumeName, appId) => appId }) | ||
|
|
||
| val appValid: Validator[AppDefinition] = { | ||
| def volumeNameUnique(appId: PathId): Validator[ExternalVolume] = { | ||
| def conflictingApps(vol: ExternalVolume): Set[PathId] = | ||
| appsByVolume.getOrElse(vol.external.name, Set.empty).filter(_ != appId) | ||
|
|
||
| isTrue { (vol: ExternalVolume) => | ||
| val conflictingAppIds = conflictingApps(vol).mkString(", ") | ||
| s"Volume name '${vol.external.name}' in $appId conflicts with volume(s) of same name in app(s): " + | ||
|
There was a problem hiding this comment. Let's fix the path, and make the description simple. There was a problem hiding this comment. I am not sure what is best there. I'll keep it "as is". We can improve it later. |
||
| s"$conflictingAppIds" | ||
| }{ vol => conflictingApps(vol).isEmpty } | ||
| } | ||
|
|
||
| validator[AppDefinition] { app => | ||
| app.externalVolumes is every(volumeNameUnique(app.id)) | ||
|
There was a problem hiding this comment. To have the correct path here, you can do: app.externalVolumes as "container/volumes" is every(volumeNameUnique(app.id))There was a problem hiding this comment. Then the index is wrong Matthias Veit notifications@github.com schrieb am Di., 12. Apr. 2016
|
||
| } | ||
| } | ||
|
|
||
| def groupValid: Validator[Group] = validator[Group] { group => | ||
| group.apps is every(appValid) | ||
| group.groups is every(groupValid) | ||
| } | ||
|
|
||
| // We need to call the validators recursively such that the "description" of the rule violations | ||
| // is correctly calculated. | ||
| groupValid(g) | ||
| } | ||
|
|
||
| } | ||
|
|
||
| override lazy val app = { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| package mesosphere.marathon.core.externalvolume.impl.providers | ||
|
|
||
| import com.wix.accord.{ RuleViolation, Success, Failure, Result } | ||
| import mesosphere.marathon.api.v2.Validation | ||
| import mesosphere.marathon.core.externalvolume.ExternalVolumes | ||
| import mesosphere.marathon.state._ | ||
| import org.apache.mesos.{ Protos => MesosProtos } | ||
| import org.scalatest.{ FunSuite, GivenWhenThen, Matchers } | ||
| import play.api.libs.json.{ JsString, Json } | ||
|
|
||
| import scala.collection.immutable.Seq | ||
|
|
||
| class DVDIProviderRootGroupValidationTest extends FunSuite with Matchers with GivenWhenThen { | ||
|
|
||
| test("two volumes with different names should not result in an error") { | ||
| val f = new Fixture | ||
| Given("a root group with two apps and conflicting volumes") | ||
| val rootGroup = Group( | ||
| id = PathId.empty, | ||
| groups = Set( | ||
| Group( | ||
| id = PathId("/nested"), | ||
| apps = Set( | ||
| f.appWithDVDIVolume(appId = PathId("/nested/foo1"), volumeName = "vol1"), | ||
| f.appWithDVDIVolume(appId = PathId("/nested/foo2"), volumeName = "vol2") | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
|
|
||
| f.checkResult( | ||
| rootGroup, | ||
| expectedViolations = Set.empty | ||
| ) | ||
| } | ||
|
|
||
| test("two volumes with same name result in an error") { | ||
| val f = new Fixture | ||
| Given("a root group with two apps and conflicting volumes") | ||
| val app1 = f.appWithDVDIVolume(appId = PathId("/nested/app1"), volumeName = "vol") | ||
| val app2 = f.appWithDVDIVolume(appId = PathId("/nested/app2"), volumeName = "vol") | ||
| val rootGroup = Group( | ||
| id = PathId.empty, | ||
| groups = Set( | ||
| Group( | ||
| id = PathId("/nested"), | ||
| apps = Set(app1, app2) | ||
| ) | ||
| ) | ||
| ) | ||
|
|
||
|
There was a problem hiding this comment. TODO: |
||
| f.checkResult( | ||
| rootGroup, | ||
| expectedViolations = Set( | ||
| RuleViolation( | ||
| value = app1.externalVolumes.head, | ||
| constraint = "Volume name 'vol' in /nested/app1 conflicts with volume(s) of same name in app(s): /nested/app2", | ||
| description = Some("/groups(0)/apps(0)/externalVolumes(0)") | ||
| ), | ||
| RuleViolation( | ||
| value = app2.externalVolumes.head, | ||
| constraint = "Volume name 'vol' in /nested/app2 conflicts with volume(s) of same name in app(s): /nested/app1", | ||
| description = Some("/groups(0)/apps(1)/externalVolumes(0)") | ||
| ) | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| class Fixture { | ||
|
There was a problem hiding this comment. why is this a class instead of an object? There was a problem hiding this comment. We use a similar pattern elsewhere. If your |
||
| def appWithDVDIVolume(appId: PathId, volumeName: String, provider: String = DVDIProvider.name): AppDefinition = { | ||
| AppDefinition( | ||
| id = appId, | ||
| cmd = Some("sleep 123"), | ||
| upgradeStrategy = UpgradeStrategy.forResidentTasks, | ||
| container = Some( | ||
| Container( | ||
| `type` = MesosProtos.ContainerInfo.Type.MESOS, | ||
| volumes = Seq( | ||
| ExternalVolume( | ||
| containerPath = "ignoreme", | ||
| external = ExternalVolumeInfo( | ||
| name = volumeName, | ||
| provider = provider, | ||
| options = Map( | ||
| DVDIProvider.driverOption -> "rexray" | ||
| ) | ||
| ), | ||
| mode = MesosProtos.Volume.Mode.RW | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| def jsonResult(result: Result): String = { | ||
| Json.prettyPrint( | ||
| result match { | ||
| case Success => JsString("Success") | ||
| case f: Failure => Json.toJson(f)(Validation.failureWrites) | ||
| } | ||
| ) | ||
| } | ||
|
|
||
| def checkResult(rootGroup: Group, expectedViolations: Set[RuleViolation]): Unit = { | ||
| val expectFailure = expectedViolations.nonEmpty | ||
|
|
||
| When("validating the root group") | ||
| val result = DVDIProviderValidations.rootGroup(rootGroup) | ||
|
|
||
| Then(s"we should ${if (expectFailure) "" else "NOT "}get an error") | ||
| withClue(jsonResult(result)) { result.isFailure should be(expectFailure) } | ||
|
|
||
| And("ExternalVolumes validation agrees") | ||
| val externalVolumesResult = ExternalVolumes.validRootGroup()(rootGroup) | ||
| withClue(jsonResult(externalVolumesResult)) { externalVolumesResult.isFailure should be(expectFailure) } | ||
|
|
||
| And("global validation agrees") | ||
| val globalResult: Result = Group.validRootGroup(maxApps = None)(rootGroup) | ||
| withClue(jsonResult(globalResult)) { globalResult.isFailure should be(expectFailure) } | ||
|
|
||
| val ruleViolations = globalResult match { | ||
| case Success => Set.empty[RuleViolation] | ||
| case f: Failure => f.violations.flatMap(Validation.allRuleViolationsWithFullDescription(_)) | ||
| } | ||
|
|
||
| And("the rule violations are the expected ones") | ||
| withClue(jsonResult(globalResult)) { | ||
| ruleViolations should equal(expectedViolations) | ||
| } | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
g.appsinstead ofg.transitiveAppsprevented validation of apps in nested groups.