[GitHub] [mesos] cf-natali commented on pull request #388: Fixed a bug where the cgroup task killer leaves the cgroup frozen.
cf-natali commented on pull request #388: URL: https://github.com/apache/mesos/pull/388#issuecomment-850602132 > @ridv This, unfortunately, is not a "small patch", but an attempt to compensate for a deficiency in a workaround in a code that uses a rather problematic kernel interface and is already full of hacks and short-term solutions. Indeed, this freezer code is quite tricky, and with good reason - at work we experienced 3 different kernel bugs involving the freezer cgroup, it's really a mess even after all those years... > @cf-natali I'm trying to look into this and to make sure this is not making some other issues worse. > > For example, does this all mean that this deferred call that freezes a cgroup > > https://github.com/apache/mesos/blob/85981d7c728798783b2e1e7cbed4f27a1497ccb2/src/linux/cgroups.cpp#L1440 > > has never been executed before this patch (due to immediate termination of a TaskKiller process), but, after this change, will be executing in some cases?... As far as I can tell, this should not exercise any new code path, and should actually make unusual code paths less frequent. The code you're referring to is only involved when the freeze operation timed out, and attempts to thaw and freeze it again - this can happen in normal circumstances, i.e. in the normal case where a cgroup is destroyed. This specific code path was exercised in the past - I checked using logs and `strace`, and will continue to be exercised if the freeze times out. One way to view this change is that it can only delay the termination of the `TaskKiller` process: therefore, the `TaskKiller` will only terminate in a state in which it could already be terminated in in the current code. The extra delay is a grace period letting some time to the `TaskKiller` to finish its freeze/kill/thaw cycle, hence making it much less likely to terminate it while it's in the middle of its freeze/kill/thaw. > (Btw, inablilty to comment on an untouched line in a github PR is one of examples why, as of 2019-2020, Mesos committers were still preferring to review contribution using Apache Reviewboard. I'm not saying that we should not do something to simplify contributing via GitHub, just illustrating why the RB has not been phased out.) Interesting, thanks! > I'm sorry, but reviewing such stuff meaningfully takes significant chunks of an uninterrupted time and should only be done in a very sound mind and a very good health (this was not my case in the beginnig of this week). I'm really sorry to hear that, and hope you're feeling better! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [mesos] ridv commented on pull request #388: Fixed a bug where the cgroup task killer leaves the cgroup frozen.
ridv commented on pull request #388: URL: https://github.com/apache/mesos/pull/388#issuecomment-850527177 @asekretenko that is absolutely understandable and I hope you find yourself in better health very soon. What you are doing here is greatly appreciated. My main concern, and this is in no way to put pressure on you as you're already doing a huge service to to project here, is the general radio silence from committers and PMC. Either the community has enough reviewers and volunteers to keep the project going, or we don't and new people that want to continue the project need to be given permission to continue the project (as per the Apache board members during the Attic discussions). I think this is a conversation best had on the mailing list (and in fact, I've kicked off a thread). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r641495281
##
File path: src/python/cli_new/lib/cli/config.py
##
@@ -119,6 +120,94 @@ def master(self):
return master
+def principal(self):
+"""
+Return the principal in the configuration file
+"""
+principal = self.data["master"].get("principal")
+if principal is None:
+return None
+elif not principal:
+raise CLIException("The 'principal' field in 'master'"
+ " must be non-empty")
+
+return principal
+
+def secret(self):
+"""
+Return the secret in the configuration file
+"""
+secret = self.data["master"].get("secret")
+if secret is None:
+return None
+elif not secret:
+raise CLIException("The 'secret' field in 'master'"
+ " must be non-empty")
+
+return secret
+
+def agent_ssl(self):
+"""
+Return if the agent support ssl
+"""
+if "agent" in self.data:
+agent_ssl = self.data["agent"].get("ssl")
+if agent_ssl is None:
+return None
+elif not agent_ssl:
+if not isinstance(self.data["agent"]["ssl"], bool):
Review comment:
Yeah I didnt commit and pushed just now. I wanted to wait until I get
the feedback to the "def secret". :-) Then I will also squash again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r641474981 ## File path: src/python/cli_new/lib/cli/http.py ## @@ -19,70 +19,63 @@ """ import json -import urllib.request -import urllib.error -import urllib.parse -import time +from urllib.parse import urlencode +import urllib3 import cli from cli.exceptions import CLIException +urllib3.disable_warnings() Review comment: Will do it :-) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r641474881
##
File path: src/python/cli_new/lib/cli/config.py
##
@@ -119,6 +120,94 @@ def master(self):
return master
+def principal(self):
+"""
+Return the principal in the configuration file
+"""
+principal = self.data["master"].get("principal")
+if principal is None:
+return None
+elif not principal:
+raise CLIException("The 'principal' field in 'master'"
+ " must be non-empty")
+
+return principal
+
+def secret(self):
+"""
+Return the secret in the configuration file
+"""
+secret = self.data["master"].get("secret")
+if secret is None:
+return None
+elif not secret:
+raise CLIException("The 'secret' field in 'master'"
+ " must be non-empty")
+
+return secret
+
+def agent_ssl(self):
+"""
+Return if the agent support ssl
+"""
+if "agent" in self.data:
+agent_ssl = self.data["agent"].get("ssl")
+if agent_ssl is None:
+return None
+elif not agent_ssl:
+if not isinstance(self.data["agent"]["ssl"], bool):
Review comment:
:+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [mesos] asekretenko edited a comment on pull request #388: Fixed a bug where the cgroup task killer leaves the cgroup frozen.
asekretenko edited a comment on pull request #388: URL: https://github.com/apache/mesos/pull/388#issuecomment-850258298 @cf-natali I'm trying to look into this and to make sure this is not making some other issues worse. For example, does this all mean that this deferred call that freezes a cgroup https://github.com/apache/mesos/blob/85981d7c728798783b2e1e7cbed4f27a1497ccb2/src/linux/cgroups.cpp#L1440 has never been executed before this patch (due to immediate termination of a TaskKiller process), but, after this change, will be executing in some cases?... (Btw, inablilty to comment on an untouched line in a github PR is one of examples why, as of 2019-2020, Mesos committers were still preferring to review contribution using Apache Reviewboard. I'm not saying that we should not do something to simplify contributing via GitHub, just illustrating why the RB has not been phased out.) @ridv This, unfortunately, is not a "small patch", but an attempt to compensate for a deficiency in a workaround in a code that uses a rather problematic kernel interface and is already full of hacks and short-term solutions. I'm sorry, but reviewing such stuff meaningfully takes significant chunks of an uninterrupted time and should only be done in a very sound mind and a very good health (this was not my case in the beginnig of this week). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [mesos] asekretenko commented on pull request #388: Fixed a bug where the cgroup task killer leaves the cgroup frozen.
asekretenko commented on pull request #388: URL: https://github.com/apache/mesos/pull/388#issuecomment-850258298 @cf-natali I'm trying to look into this and to make sure this is not making some other issues worse. For example, does this all mean that this deferred call that freezes a cgroup https://github.com/apache/mesos/blob/85981d7c728798783b2e1e7cbed4f27a1497ccb2/src/linux/cgroups.cpp#L1440 has never been executed before this patch (due to immediate termination of a TaskKiller process), but, after this change, will be executing in some cases?... (Btw, inablilty to comment on an untouched line in a github PR is one of examples why, as of 2019-2020, Mesos committers were still preferring to review contribution using Apache Reviewboard. I'm not saying that we should not do something to simplify contributing via GitHub, just illustrating why the RB has not been phased out.) @ridv This, unfortunately, is not a "small patch", but an attempt to compensate for a deficiency in a workaround in a code that uses a rather problematic kernel interface and is already full of hacks and short-term solutions. I'm sorry, but reviewing such stuff meaningfully takes significant chunks of an uninterrupted time and should only be done in a sound mind and a good health (this was not my case in the beginnig of this week). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
