You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by vixns <gi...@git.apache.org> on 2018/06/21 16:59:34 UTC
[GitHub] mesos pull request #299: Fix orphaning CNI network on recovery.
GitHub user vixns opened a pull request:
https://github.com/apache/mesos/pull/299
Fix orphaning CNI network on recovery.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vixns/mesos cni-orphan-all
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/mesos/pull/299.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #299
----
commit a8c69208e70c7e1cdc903e64c82ffba7ba358ebd
Author: Stéphane Cottin <st...@...>
Date: 2018-06-21T16:56:56Z
Fix orphaning CNI network on recovery.
----
---
[GitHub] mesos pull request #299: Fix orphaning CNI network on recovery.
Posted by vixns <gi...@git.apache.org>.
Github user vixns commented on a diff in the pull request:
https://github.com/apache/mesos/pull/299#discussion_r197739766
--- Diff: src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ---
@@ -428,6 +428,10 @@ Future<Nothing> NetworkCniIsolatorProcess::recover(
}
}
+ if (infos.contains(containerId)) {
+ continue;
+ }
+
--- End diff --
Understood, L421 is for orphaned tasks only, and in this case the L437 test matches.
So you're right, moving the continue to L421 make sense.
---
[GitHub] mesos pull request #299: Fix orphaning CNI network on recovery.
Posted by vixns <gi...@git.apache.org>.
Github user vixns commented on a diff in the pull request:
https://github.com/apache/mesos/pull/299#discussion_r197728745
--- Diff: src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ---
@@ -428,6 +428,10 @@ Future<Nothing> NetworkCniIsolatorProcess::recover(
}
}
+ if (infos.contains(containerId)) {
+ continue;
+ }
+
--- End diff --
are you sure ? if we move the continue to L421, L423 will have no effect.
---
[GitHub] mesos pull request #299: Fix orphaning CNI network on recovery.
Posted by vixns <gi...@git.apache.org>.
Github user vixns closed the pull request at:
https://github.com/apache/mesos/pull/299
---
[GitHub] mesos pull request #299: Fix orphaning CNI network on recovery.
Posted by qianzhangxa <gi...@git.apache.org>.
Github user qianzhangxa commented on a diff in the pull request:
https://github.com/apache/mesos/pull/299#discussion_r197723026
--- Diff: src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ---
@@ -428,6 +428,10 @@ Future<Nothing> NetworkCniIsolatorProcess::recover(
}
}
+ if (infos.contains(containerId)) {
--- End diff --
This will make the unknown orphaned containers are skipped (i.e., we will miss to do cleanup for unknown orphaned container). So I would suggest to just do a `continue;` between L420 and L421.
@vixns Do you want to post a patch in https://reviews.apache.org/?
---
[GitHub] mesos pull request #299: Fix orphaning CNI network on recovery.
Posted by qianzhangxa <gi...@git.apache.org>.
Github user qianzhangxa commented on a diff in the pull request:
https://github.com/apache/mesos/pull/299#discussion_r197734403
--- Diff: src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ---
@@ -428,6 +428,10 @@ Future<Nothing> NetworkCniIsolatorProcess::recover(
}
}
+ if (infos.contains(containerId)) {
+ continue;
+ }
+
--- End diff --
Why will L423 have no effect? I mean we should just do a `continue;` for the recoverable containers (L410 - L421), for the orphaned containers, we can still keep the current logic.
---
[GitHub] mesos pull request #299: Fix orphaning CNI network on recovery.
Posted by qianzhangxa <gi...@git.apache.org>.
Github user qianzhangxa commented on a diff in the pull request:
https://github.com/apache/mesos/pull/299#discussion_r197723561
--- Diff: src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ---
@@ -428,6 +428,10 @@ Future<Nothing> NetworkCniIsolatorProcess::recover(
}
}
+ if (infos.contains(containerId)) {
+ continue;
+ }
+
--- End diff --
This will make the unknown orphaned containers are skipped (i.e., we will miss to do cleanup for unknown orphaned container). So I would suggest to just do a continue; between L420 and L421.
@vixns Do you want to post a patch in https://reviews.apache.org/?
---