Re: Review Request 49520: Fixed the flaky BusyMountPoint test.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49520/#review141302
---


Fix it, then Ship it!





src/tests/gc_tests.cpp (line 899)


We don't need to name it `_mountPoint` anymore now that we don't have the 
test fixture's member variable `mountPoint`.

I'll clean it up when committing.


- Jiang Yan Xu


On July 8, 2016, 10:29 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49520/
> ---
> 
> (Updated July 8, 2016, 10:29 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5752
> https://issues.apache.org/jira/browse/MESOS-5752
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In GarbageCollectorIntegrationTest.BusyMountPoint there is
> a race between the task creating a file and the test looking for it
> which sometimes leads to assertion failure on the existence of the
> file.
> 
> 
> Diffs
> -
> 
>   src/tests/gc_tests.cpp d6335dc711f0697a0728a912e713cf1ed783c146 
> 
> Diff: https://reviews.apache.org/r/49520/diff/
> 
> 
> Testing
> ---
> 
> make check on mac os and linux for 50 iterations and verified the temp 
> directory is cleaned up after the test.
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 49520: Fixed the flaky BusyMountPoint test.

[Megha Sharma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49520/
---

(Updated July 8, 2016, 10:29 a.m.)


Review request for mesos and Jiang Yan Xu.


Bugs: MESOS-5752
https://issues.apache.org/jira/browse/MESOS-5752


Repository: mesos


Description
---

In GarbageCollectorIntegrationTest.BusyMountPoint there is
a race between the task creating a file and the test looking for it
which sometimes leads to assertion failure on the existence of the
file.


Diffs
-

  src/tests/gc_tests.cpp d6335dc711f0697a0728a912e713cf1ed783c146 

Diff: https://reviews.apache.org/r/49520/diff/


Testing (updated)
---

make check on mac os and linux for 50 iterations and verified the temp 
directory is cleaned up after the test.


Thanks,

Megha Sharma

Re: Review Request 49520: Fixed the flaky BusyMountPoint test.

[Megha Sharma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49520/
---

(Updated July 7, 2016, 11:45 p.m.)


Review request for mesos and Jiang Yan Xu.


Bugs: MESOS-5752
https://issues.apache.org/jira/browse/MESOS-5752


Repository: mesos


Description
---

In GarbageCollectorIntegrationTest.BusyMountPoint there is
a race between the task creating a file and the test looking for it
which sometimes leads to assertion failure on the existence of the
file.


Diffs
-

  src/tests/gc_tests.cpp d6335dc711f0697a0728a912e713cf1ed783c146 

Diff: https://reviews.apache.org/r/49520/diff/


Testing (updated)
---

make check on mac os and linux


Thanks,

Megha Sharma

Re: Review Request 49520: Fixed the flaky BusyMountPoint test.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49520/#review141219
---



Patch looks great!

Reviews applied: [49520]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On July 7, 2016, 9:20 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49520/
> ---
> 
> (Updated July 7, 2016, 9:20 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5752
> https://issues.apache.org/jira/browse/MESOS-5752
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In GarbageCollectorIntegrationTest.BusyMountPoint there is
> a race between the task creating a file and the test looking for it
> which sometimes leads to assertion failure on the existence of the
> file.
> 
> 
> Diffs
> -
> 
>   src/tests/gc_tests.cpp d6335dc711f0697a0728a912e713cf1ed783c146 
> 
> Diff: https://reviews.apache.org/r/49520/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 49520: Fixed the flaky BusyMountPoint test.

[Megha Sharma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49520/
---

(Updated July 7, 2016, 9:20 p.m.)


Review request for mesos and Jiang Yan Xu.


Bugs: MESOS-5752
https://issues.apache.org/jira/browse/MESOS-5752


Repository: mesos


Description (updated)
---

In GarbageCollectorIntegrationTest.BusyMountPoint there is
a race between the task creating a file and the test looking for it
which sometimes leads to assertion failure on the existence of the
file.


Diffs (updated)
-

  src/tests/gc_tests.cpp d6335dc711f0697a0728a912e713cf1ed783c146 

Diff: https://reviews.apache.org/r/49520/diff/


Testing
---


Thanks,

Megha Sharma

Re: Review Request 49520: Fixed the flaky BusyMountPoint test.

[Jie Yu]

> On July 3, 2016, 5:32 p.m., Jie Yu wrote:
> > src/tests/gc_tests.cpp, line 963
> > 
> >
> > Is it possible that mountPoint is not set but test failed? Are we 
> > leaking mounts in that case?
> > 
> > I would suggest that we override `CreateSlaveFlags` and save slave's 
> > work_dir in the test fixture.
> > 
> > During teardown, you can call fs::unmountAll(workDir) to cleanup the 
> > mounts.
> 
> Jiang Yan Xu wrote:
> Hey Jie I didn't realize this, but it seems like Environment already 
> takes care of this (you wrote this part): 
> https://github.com/apache/mesos/blob/ada24cb6b29cc74e30ce31550459a6b14d409c96/src/tests/environment.cpp#L847-L861
> 
> And `umount -l` does cover this case here: so we don't even need a 
> separate test fixture anymore.
> 
> Of course we should verify this.

Oh, right! Yeah, we probably can get rid of the test fixture! Can you test it?


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49520/#review140560
---


On July 1, 2016, 6:22 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49520/
> ---
> 
> (Updated July 1, 2016, 6:22 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5752
> https://issues.apache.org/jira/browse/MESOS-5752
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In ROOT_GarbageCollectorUndeletableFilesTest.BusyMountPoint there is
> a race between the task creating a file and the test looking for it
> which sometimes leads to assertion failure on the existence of the
> file.
> 
> 
> Diffs
> -
> 
>   src/tests/gc_tests.cpp 2e23f04ea908aadcefb21e11203b95b94ec3c60b 
> 
> Diff: https://reviews.apache.org/r/49520/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 49520: Fixed the flaky BusyMountPoint test.

[Jie Yu]

> On July 3, 2016, 5:32 p.m., Jie Yu wrote:
> >

Could you please run the test to make sure there's no mount leak?


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49520/#review140560
---


On July 1, 2016, 6:22 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49520/
> ---
> 
> (Updated July 1, 2016, 6:22 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5752
> https://issues.apache.org/jira/browse/MESOS-5752
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In ROOT_GarbageCollectorUndeletableFilesTest.BusyMountPoint there is
> a race between the task creating a file and the test looking for it
> which sometimes leads to assertion failure on the existence of the
> file.
> 
> 
> Diffs
> -
> 
>   src/tests/gc_tests.cpp 2e23f04ea908aadcefb21e11203b95b94ec3c60b 
> 
> Diff: https://reviews.apache.org/r/49520/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 49520: Fixed the flaky BusyMountPoint test.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49520/#review140560
---




src/tests/gc_tests.cpp (line 919)


new line above



src/tests/gc_tests.cpp (line 932)


I prefer using `_mountPoint` than `mountPoint_`. For those guys used to 
google style, a suffix `_` indicates that it's a member field.



src/tests/gc_tests.cpp (line 955)


Ditto on using `_sandbox`



src/tests/gc_tests.cpp (line 963)


Is it possible that mountPoint is not set but test failed? Are we leaking 
mounts in that case?

I would suggest that we override `CreateSlaveFlags` and save slave's 
work_dir in the test fixture.

During teardown, you can call fs::unmountAll(workDir) to cleanup the mounts.



src/tests/gc_tests.cpp (line 968)


Please add `using process::Timeout` in the beginning of this file and use 
`Timeout` here.


- Jie Yu


On July 1, 2016, 6:22 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49520/
> ---
> 
> (Updated July 1, 2016, 6:22 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5752
> https://issues.apache.org/jira/browse/MESOS-5752
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In ROOT_GarbageCollectorUndeletableFilesTest.BusyMountPoint there is
> a race between the task creating a file and the test looking for it
> which sometimes leads to assertion failure on the existence of the
> file.
> 
> 
> Diffs
> -
> 
>   src/tests/gc_tests.cpp 2e23f04ea908aadcefb21e11203b95b94ec3c60b 
> 
> Diff: https://reviews.apache.org/r/49520/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 49520: Fixed the flaky BusyMountPoint test.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49520/#review140452
---



Patch looks great!

Reviews applied: [49520]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On July 1, 2016, 6:22 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49520/
> ---
> 
> (Updated July 1, 2016, 6:22 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5752
> https://issues.apache.org/jira/browse/MESOS-5752
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In ROOT_GarbageCollectorUndeletableFilesTest.BusyMountPoint there is
> a race between the task creating a file and the test looking for it
> which sometimes leads to assertion failure on the existence of the
> file.
> 
> 
> Diffs
> -
> 
>   src/tests/gc_tests.cpp 2e23f04ea908aadcefb21e11203b95b94ec3c60b 
> 
> Diff: https://reviews.apache.org/r/49520/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 49520: Fixed the flaky BusyMountPoint test.

[Megha Sharma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49520/
---

(Updated July 1, 2016, 6:22 p.m.)


Review request for mesos and Jiang Yan Xu.


Bugs: MESOS-5752
https://issues.apache.org/jira/browse/MESOS-5752


Repository: mesos


Description
---

In ROOT_GarbageCollectorUndeletableFilesTest.BusyMountPoint there is
a race between the task creating a file and the test looking for it
which sometimes leads to assertion failure on the existence of the
file.


Diffs (updated)
-

  src/tests/gc_tests.cpp 2e23f04ea908aadcefb21e11203b95b94ec3c60b 

Diff: https://reviews.apache.org/r/49520/diff/


Testing
---


Thanks,

Megha Sharma

Re: Review Request 49520: Fixed the flaky BusyMountPoint test.

[Megha Sharma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49520/
---

(Updated July 1, 2016, 6:06 p.m.)


Review request for mesos and Jiang Yan Xu.


Bugs: MESOS-5752
https://issues.apache.org/jira/browse/MESOS-5752


Repository: mesos


Description
---

In ROOT_GarbageCollectorUndeletableFilesTest.BusyMountPoint there is
a race between the task creating a file and the test looking for it
which sometimes leads to assertion failure on the existence of the
file.


Diffs (updated)
-

  src/tests/gc_tests.cpp 2e23f04ea908aadcefb21e11203b95b94ec3c60b 

Diff: https://reviews.apache.org/r/49520/diff/


Testing
---


Thanks,

Megha Sharma

Re: Review Request 49520: Fixed the flaky BusyMountPoint test.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49520/#review140386
---



Could you verify on testing done section that it's been run on both macOS and 
Linux?

We missed something last time :)
https://github.com/apache/mesos/commit/6619503c0b6f9c83a4af24c5d3556d4ee68d0e65


src/tests/gc_tests.cpp (lines 965 - 968)


I think we should timebox this wait.

```
// Wait for the task to create these paths.
Timeout timeout = Timeout::in(Seconds(15));
while (!os::exists(path::join(sandbox, mountPoint_) ||
   !os::exists(path::join(sandbox, regularFile) ||
   !timeout.expired()) {
  os::sleep(Milliseconds(10));
}

ASSERT_TRUE(os::exists(path::join(sandbox, mountPoint_)));
ASSERT_TRUE(os::exists(path::join(sandbox, regularFile)));
```

Because we can exist the loop due to timeout, we need to ASSERT here.



src/tests/gc_tests.cpp (lines 971 - 972)


Use `ASSERT_EQ` here for both. (Sorry I commented on this in 
rmdir_tests.cpp earlier but overlooked it in gc_tests.cpp)

Theses are assertions because we don't want the test to proceed if these 
aren't met. They are part of the setup.


- Jiang Yan Xu


On July 1, 2016, 9:39 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49520/
> ---
> 
> (Updated July 1, 2016, 9:39 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5752
> https://issues.apache.org/jira/browse/MESOS-5752
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In ROOT_GarbageCollectorUndeletableFilesTest.BusyMountPoint there is
> a race between the task creating a file and the test looking for it
> which sometimes leads to assertion failure on the existence of the
> file.
> 
> 
> Diffs
> -
> 
>   src/tests/gc_tests.cpp 2e23f04ea908aadcefb21e11203b95b94ec3c60b 
> 
> Diff: https://reviews.apache.org/r/49520/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Review Request 49520: Fixed the flaky BusyMountPoint test.

[Megha Sharma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49520/
---

Review request for mesos and Jiang Yan Xu.


Bugs: MESOS-5752
https://issues.apache.org/jira/browse/MESOS-5752


Repository: mesos


Description
---

In ROOT_GarbageCollectorUndeletableFilesTest.BusyMountPoint there is
a race between the task creating a file and the test looking for it
which sometimes leads to assertion failure on the existence of the
file.


Diffs
-

  src/tests/gc_tests.cpp 2e23f04ea908aadcefb21e11203b95b94ec3c60b 

Diff: https://reviews.apache.org/r/49520/diff/


Testing
---


Thanks,

Megha Sharma