Re: Review Request 69727: Compared the device number of namespace handle instead of /proc.

2019-01-11 Thread Deepak Goel

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


Ship it!




Ship It!

- Deepak Goel


On Jan. 11, 2019, 11:21 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69727/
> ---
> 
> (Updated Jan. 11, 2019, 11:21 p.m.)
> 
> 
> Review request for mesos, Deepak Goel and Gilbert Song.
> 
> 
> Bugs: MESOS-9518
> https://issues.apache.org/jira/browse/MESOS-9518
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In recent versions of kernels, the device number of '/proc//ns/net'
> is different than that of '/proc'. It shows up as "nsfs" instead of
> "proc" like the old kernels. For instance:
> 
> Newer kernel:
> 
> ```
> $ uname -nr
> ubuntu-xenial 4.4.0-83-generic
> $ stat -L -c %d /proc/self/ns/net
> 3
> $ stat -L -c %d /proc
> 4
> ```
> 
> Older kernel:
> 
> ```
> $ uname -nr
> core-dev 3.10.0-693.5.2.el7.x86_64
> $ stat -L -c %d /proc/self/ns/net
> 3
> $ stat -L -c %d /proc
> 3
> ```
> 
> As a result, we should compare the device number directly against the
> namespace handle, instead of `/proc`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> a1130a58553fb67bd8c212498b98978f116d7b0c 
> 
> 
> Diff: https://reviews.apache.org/r/69727/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 69715: Fixed the CNI_NETNS handling in port mapper CNI plugin.

2019-01-11 Thread Deepak Goel

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


Ship it!




Ship It!

- Deepak Goel


On Jan. 11, 2019, 6:10 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69715/
> ---
> 
> (Updated Jan. 11, 2019, 6:10 p.m.)
> 
> 
> Review request for mesos, Deepak Goel, Gilbert Song, and Qian Zhang.
> 
> 
> Bugs: MESOS-9518
> https://issues.apache.org/jira/browse/MESOS-9518
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> According CNI spec, it is possible that the container runtime does not
> set CNI_NETNS environment variable when it is not available. This is
> possible in scenarios like a host reboot. In that case, the CNI plugin
> should do best effort cleanup, instead of failing.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  25f49f4b90ec6d0d55fc306b6ab324ba5b4e7403 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  4e784ffb4ac29861c888fdbed4fcf9902bf4182a 
> 
> 
> Diff: https://reviews.apache.org/r/69715/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 69712: Added a CNI reboot test.

2019-01-11 Thread Deepak Goel

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


Ship it!




Ship It!

- Deepak Goel


On Jan. 11, 2019, 6:49 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69712/
> ---
> 
> (Updated Jan. 11, 2019, 6:49 p.m.)
> 
> 
> Review request for mesos, Deepak Goel, Gilbert Song, James Peach, and Qian 
> Zhang.
> 
> 
> Bugs: MESOS-9518
> https://issues.apache.org/jira/browse/MESOS-9518
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that CNI DEL is properly called even after the agent
> host is rebooted, assuming `--network_cni_root_dir_persist` flag is set
> to true.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> eb20e637ecbe1b39e2dbb274c5198828f2fdf62f 
> 
> 
> Diff: https://reviews.apache.org/r/69712/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 69706: Kept `CNI_NETNS` unset in detach if network namespace is gone.

2019-01-10 Thread Deepak Goel

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


Ship it!




Ship It!

- Deepak Goel


On Jan. 10, 2019, 8:44 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69706/
> ---
> 
> (Updated Jan. 10, 2019, 8:44 p.m.)
> 
> 
> Review request for mesos, Deepak Goel, Gilbert Song, James Peach, and Qian 
> Zhang.
> 
> 
> Bugs: MESOS-9518
> https://issues.apache.org/jira/browse/MESOS-9518
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We introduced a new agent flag in MESOS-9492 so that CNI configs can be
> persisted across reboot. This is for some CNI plugins to be able to
> cleanup IP allocated to the containers after a sudden reboot of the host
> (not all CNI plugins need this).
> 
> It's important to unset `CNI_NETNS` environment variable after reboot
> when invoking CNI plugin "DEL" command so that it conforms to the spec.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> cc23428d27d40be8c4ff1476e6e984c7d12760c4 
> 
> 
> Diff: https://reviews.apache.org/r/69706/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Review Request 69592: Updated upgrades.md.

2018-12-18 Thread Deepak Goel

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

Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

Updated upgrades.md with the information about the new flag
`--network_cni_root_dir_persist`. (MESOS-9492)


Diffs
-

  docs/upgrades.md f8166be75924f4f72ac291dfe7df92bd32f79f06 


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


Testing
---


Thanks,

Deepak Goel



Re: Review Request 69590: Moves CNI root directory to a persistent location.

2018-12-18 Thread Deepak Goel


> On Dec. 19, 2018, 12:48 a.m., Till Toenshoff wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
> > Lines 1307-1308 (original)
> > <https://reviews.apache.org/r/69590/diff/1/?file=2114813#file2114813line1309>
> >
> > Are we assuming that this location was never meant for users?
> 
> Deepak Goel wrote:
> Didn't understand your comment? Which location you are talking about?
> 
> Chun-Hung Hsiao wrote:
> I guess Till was asking why the log of `networkConfigJSON` is hidden from 
> user-facing logs (`LOG(INFO)`), unlike the other log lines.

Got it. networkConfigJSON might get big and could quickly fill up the log 
space. Hence, kept it at VLOG level just for debugging purpose.


- Deepak


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


On Dec. 19, 2018, 4:52 a.m., Deepak Goel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69590/
> ---
> 
> (Updated Dec. 19, 2018, 4:52 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Sergey 
> Urbanovich.
> 
> 
> Bugs: MESOS-9492
> https://issues.apache.org/jira/browse/MESOS-9492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch moves the CNI root directory to mesos working directory
> so that the network related information persist across reboot which
> will allow cni isolator to do proper cleanup post reboot
> 
> 
> Diffs
> -
> 
>   docs/cni.md 4a0e0bf57c6ff8d2bcc9fdf30d20672324da224c 
>   docs/configuration/agent.md 7a8df6852dc2af174a6c5a552dca88fa1b1c29f3 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> f19edce0671686bc2dccf5dc6d3abc40b925852e 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
> 3b06fb101d03b32a3a148e2b79502413a7268d1e 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
> c1cb4f7d667159abbf87219cde30528a4a7b5283 
>   src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
>   src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 5473737cdc71a3179c22be0d4db2060a6ef53d24 
> 
> 
> Diff: https://reviews.apache.org/r/69590/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Goel
> 
>



Re: Review Request 69590: Moves CNI root directory to a persistent location.

2018-12-18 Thread Deepak Goel

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

(Updated Dec. 19, 2018, 4:52 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Sergey 
Urbanovich.


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


Repository: mesos


Description
---

This patch moves the CNI root directory to mesos working directory
so that the network related information persist across reboot which
will allow cni isolator to do proper cleanup post reboot


Diffs (updated)
-

  docs/cni.md 4a0e0bf57c6ff8d2bcc9fdf30d20672324da224c 
  docs/configuration/agent.md 7a8df6852dc2af174a6c5a552dca88fa1b1c29f3 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
f19edce0671686bc2dccf5dc6d3abc40b925852e 
  src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
3b06fb101d03b32a3a148e2b79502413a7268d1e 
  src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
c1cb4f7d667159abbf87219cde30528a4a7b5283 
  src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
  src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 
  src/tests/containerizer/cni_isolator_tests.cpp 
5473737cdc71a3179c22be0d4db2060a6ef53d24 


Diff: https://reviews.apache.org/r/69590/diff/4/

Changes: https://reviews.apache.org/r/69590/diff/3-4/


Testing
---


Thanks,

Deepak Goel



Re: Review Request 69590: Moves CNI root directory to a persistent location.

2018-12-18 Thread Deepak Goel

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

(Updated Dec. 19, 2018, 3:25 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Sergey 
Urbanovich.


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


Repository: mesos


Description
---

This patch moves the CNI root directory to mesos working directory
so that the network related information persist across reboot which
will allow cni isolator to do proper cleanup post reboot


Diffs (updated)
-

  docs/cni.md 4a0e0bf57c6ff8d2bcc9fdf30d20672324da224c 
  docs/configuration/agent.md 7a8df6852dc2af174a6c5a552dca88fa1b1c29f3 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
f19edce0671686bc2dccf5dc6d3abc40b925852e 
  src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
3b06fb101d03b32a3a148e2b79502413a7268d1e 
  src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
c1cb4f7d667159abbf87219cde30528a4a7b5283 
  src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
  src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 
  src/tests/containerizer/cni_isolator_tests.cpp 
5473737cdc71a3179c22be0d4db2060a6ef53d24 


Diff: https://reviews.apache.org/r/69590/diff/3/

Changes: https://reviews.apache.org/r/69590/diff/2-3/


Testing
---


Thanks,

Deepak Goel



Re: Review Request 69590: Moves CNI root directory to a persistent location.

2018-12-18 Thread Deepak Goel


> On Dec. 19, 2018, 12:48 a.m., Till Toenshoff wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
> > Lines 1307-1308 (original)
> > <https://reviews.apache.org/r/69590/diff/1/?file=2114813#file2114813line1309>
> >
> > Are we assuming that this location was never meant for users?

Didn't understand your comment? Which location you are talking about?


- Deepak


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


On Dec. 19, 2018, 2:03 a.m., Deepak Goel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69590/
> ---
> 
> (Updated Dec. 19, 2018, 2:03 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Sergey 
> Urbanovich.
> 
> 
> Bugs: MESOS-9492
> https://issues.apache.org/jira/browse/MESOS-9492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch moves the CNI root directory to mesos working directory
> so that the network related information persist across reboot which
> will allow cni isolator to do proper cleanup post reboot
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> f19edce0671686bc2dccf5dc6d3abc40b925852e 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
> 3b06fb101d03b32a3a148e2b79502413a7268d1e 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
> c1cb4f7d667159abbf87219cde30528a4a7b5283 
>   src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
>   src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 5473737cdc71a3179c22be0d4db2060a6ef53d24 
> 
> 
> Diff: https://reviews.apache.org/r/69590/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Goel
> 
>



Re: Review Request 69590: Moves CNI root directory to a persistent location.

2018-12-18 Thread Deepak Goel

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

(Updated Dec. 19, 2018, 2:03 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Sergey 
Urbanovich.


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


Repository: mesos


Description
---

This patch moves the CNI root directory to mesos working directory
so that the network related information persist across reboot which
will allow cni isolator to do proper cleanup post reboot


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
f19edce0671686bc2dccf5dc6d3abc40b925852e 
  src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
3b06fb101d03b32a3a148e2b79502413a7268d1e 
  src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
c1cb4f7d667159abbf87219cde30528a4a7b5283 
  src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
  src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 
  src/tests/containerizer/cni_isolator_tests.cpp 
5473737cdc71a3179c22be0d4db2060a6ef53d24 


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

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


Testing
---


Thanks,

Deepak Goel



Review Request 69590: Moves CNI root directory to a persistent location.

2018-12-18 Thread Deepak Goel

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

Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Sergey 
Urbanovich.


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


Repository: mesos


Description
---

This patch moves the CNI root directory to mesos working directory
so that the network related information persist across reboot which
will allow cni isolator to do proper cleanup post reboot


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
f19edce0671686bc2dccf5dc6d3abc40b925852e 
  src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
3b06fb101d03b32a3a148e2b79502413a7268d1e 
  src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
c1cb4f7d667159abbf87219cde30528a4a7b5283 
  src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
  src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 
  src/tests/containerizer/cni_isolator_tests.cpp 
5473737cdc71a3179c22be0d4db2060a6ef53d24 


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


Testing
---


Thanks,

Deepak Goel



Re: Review Request 69538: Added UCR bridge network for Mesos Mini.

2018-12-10 Thread Deepak Goel

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


Ship it!




Ship It!

- Deepak Goel


On Dec. 10, 2018, 6:11 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69538/
> ---
> 
> (Updated Dec. 10, 2018, 6:11 a.m.)
> 
> 
> Review request for mesos, Deepak Goel and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the UCR bridge network support for Mesos Mini using CNI
> bridge plugin and Mesos port mapper CNI plugin.
> 
> 
> Diffs
> -
> 
>   support/mesos-mini/Dockerfile f8af422d8b8b0be4c462d40bce2d1337ab8b6e95 
>   support/mesos-mini/mesos/agent_environment 
> 48767281987c2e70b3a64f28c5eac139435c17f6 
>   support/mesos-mini/mesos/master_environment 
> 0382558744cb693c862246856b8c16ff42a95d19 
>   support/mesos-mini/mesos/ucr-default-bridge.json PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69538/diff/1/
> 
> 
> Testing
> ---
> 
> Tested using this Marathon app
> ```json
> {
>   "id": "/test",
>   "instances": 1,
>   "container": {
> "type": "MESOS",
> "volumes": [],
> "portMappings": [],
> "docker": {
>   "image": "alpine"
> }
>   },
>   "cpus": 0.1,
>   "mem": 128,
>   "requirePorts": false,
>   "networks": [
> {
>   "mode": "container/bridge"
> }
>   ],
>   "healthChecks": [],
>   "fetch": [],
>   "constraints": [],
>   "cmd": "sleep 10"
> }
> ```
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 68335: Added a CNI test to verify destroy while preparing.

2018-08-13 Thread Deepak Goel

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


Ship it!




Ship It!

- Deepak Goel


On Aug. 13, 2018, 11 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68335/
> ---
> 
> (Updated Aug. 13, 2018, 11 p.m.)
> 
> 
> Review request for mesos, Deepak Goel, Qian Zhang, and Sergey Urbanovich.
> 
> 
> Bugs: MESOS-9142
> https://issues.apache.org/jira/browse/MESOS-9142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test is used to catch the regression described in MESOS-9142.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 7711d463c8ed92e2580c56e88d7f372c6dfaeb2b 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> cb22e73b4215b5b0a49ac610e5f657b73d38963b 
> 
> 
> Diff: https://reviews.apache.org/r/68335/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 68333: Made CNI isolator cleanup more robust.

2018-08-13 Thread Deepak Goel

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


Ship it!




Ship It!

- Deepak Goel


On Aug. 13, 2018, 10:59 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68333/
> ---
> 
> (Updated Aug. 13, 2018, 10:59 p.m.)
> 
> 
> Review request for mesos, Deepak Goel, Qian Zhang, and Sergey Urbanovich.
> 
> 
> Bugs: MESOS-9142
> https://issues.apache.org/jira/browse/MESOS-9142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the container is destroyed while in isolator preparing state, the
> cleanup might fail due to missing files or directories. This patch makes
> the cleanup path in CNI isolator more robust so that the cleanup does
> not fail in those scenarios.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> f46c962d8f044092aaa113fafc536c6b25bab45c 
> 
> 
> Diff: https://reviews.apache.org/r/68333/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

2018-08-06 Thread Deepak Goel

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


Ship it!




Ship It!

- Deepak Goel


On Aug. 6, 2018, 4:52 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68158/
> ---
> 
> (Updated Aug. 6, 2018, 4:52 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.
> 
> 
> Bugs: MESOS-9127
> https://issues.apache.org/jira/browse/MESOS-9127
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is possible that the port mapping cleanup command will cause iptables
> to deadlock if there are a lot of entires in the iptables, because the
> `sed` won't process the next line while executing 'iptables -w -t nat -D
> ...'. But the executing of 'iptables -w -t nat -D ...' might get stuck
> if the first command 'iptables -w -t nat -S %s' didn't finish (because
> the xtables lock is not released). The first command might not finish if
> it has a lot of output, filling the pipe that `sed` hasn't had a chance
> to process yet. See more details in MESOS-9127.
> 
> This patch fixed the issue by writing the commands to a file and then
> executing them.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  f1a3d263b7baa3ccbf270426745022d42fcc66ed 
> 
> 
> Diff: https://reviews.apache.org/r/68158/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> ```
> [   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
> ```
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 62017: Allows port mapper plugin to have optional args.

2017-09-05 Thread Deepak Goel


> On Sept. 6, 2017, 2:09 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
> > Lines 149 (patched)
> > <https://reviews.apache.org/r/62017/diff/4/?file=1816054#file1816054line149>
> >
> > Why are we setting this empty `args` here instead of setting up a 
> > `NetworkInfo networkInfo` object and passing it down into the constructor 
> > of the port-mapper?
> > 
> > the `port-mapper` really doesn't care about `args` it cares about 
> > `NetworkInfo`. This code seems to conflate the whole need for not setting 
> > up `args` here.
> > 
> > Also setting up `mesos.values` seems to tie the port-mapper plugin back 
> > to Mesos semantics of setting up the args which seems like a bit of a hack.

Yeah, you are right. Initially, I thought of doing that but then it led to 
making exceptions to various checks that happens before we reach to the 
constructor and the code looked even more hacky and error prone. This patch 
achieves the same goal with fewer lines of code change


- Deepak


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


On Sept. 6, 2017, 12:25 a.m., Deepak Goel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> ---
> 
> (Updated Sept. 6, 2017, 12:25 a.m.)
> 
> 
> Review request for mesos and Avinash sridharan.
> 
> 
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  43cf3e44a55c56dc8195c9cd05f6edd8bf13d448 
> 
> 
> Diff: https://reviews.apache.org/r/62017/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Goel
> 
>



Re: Review Request 62017: Allows port mapper plugin to have optional args.

2017-09-05 Thread Deepak Goel

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

(Updated Sept. 6, 2017, 12:25 a.m.)


Review request for mesos and Avinash sridharan.


Bugs: mesos-7923
https://issues.apache.org/jira/browse/mesos-7923


Repository: mesos


Description
---

Mesos port mapper cni plugin is a wrapper around bridge plugin
to add port mapping functionality to bridge plugin. However, in
certain cases the network creator doesn't need port mapping
functionality and just want to access bridge plugin. In this case,
the creator may not supply `args` in cni config which will makes
mesos port mapper plugin to fail. This patch makes `args` in cni
config optional for mesos port mapper plugin


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448 


Diff: https://reviews.apache.org/r/62017/diff/4/

Changes: https://reviews.apache.org/r/62017/diff/3-4/


Testing
---


Thanks,

Deepak Goel



Re: Review Request 62017: Allows port mapper plugin to have optional args.

2017-09-05 Thread Deepak Goel

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

(Updated Sept. 5, 2017, 9:47 p.m.)


Review request for mesos and Avinash sridharan.


Bugs: mesos-7923
https://issues.apache.org/jira/browse/mesos-7923


Repository: mesos


Description
---

Mesos port mapper cni plugin is a wrapper around bridge plugin
to add port mapping functionality to bridge plugin. However, in
certain cases the network creator doesn't need port mapping
functionality and just want to access bridge plugin. In this case,
the creator may not supply `args` in cni config which will makes
mesos port mapper plugin to fail. This patch makes `args` in cni
config optional for mesos port mapper plugin


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448 


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

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


Testing
---


Thanks,

Deepak Goel



Review Request 62017: Allows port mapper plugin to have optional args.

2017-08-31 Thread Deepak Goel

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

Review request for mesos and Avinash sridharan.


Bugs: mesos-7923
https://issues.apache.org/jira/browse/mesos-7923


Repository: mesos


Description
---

Mesos port mapper cni plugin is a wrapper around bridge plugin
to add port mapping functionality to bridge plugin. However, in
certain cases the network creator doesn't need port mapping
functionality and just want to access bridge plugin. In this case,
the creator may not supply `args` in cni config which will makes
mesos port mapper plugin to fail. This patch makes `args` in cni
config optional for mesos port mapper plugin


Diffs
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448 


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


Testing
---


Thanks,

Deepak Goel



Review Request 61866: Adds port mapping information in NetworkInfo as part of `state` output.

2017-08-23 Thread Deepak Goel

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

Review request for mesos.


Repository: mesos


Description
---

Adds port mapping information in NetworkInfo as part of `state` output.


Diffs
-

  src/common/http.cpp 43d674e9286170aae92014a952c6a8ba66437ca5 


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


Testing
---


Thanks,

Deepak Goel