-
Notifications
You must be signed in to change notification settings - Fork 519
feat: upgrade metrics server to v0.3.4 #1109
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1109 +/- ##
==========================================
+ Coverage 76.67% 76.67% +<.01%
==========================================
Files 135 135
Lines 20536 20544 +8
==========================================
+ Hits 15745 15753 +8
Misses 3873 3873
Partials 918 918 |
/hold |
Metrics server 0.3.x doesn’t work with Windows nodes yet, |
thanks for reminder, I think we can upgrade to Metrics server 0.3.0 on Linux node first. |
I would suggest we introduce this as a 1.15 feature and not backport to pre-existing version (and thus, pre-existing clusters built on pre-1.15). If we do want to introduce this to older versions, we'll need thorough upgrade tests. |
@@ -126,7 +126,6 @@ spec: | |||
imagePullPolicy: IfNotPresent | |||
command: | |||
- /metrics-server | |||
- --source=kubernetes.summary_api:'' |
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.
@jackfrancis how do we handle command parameter change in kubernetesmasteraddons_xxx.yaml file in aks-engine?
I am planning to upgrade metrics-server
from v0.2.0 to v0.3.0 in k8s v1.15.0, while that require command parameters change, do you know what's the correct way to do this? One way could be move original kubernetesmasteraddons-metrics-server-deployment.yaml
file to new folder like 1.14
, 1.13
, 1.12
, ... , looks like I need to create a lot of such version folder, is that the correct way?
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.
Correct, the current way to define k8s version-specific changes is to create a new directory with the minor.major version of the first version that these changes work for. If there is only a 1.15 directory, for example (and not a 1.16 directory), then the logic assumes the spec is valid for 1.15 and above. So every time there is a forward looking version-breaking change, we create a new dir to store those with the same name as the version that introduces the change.
Hope that makes sense!
44a84df
to
cd703c7
Compare
Finally I have time to complete this PR, this PR only applies for k8s v1.15.0:
|
/hold
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Windows 1.16 tests failed the first attempt for unrelated reasons. Restarted it. |
Rekicked the tests, though the HPA failure in Linux test is almost certainly related to metrics-server non-functionality |
Peeking at the E2E tests in progress: it looks like the repro is HPA doesn't scale pods back down. |
Autoscaler tests are failing on Linux:
|
Windows nodes are autoscaling successfully:
Linux failing in my manual tests:
|
Actually I had a mistake in my Linux deployment. I didn't have a cpu request set, which needs to be there for metrics to be served per kubernetes/kubernetes#79365 (comment) after that it scales up and back down based on cpu
Seems like a test bug |
At this point I need to stop work on this. Can someone else pick it up? I have higher priority stuff waiting on me and the other maintainers should be able to handle it from here. I see no blockers from a Windows standpoint and my manual tests are good. |
6854051
to
43a2450
Compare
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyzhangx, jackfrancis The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
Reason for Change:
This PR upgrades metrics server from
v0.2.1
tov0.3.4
BTW, we cannot disable
--read-only-port
now since AKS also depends on this port and metrics server on AKS is still v0.2.xmetric server(v0.2.1) is still using KubeletReadOnlyPort(10255), we cannot switch to KubeletPort(10250) now, after upgrade to metric server v0.3.4, it uses 10250 port instead, then we could move on to merge PR fix: disable ReadOnlyPort
This PR only applies for k8s v1.16.0:
Tested on k8s v1.16.0-beta.1, test result:
Issue Fixed:
Fixes #1172
Requirements:
Notes:
cc @feiskyer
/azp run pr-e2e