Re: Review Request 65122: Fixed race condition in io switchboard for ATTACH_CONTAINER_INPUT call.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Jan. 17, 2018, 2:14 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65122/
> ---
> 
> (Updated Jan. 17, 2018, 2:14 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Kevin Klues.
> 
> 
> Bugs: MESOS-7742
> https://issues.apache.org/jira/browse/MESOS-7742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, io switchboard terminated itself after io redirect had
> been finished and before http response had been sent to the agent for
> `ATTACH_CONTAINER_INPUT` request. This patch adds a check that an
> input connection exists in io redirect completion callback to postpone
> termination until `ATTACH_CONTAINER_INPUT` is completely processed.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 4ef640616d69b91c225206444737a40f9571da55 
> 
> 
> Diff: https://reviews.apache.org/r/65122/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 65122: Fixed race condition in io switchboard for ATTACH_CONTAINER_INPUT call.

[Alexander Rukletsov]

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



Follow up review: https://reviews.apache.org/r/65190/

- Alexander Rukletsov


On Jan. 17, 2018, 2:14 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65122/
> ---
> 
> (Updated Jan. 17, 2018, 2:14 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Kevin Klues.
> 
> 
> Bugs: MESOS-7742
> https://issues.apache.org/jira/browse/MESOS-7742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, io switchboard terminated itself after io redirect had
> been finished and before http response had been sent to the agent for
> `ATTACH_CONTAINER_INPUT` request. This patch adds a check that an
> input connection exists in io redirect completion callback to postpone
> termination until `ATTACH_CONTAINER_INPUT` is completely processed.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 4ef640616d69b91c225206444737a40f9571da55 
> 
> 
> Diff: https://reviews.apache.org/r/65122/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 65122: Fixed race condition in io switchboard for ATTACH_CONTAINER_INPUT call.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65122 was successfully built and tested.

Reviews applied: `['65122']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65122

- Mesos Reviewbot Windows


On Jan. 17, 2018, 2:14 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65122/
> ---
> 
> (Updated Jan. 17, 2018, 2:14 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Kevin Klues.
> 
> 
> Bugs: MESOS-7742
> https://issues.apache.org/jira/browse/MESOS-7742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, io switchboard terminated itself after io redirect had
> been finished and before http response had been sent to the agent for
> `ATTACH_CONTAINER_INPUT` request. This patch adds a check that an
> input connection exists in io redirect completion callback to postpone
> termination until `ATTACH_CONTAINER_INPUT` is completely processed.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 4ef640616d69b91c225206444737a40f9571da55 
> 
> 
> Diff: https://reviews.apache.org/r/65122/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 65122: Fixed race condition in io switchboard for ATTACH_CONTAINER_INPUT call.

[Andrei Budnik]

> On Jan. 16, 2018, 7:21 p.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp
> > Lines 1223 (patched)
> > 
> >
> > If I understand this correctly, this is solving for the case where we 
> > currently have input connected but our output IO has completed. In such a 
> > case, we don't want to terminate immediately, but instead hold off  
> > terminating until the input connection has been closed.
> > 
> > I think the code you have is correct, but we should have a more 
> > detailed comment here about when the process will be terminated in the case 
> > where `inputConnected` is true.
> > 
> > Likewise on line 1673, we should have a more detailed comment as to why 
> > we need to handle this case here.

Updated comments.


> On Jan. 16, 2018, 7:21 p.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp
> > Lines 1673 (patched)
> > 
> >
> > We shoudl reverse the order of these to fist check `redirectFinished` 
> > before checking `failure.isSome()`. This will make writing the comment I 
> > mentioned in my other review flow more easily.

Updated.


- Andrei


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


On Jan. 17, 2018, 2:14 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65122/
> ---
> 
> (Updated Jan. 17, 2018, 2:14 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Kevin Klues.
> 
> 
> Bugs: MESOS-7742
> https://issues.apache.org/jira/browse/MESOS-7742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, io switchboard terminated itself after io redirect had
> been finished and before http response had been sent to the agent for
> `ATTACH_CONTAINER_INPUT` request. This patch adds a check that an
> input connection exists in io redirect completion callback to postpone
> termination until `ATTACH_CONTAINER_INPUT` is completely processed.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 4ef640616d69b91c225206444737a40f9571da55 
> 
> 
> Diff: https://reviews.apache.org/r/65122/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 65122: Fixed race condition in io switchboard for ATTACH_CONTAINER_INPUT call.

[Andrei Budnik]

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

(Updated Jan. 17, 2018, 2:14 p.m.)


Review request for mesos, Alexander Rukletsov, Greg Mann, and Kevin Klues.


Changes
---

Updated comments.


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


Repository: mesos


Description (updated)
---

Previously, io switchboard terminated itself after io redirect is
finished and before http response was sent to the agent for
`ATTACH_CONTAINER_INPUT` request. This patch adds a check that an
input connection exists in io redirect completion callback to postpone
termination until `ATTACH_CONTAINER_INPUT` is completely processed.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
4ef640616d69b91c225206444737a40f9571da55 


Diff: https://reviews.apache.org/r/65122/diff/2/

Changes: https://reviews.apache.org/r/65122/diff/1-2/


Testing
---

sudo make check (Fedora 25)
internal CI


Thanks,

Andrei Budnik

Re: Review Request 65122: Fixed race condition in io switchboard for ATTACH_CONTAINER_INPUT call.

[Kevin Klues]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/io/switchboard.cpp
Lines 1223 (patched)


If I understand this correctly, this is solving for the case where we 
currently have input connected but our output IO has completed. In such a case, 
we don't want to terminate immediately, but instead hold off  terminating until 
the input connection has been closed.

I think the code you have is correct, but we should have a more detailed 
comment here about when the process will be terminated in the case where 
`inputConnected` is true.

Likewise on line 1673, we should have a more detailed comment as to why we 
need to handle this case here.



src/slave/containerizer/mesos/io/switchboard.cpp
Lines 1673 (patched)


We shoudl reverse the order of these to fist check `redirectFinished` 
before checking `failure.isSome()`. This will make writing the comment I 
mentioned in my other review flow more easily.


- Kevin Klues


On Jan. 15, 2018, 4:42 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65122/
> ---
> 
> (Updated Jan. 15, 2018, 4:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Kevin Klues.
> 
> 
> Bugs: MESOS-7742
> https://issues.apache.org/jira/browse/MESOS-7742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, io switchboard terminated itself after io redirect had
> been finished and before http response had been sent to the agent for
> `ATTACH_CONTAINER_INPUT` request. This patch adds a check that an
> input connection exists in io redirect completion callback to postpone
> termination until `ATTACH_CONTAINER_INPUT` is completely processed.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 89dd4988c7132d3c7aeacecad7a76961b0ec5a2c 
> 
> 
> Diff: https://reviews.apache.org/r/65122/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 65122: Fixed race condition in io switchboard for ATTACH_CONTAINER_INPUT call.

[Andrei Budnik]

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

(Updated Jan. 15, 2018, 4:42 p.m.)


Review request for mesos, Alexander Rukletsov, Greg Mann, and Kevin Klues.


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


Repository: mesos


Description (updated)
---

Previously, io switchboard terminated itself after io redirect had
been finished and before http response had been sent to the agent for
`ATTACH_CONTAINER_INPUT` request. This patch adds a check that an
input connection exists in io redirect completion callback to postpone
termination until `ATTACH_CONTAINER_INPUT` is completely processed.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
89dd4988c7132d3c7aeacecad7a76961b0ec5a2c 


Diff: https://reviews.apache.org/r/65122/diff/1/


Testing
---

sudo make check (Fedora 25)
internal CI


Thanks,

Andrei Budnik

Re: Review Request 65122: Fixed race condition in io switchboard for ATTACH_CONTAINER_INPUT call.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65122 was successfully built and tested.

Reviews applied: `['65122']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65122

- Mesos Reviewbot Windows


On Jan. 12, 2018, 12:23 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65122/
> ---
> 
> (Updated Jan. 12, 2018, 12:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Kevin Klues.
> 
> 
> Bugs: MESOS-7742
> https://issues.apache.org/jira/browse/MESOS-7742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, io switchboard terminated itself after io redirect is
> finished and before http response was sent to the agent for
> `ATTACH_CONTAINER_INPUT` request. This patch adds a check that an
> input connection exists in io redirect completion callback to postpone
> termination until `ATTACH_CONTAINER_INPUT` is completely processed.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 89dd4988c7132d3c7aeacecad7a76961b0ec5a2c 
> 
> 
> Diff: https://reviews.apache.org/r/65122/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 65122: Fixed race condition in io switchboard for ATTACH_CONTAINER_INPUT call.

[Mesos Reviewbot Windows]

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



FAIL: mesos-java failed to build.

Reviews applied: `['65122']`

Failed command: `cmake.exe --build . --target mesos-java`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65122

Relevant logs:

- 
[mesos-java-build-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65122/logs/mesos-java-build-cmake-stdout.log):

```
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3501):
 warning C4996: 'sprintf': This function or variable may be unsafe. Consider 
using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3479):
 warning C4101: 'addrstr': unreferenced local variable 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(512):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(543):
 warning C4996: 'strcpy': This function or variable may be unsafe. Consider 
using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(548):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(569):
 warning C4996: 'strcpy': This function or variable may be unsafe. Consider 
using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]


"D:\DCOS\mesos\src\java\mesos-java.vcxproj" (default target) (1) ->
"D:\DCOS\mesos\3rdparty\libprocess\src\process.vcxproj" (default target) (19) ->
(ClCompile target) -> 
  D:\DCOS\mesos\mesos\3rdparty\libprocess\src\poll_socket.cpp(279): fatal error 
C1088: Cannot flush 

Review Request 65122: Fixed race condition in io switchboard for ATTACH_CONTAINER_INPUT call.

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov, Greg Mann, and Kevin Klues.


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


Repository: mesos


Description
---

Previously, io switchboard terminated itself after io redirect is
finished and before http response was sent to the agent for
`ATTACH_CONTAINER_INPUT` request. This patch adds a check that an
input connection exists in io redirect completion callback to postpone
termination until `ATTACH_CONTAINER_INPUT` is completely processed.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
89dd4988c7132d3c7aeacecad7a76961b0ec5a2c 


Diff: https://reviews.apache.org/r/65122/diff/1/


Testing
---

sudo make check (Fedora 25)
internal CI


Thanks,

Andrei Budnik