Add support to query kubelet's logs and cadvisor's stats through apiserver by passing rawquery #457
Conversation
dchen1107
commented
Jul 15, 2014
dchen1107
commented
- Also add a thirdparty library: code.google.com/p/go.net/html since it is required to override customized Transport for apiserver Proxy to update HTTPResponse pointing to proper minion.
- Now minion information can be queried as: 1) id=${minion}&query=/stats// or 2) id=${minion}&query=logs/
|
The PR is ready for review now. Ping @lavalamp @thockin @brendandburns Thanks! |
|
|
||
| minionHost := values.Get("id") | ||
| queryUrl := "http://" + minionHost | ||
| if strings.LastIndex(queryUrl, ":10250") < 0 { |
There was a problem hiding this comment.
Suggest just checking the port in the parsed URL object you make below.
There was a problem hiding this comment.
If I just checked URL and found there is no port, I have to feed port and re-parse. Is that what you suggested here? I did make a change to use SplitHostPort here instead.
|
@smarterclayton Can I send a separate PR for handling those requests in a operations queue? @lavalamp I addressed most of your comments. PTAL? If you prefer, I could split this PR into several different PRs. Thanks! |
|
Agree adding to operation queue is probably best added in another PR. @dchen1107, I'll take another look soon, hopefully before lunch. |
| // id=${minion}&query=podInfo?podID=<podid> | ||
| // | ||
| // To query logs on a minion, rawQuery can be: | ||
| // id=${minion}&query=logs/ |
There was a problem hiding this comment.
I hope you'll consider my suggestion about switching to a proxy/minion/<minion id>/... system. But if not, I feel that the parameter should be "path" or perhaps "rawurl", not "query"; query specifically means the part of the url string between the ? and # characters, but you're sending a path.
There was a problem hiding this comment.
Actually I am considering to change it. How about hold the review, once I changed it and ping you again?
|
PTAL? Thanks |
| glog.Errorf("Failed to create request: %s", err) | ||
| } | ||
|
|
||
| proxy := httputil.NewSingleHostReverseProxy(minionURL) |
There was a problem hiding this comment.
Shouldn't this URL and the one you pass to the GET request match?
There was a problem hiding this comment.
That is what I thought initially. Unfortunately they don't. From godoc, the new ReverseProxy returned here treats Path provides in URL as base path in target.
|
Needs rebase. A few more comments, otherwise LGTM . |
| Scheme: "http", | ||
| Host: minionHost, | ||
| } | ||
| newReq, err := http.NewRequest("GET", minionPath+"?"+rawQuery, nil) |
There was a problem hiding this comment.
In that case, can you construct this URL via a URL object? Sorry to be such a pain about this...
There was a problem hiding this comment.
I also tried that, actually that is my initial version I started from at the beginning, but ran into mystery issue with a nil response. I tried it yesterday again, and had the same result. I also dumped the entire Request, before and after proxied, and comparing them with dumps with this version field by field yesterday, cannot figure it out. It looks like a bug in httputil package. Already spent too much time on this, didn't dive deeper.
|
I think I'm OK with this. If you can squash some of the commits and figure out why your test is flaky in travis, I think we're good to go. |
apiserver by passing rawquery. minor changes Fixed a minor rebase issues. Using ioutil.ReadAll instead of httputil.DumpResponse
|
I did rebase, also replace httputil.DumpResponse with ioutil.ReadAll since there are a race issue found in dump.go in go1.2. Travis build is failed due to the command "gvm install" failed. I noticed that several other PR failed due to the same reason. I ran the tests many times on my local machine, and all succeed! |
|
I restarted travis. Looks like it's still broken for go :( |
|
@lavalamp Travis is back to normal, and I think this PR is ready for merge now? |
Add support to query kubelet's logs and cadvisor's stats through apiserver by passing rawquery
|
Thanks for the change! |
Fix broken links of scheduling design docs
add configGeneration to README in demos dir
scripts/build: compat header
