Good point, I think `DEFAULT_TEST_ROLE` would be better. I was indeed
hoping to make use of this in other tests, but we could keep them local to
the tests for now, and move it later if it becomes a common pattern.

Greg

On Thu, Sep 22, 2016 at 10:13 AM, Neil Conway <neil.con...@gmail.com> wrote:

> I'm not sure this is a good idea: the "default role" is actually "*".
> That is also the default value for the "role" fields in the protobufs.
> Perhaps we should name this new constant something like
> DEFAULT_TEST_ROLE?
>
> I wonder also if we should keep the definition local to
> "persistent_volume_tests.cpp", unless there are imminent plans to use
> it elsewhere.
>
> Neil
>
> On Thu, Sep 22, 2016 at 6:37 PM,  <mp...@apache.org> wrote:
> > Repository: mesos
> > Updated Branches:
> >   refs/heads/master 4df496aaf -> c2b595e1c
> >
> >
> > Added `DEFAULT_ROLE` constant to persistent volume tests.
> >
> > Review: https://reviews.apache.org/r/41613/
> >
> >
> > Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
> > Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/c2b595e1
> > Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/c2b595e1
> > Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/c2b595e1
> >
> > Branch: refs/heads/master
> > Commit: c2b595e1c59bb23e3f87545a7e22a76ad232ae9f
> > Parents: 4df496a
> > Author: Greg Mann <g...@mesosphere.io>
> > Authored: Thu Sep 22 15:47:31 2016 +0200
> > Committer: Michael Park <mp...@apache.org>
> > Committed: Thu Sep 22 17:28:32 2016 +0200
> >
> > ----------------------------------------------------------------------
> >  src/tests/mesos.hpp                   |  1 +
> >  src/tests/persistent_volume_tests.cpp | 51
> +++++++++++++++++-------------
> >  2 files changed, 30 insertions(+), 22 deletions(-)
> > ----------------------------------------------------------------------
> >
> >
> > http://git-wip-us.apache.org/repos/asf/mesos/blob/c2b595e1/
> src/tests/mesos.hpp
> > ----------------------------------------------------------------------
> > diff --git a/src/tests/mesos.hpp b/src/tests/mesos.hpp
> > index 7095101..3cd63bd 100644
> > --- a/src/tests/mesos.hpp
> > +++ b/src/tests/mesos.hpp
> > @@ -93,6 +93,7 @@ namespace tests {
> >
> >  constexpr char READONLY_HTTP_AUTHENTICATION_REALM[] =
> "test-readonly-realm";
> >  constexpr char READWRITE_HTTP_AUTHENTICATION_REALM[] =
> "test-readwrite-realm";
> > +constexpr char DEFAULT_ROLE[] = "default-role";
> >
> >  // Forward declarations.
> >  class MockExecutor;
> >
> > http://git-wip-us.apache.org/repos/asf/mesos/blob/c2b595e1/
> src/tests/persistent_volume_tests.cpp
> > ----------------------------------------------------------------------
> > diff --git a/src/tests/persistent_volume_tests.cpp
> b/src/tests/persistent_volume_tests.cpp
> > index c38d848..a726d78 100644
> > --- a/src/tests/persistent_volume_tests.cpp
> > +++ b/src/tests/persistent_volume_tests.cpp
> > @@ -166,7 +166,7 @@ protected:
> >        case NONE: {
> >          diskResource = createDiskResource(
> >              stringify(mb.megabytes()),
> > -            "role1",
> > +            DEFAULT_ROLE,
> >              None(),
> >              None());
> >
> > @@ -175,7 +175,7 @@ protected:
> >        case PATH: {
> >          diskResource = createDiskResource(
> >              stringify(mb.megabytes()),
> > -            "role1",
> > +            DEFAULT_ROLE,
> >              None(),
> >              None(),
> >              createDiskSourcePath(path::join(diskPath, "disk" +
> stringify(id))));
> > @@ -185,7 +185,7 @@ protected:
> >        case MOUNT: {
> >          diskResource = createDiskResource(
> >              stringify(mb.megabytes()),
> > -            "role1",
> > +            DEFAULT_ROLE,
> >              None(),
> >              None(),
> >              createDiskSourceMount(
> > @@ -254,7 +254,7 @@ TEST_P(PersistentVolumeTest,
> CreateAndDestroyPersistentVolumes)
> >    Clock::pause();
> >
> >    FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
> > -  frameworkInfo.set_role("role1");
> > +  frameworkInfo.set_role(DEFAULT_ROLE);
> >
> >    // Create a master.
> >    master::Flags masterFlags = CreateMasterFlags();
> > @@ -429,7 +429,7 @@ TEST_P(PersistentVolumeTest, ResourcesCheckpointing)
> >    ASSERT_SOME(slave);
> >
> >    FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
> > -  frameworkInfo.set_role("role1");
> > +  frameworkInfo.set_role(DEFAULT_ROLE);
> >
> >    MockScheduler sched;
> >    MesosSchedulerDriver driver(
> > @@ -495,7 +495,7 @@ TEST_P(PersistentVolumeTest, PreparePersistentVolume)
> >    ASSERT_SOME(slave);
> >
> >    FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
> > -  frameworkInfo.set_role("role1");
> > +  frameworkInfo.set_role(DEFAULT_ROLE);
> >
> >    MockScheduler sched;
> >    MesosSchedulerDriver driver(
> > @@ -564,7 +564,7 @@ TEST_P(PersistentVolumeTest, MasterFailover)
> >    ASSERT_SOME(slave);
> >
> >    FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
> > -  frameworkInfo.set_role("role1");
> > +  frameworkInfo.set_role(DEFAULT_ROLE);
> >
> >    MockScheduler sched;
> >    TestingMesosSchedulerDriver driver(&sched, &detector, frameworkInfo);
> > @@ -659,7 +659,7 @@ TEST_P(PersistentVolumeTest,
> IncompatibleCheckpointedResources)
> >    spawn(slave1);
> >
> >    FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
> > -  frameworkInfo.set_role("role1");
> > +  frameworkInfo.set_role(DEFAULT_ROLE);
> >
> >    MockScheduler sched;
> >    MesosSchedulerDriver driver(
> > @@ -746,7 +746,7 @@ TEST_P(PersistentVolumeTest, AccessPersistentVolume)
> >    ASSERT_SOME(slave);
> >
> >    FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
> > -  frameworkInfo.set_role("role1");
> > +  frameworkInfo.set_role(DEFAULT_ROLE);
> >
> >    MockScheduler sched;
> >    MesosSchedulerDriver driver(
> > @@ -899,14 +899,15 @@ TEST_P(PersistentVolumeTest,
> SharedPersistentVolumeMultipleTasks)
> >    ASSERT_SOME(master);
> >
> >    slave::Flags slaveFlags = CreateSlaveFlags();
> > -  slaveFlags.resources = "cpus:2;mem:1024;disk(role1):1024";
> > +  slaveFlags.resources =
> > +    "cpus:2;mem:1024;disk(" + string(DEFAULT_ROLE) + "):1024";
> >
> >    Owned<MasterDetector> detector = master.get()->createDetector();
> >    Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(),
> slaveFlags);
> >    ASSERT_SOME(slave);
> >
> >    FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
> > -  frameworkInfo.set_role("role1");
> > +  frameworkInfo.set_role(DEFAULT_ROLE);
> >
> >    MockScheduler sched;
> >    MesosSchedulerDriver driver(
> > @@ -932,7 +933,7 @@ TEST_P(PersistentVolumeTest,
> SharedPersistentVolumeMultipleTasks)
> >
> >    Resources volume = createPersistentVolume(
> >        Megabytes(64),
> > -      "role1",
> > +      DEFAULT_ROLE,
> >        "id1",
> >        "path1",
> >        None(),
> > @@ -940,15 +941,21 @@ TEST_P(PersistentVolumeTest,
> SharedPersistentVolumeMultipleTasks)
> >        frameworkInfo.principal(),
> >        true); // Shared.
> >
> > +  Try<Resources> taskResources1 =
> > +    Resources::parse("cpus:1;mem:128;disk(" + string(DEFAULT_ROLE) +
> "):32");
> > +
> >    // Create 2 tasks which write distinct files in the shared volume.
> >    TaskInfo task1 = createTask(
> >        offer.slave_id(),
> > -      Resources::parse("cpus:1;mem:128;disk(role1):32").get() + volume,
> > +      taskResources1.get() + volume,
> >        "echo task1 > path1/file1");
> >
> > +  Try<Resources> taskResources2 =
> > +    Resources::parse("cpus:1;mem:256;disk(" + string(DEFAULT_ROLE) +
> "):64");
> > +
> >    TaskInfo task2 = createTask(
> >        offer.slave_id(),
> > -      Resources::parse("cpus:1;mem:256;disk(role1):64").get() + volume,
> > +      taskResources2.get() + volume,
> >        "echo task2 > path1/file2");
> >
> >    // We should receive a TASK_RUNNING followed by a TASK_FINISHED for
> > @@ -981,7 +988,7 @@ TEST_P(PersistentVolumeTest,
> SharedPersistentVolumeMultipleTasks)
> >
> >    const string& volumePath = slave::paths::getPersistentVolumePath(
> >        slaveFlags.work_dir,
> > -      "role1",
> > +      DEFAULT_ROLE,
> >        "id1");
> >
> >    EXPECT_SOME_EQ("task1\n", os::read(path::join(volumePath, "file1")));
> > @@ -1011,7 +1018,7 @@ TEST_P(PersistentVolumeTest, SlaveRecovery)
> >    ASSERT_SOME(slave);
> >
> >    FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
> > -  frameworkInfo.set_role("role1");
> > +  frameworkInfo.set_role(DEFAULT_ROLE);
> >    frameworkInfo.set_checkpoint(true);
> >
> >    MockScheduler sched;
> > @@ -1145,7 +1152,7 @@ TEST_P(PersistentVolumeTest,
> GoodACLCreateThenDestroy)
> >    filters.set_refuse_seconds(0);
> >
> >    FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
> > -  frameworkInfo.set_role("role1");
> > +  frameworkInfo.set_role(DEFAULT_ROLE);
> >
> >    // Create a master.
> >    master::Flags masterFlags = CreateMasterFlags();
> > @@ -1291,7 +1298,7 @@ TEST_P(PersistentVolumeTest, GoodACLNoPrincipal)
> >    FrameworkInfo frameworkInfo;
> >    frameworkInfo.set_name("no-principal");
> >    frameworkInfo.set_user(os::user().get());
> > -  frameworkInfo.set_role("role1");
> > +  frameworkInfo.set_role(DEFAULT_ROLE);
> >
> >    // Create a master. Since the framework has no
> >    // principal, we don't authenticate frameworks.
> > @@ -1445,11 +1452,11 @@ TEST_P(PersistentVolumeTest, BadACLNoPrincipal)
> >    FrameworkInfo frameworkInfo1;
> >    frameworkInfo1.set_name("no-principal");
> >    frameworkInfo1.set_user(os::user().get());
> > -  frameworkInfo1.set_role("role1");
> > +  frameworkInfo1.set_role(DEFAULT_ROLE);
> >
> >    // Create a `FrameworkInfo` with a principal.
> >    FrameworkInfo frameworkInfo2 = DEFAULT_FRAMEWORK_INFO;
> > -  frameworkInfo2.set_role("role1");
> > +  frameworkInfo2.set_role(DEFAULT_ROLE);
> >
> >    // Create a master. Since one framework has no
> >    // principal, we don't authenticate frameworks.
> > @@ -1660,13 +1667,13 @@ TEST_P(PersistentVolumeTest,
> BadACLDropCreateAndDestroy)
> >
> >    // Create a `FrameworkInfo` that cannot create or destroy volumes.
> >    FrameworkInfo frameworkInfo1 = DEFAULT_FRAMEWORK_INFO;
> > -  frameworkInfo1.set_role("role1");
> > +  frameworkInfo1.set_role(DEFAULT_ROLE);
> >
> >    // Create a `FrameworkInfo` that can create volumes.
> >    FrameworkInfo frameworkInfo2;
> >    frameworkInfo2.set_name("creator-framework");
> >    frameworkInfo2.set_user(os::user().get());
> > -  frameworkInfo2.set_role("role1");
> > +  frameworkInfo2.set_role(DEFAULT_ROLE);
> >    frameworkInfo2.set_principal("creator-principal");
> >
> >    // Create a master.
> >
>

Reply via email to