-
Notifications
You must be signed in to change notification settings - Fork 200
Support the Kubernetes Metrics Server #319
base: master
Are you sure you want to change the base?
Conversation
|
@abcdabcd987 thank you for sending this PR! Following the merge of #320, it now has conflicts which must be resolved. Could you please resolve them? |
|
resolved. |
|
Thanks a lot for your kind words and contributions, @abcdabcd987. The commit log shows that there is a merge commit. Can you rebase instead, so it's just one clean commit? |
45949ce to
34e9433
Compare
34e9433 to
f80d9b1
Compare
|
rebased. |
pires
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think @abcdabcd987 @bmcstdio ?
|
|
||
| - **USE_METRICS_SERVER** defines whether to deploy or not the [Kubernetes Metrics Server](https://github.com/kubernetes-incubator/metrics-server) | ||
|
|
||
| Defaults to `false`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Defaults to `false`. | |
| Defaults to `true`. |
This is required for the Horizontal Pod Autoscaler to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say agree because HPA is my motivation for adding this feature. The reason why I put it to false by default is that I haven't tested it in Windows nor RBAC (and I probably won't because I don't have this kind of setup).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure @bmcstdio can help here, right Bruno?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely. However, it seems to me that this PR needs some love. I checked out this branch and ran the following command:
$ NODES=1 USE_METRICS_SERVER=true vagrant up
Turns out the Kubernetes API can never be reachable:
$ kubectl get node
Unable to connect to the server: net/http: TLS handshake timeout
This does not happen in USE_METRICS_SERVER is set to false.
I also noticed high load on the system - far higher than usual. While I am not sure, part of what the root cause for this may be the fact that the following flag is not being defined:
--enable-aggregator-routing=true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, definitely need more tests. I only tested it on macOS. Also, good catch on --enable-aggregator-routing=true.
First of all, thank you Mr. Pires, for this very handy Vagrant project.
I added the support of the Kubernetes Metrics Server.
plugins/metrics-server/is frommetrics-server/deploy/1.8+/. And I have to make some changes to themanifests/master-apiserver.yamlaccording to this comment.I've tested it on Mac. But I'm not sure if it works on Windows. And also not sure about RBAC.