-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prep for v0.3.0 release #113
Prep for v0.3.0 release #113
Conversation
@serathius @brancz some quick improvements to the README before tagging an alpha. Will tag the alpha once this merges. |
emptyDir: {} | ||
containers: | ||
- name: metrics-server | ||
image: gcr.io/google_containers/metrics-server-amd64 |
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.
image descriptors without tag often cause problem, we should be explicit here
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.
the intention is that these are used to deploy pre-release versions, or things like that. Since we don't actually specify different options in the manifest, I'm not certain it's strictly necessary to have them, but I don't think a tag makes much sense here.
@@ -21,6 +21,7 @@ Compatibility matrix: | |||
|
|||
Metrics Server | Metrics API group/version | Supported Kubernetes version | |||
---------------|---------------------------|----------------------------- | |||
0.3.x | `metrics.k8s.io/v1beta1` | 1.8+ |
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 think new release dropped support for unsecured kubelet port, this also adds requirement for kubelet to support Token Review.
Will this change supported versions?
For example GKE does not support Token Review yet (I'm working on it, but still don't have confirmation from mikedanese)
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.
If we consider rolling this version into k8s 1.12, I would prefer to have 9bb8fe9 rolled back.
We can change default behavior to use secure port, but we should not drop support for http.
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.
Will this change supported versions?
It shouldn't change the supported versions, since Kubernetes has supported kubelet token review for a while, IIRC (1.5ish, I think -- kubernetes/kubernetes#34381).
Plus, it doesn't require token review -- presumably, you can use kubeconfig with client certs instead. Is that insufficient?
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.
Can you provide configuration for GKE in deployment?
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.
talked with @serathius on slack a bit, and the upshot is that if GKE doesn't have Kubelet token support, configuration for the automated GKE tests in k/k is going to be tricky. I'm going to add the option back in, with the expectation that we'll get a timeline for it's removal in place soon.
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.
README.md
Outdated
behavior: | ||
|
||
- `--metric-resolution=<duration>`: the interval at which metrics will be | ||
scraped from Kubelets. |
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.
Should it mention the default?
README.md
Outdated
scraped from Kubelets. | ||
|
||
- `--kubelet-insecure-tls`: skip verifying Kubelet CA certificates. Not | ||
reccomended for production usage, but can be useful in test clusters |
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.
s/reccomended/recommended
/cc @tallclair |
I'm testing image before release and I noticed behavior that metrics server is always reporting 1m window, even if I change metric-resolution to different values. |
Yeah, we used to hard code the window, so I kept that behavior. I've got a PR that changes it to be closer to the underlying summary window, but since the summary API doesn't report the actual window size (it could, but it doesn't), we just have to fix at the max the summary can give (with is 30s, since max-housekeeping is 15s, and the jitter logic can double that). |
(and changing the "metrics-resolution" doesn't actually change the window size, since we're pulling usage rate from the summary API) |
/cc @kawych |
This preps for the v0.3.0 release by adding, a note in the README, and flag documentation to the README. Changing the manifest versions will come once the image is published.
8b37b30
to
4a2c4f1
Compare
This preps for the v0.3.0 release by adding example deployment
manifests for the development release, a note in the README, and flag
documentation to the README.