18723 - pass some of advisor config via --scheduler-config #20331
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
1 similar comment
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
|
Labelling this PR as size/S |
k8s-github-robot
added
the
size/S
|
I have signed the IBM corporate CLA -- My ibm email is linsun@us.ibm.com |
| @@ -351,7 +361,7 @@ func (s *SchedulerServer) serveFrameworkArtifactWithFilename(path string, filena | |||
| } | |||
|
|
|||
| func (s *SchedulerServer) prepareExecutorInfo(hks hyperkube.Interface) (*mesos.ExecutorInfo, error) { | |||
| ci := &mesos.CommandInfo{ | |||
| ci := &mesos.CommandInfo{ | |||
There was a problem hiding this comment.
Lin, did your git commit trigger a go syntax checker? If not then you need to upgrade your methodology.
There was a problem hiding this comment.
@linsun: I am referring specifically to http://kubernetes.io/v1.1/docs/devel/development.html#hooks
There was a problem hiding this comment.
@MikeSpreitzer I just added the hooks - thanks! Looking at the kubernetes master tree, the code was not correctly formatted: https://github.com/kubernetes/kubernetes/blob/master/contrib/mesos/pkg/scheduler/service/service.go -- line 354, so the patch is ok.
There was a problem hiding this comment.
this still isn't properly formatted
|
looking better. good rule of thumb: pushed code should compile and pass unit tests. |
|
@ixdy not sure what's going on with the CLA bot here |
|
CLAs look good, thanks! |
|
try naming the flags with On Fri, Jan 29, 2016 at 2:49 PM, googlebot notifications@github.com wrote:
|
|
check out the docs here: On Fri, Jan 29, 2016 at 2:57 PM, James DeFelice james@mesosphere.io wrote:
|
The executor passes flags on to cadvisor, so it they should pass through the filtering done in server.go. Fixes issue kubernetes#18723
|
@jdef sorry for the delay. I've reworked for the "_" issue. It took me quite long to get through running the smoke-test locally on my MacOS. I ran into a few issues (opened as JIRA) that I was able to resolve, but a few I could not. So at the end, I was able to run the test on a ubuntu VM. Here is the output, looks good: root@linsun3:~/kubernetes# ./contrib/mesos/ci/test-smoke.sh -v=2
|
|
I'm checking on the above failure from "Travis CI build failed". |
|
@jdef looking at the latest CI test result, it seems cadvisor is expecting "_" like housekeeping_interval, after I change it to housekeeping-interval. Any advice? |
|
Perhaps we try this approach? Since the For example: func HousekeepingInterval(defaultValue time.Duration) (v time.Duration) {
v = defaultValue
if f := flag.Lookup("housekeeping_interval"); f != nil && f.Value != nil {
if vstr := f.Value.String(); vstr != "" {
v = // ... attempt to parse it here
}
}
return
}
|
|
PR needs rebase |
k8s-github-robot
added
the
needs-rebase
|
@jdef, thank you for your comment! I'm looking into that and will provide update. |
k8s-github-robot
removed
the
needs-rebase
|
Labelling this PR as size/XS |
k8s-github-robot
added
size/XS
|
PR needs rebase |
k8s-github-robot
added
the
needs-rebase
|
were there any additional questions/concerns with the approach I suggested? |
|
Sorry for the delayed reply! My priority has changed, and I hope to look at this soon! |
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
|
Sorry for the super long delay - just started to look at this. @jdef, I don't fully understand your comment on Feb5. Are you saying creating a new pkg contrib/mesos/pkg/cadvisor and put the helper go class in there? If so, how does this helper method getting the flags passed from user from the Kubernetes scheduler? Also, why cannot I simply not use -, e.g. just use housekeeping_interval and remove the need for housekeeping-interval? |
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
k8s-github-robot
removed
the
needs-rebase
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
@jdef with my latest change, the km scheduler output looks ok, i.e. I no longer get 2 entries for each. I'm checking on my CLA, and check if I can test this. Thanks. |
|
sorry it's taken me a while to get back to this. thanks for revising this PR. i think there's a way forward without adding flags to the k8sm servers the "normal" way, but instead to just leverage the fact that the cadvisor flags are global and use that to more simply forward the values from process to process. this is what i was trying to say back in Feb (from your response it seems that my writing did not properly convey the idea). the cadvisor flags are being refactored and will, at some point, no longer be global. we're still waiting to see what form that will take. in the meantime, i'm trying to avoid changes to the scheduler and minion flag sets for this. see #22974 |
|
@jdef thank you for the code change and other pointers! Sorry I didn't quite understand how the util class can get the two flags earlier. I'm going to close this pull request and give your changes a try! |
|
great - let me know if it works for you. if so, maybe we can squeeze this On Tue, Mar 15, 2016 at 1:54 PM, Lin Sun notifications@github.com wrote:
|
@jdef -- reworked the patch based on your late last night's comment. thank you for your help!