Introduce types and clients for Metrics APIs #2
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
@piosz PTAL |
|
A couple of notes on this PR: Generating the generated files requires kubernetes/repo-infra#7 (and needs to be placed in vendor). I've tested this PR against my WIP copy of HPA v2 part III (use the custom metrics API, and also use the actual resource metrics API clients), so the code is at least sane enough to work properly (the bits required for the testing can be found at https://github.com/DirectXMan12/kubernetes/tree/feature/hpa-v2-use-cm-api and https://github.com/DirectXMan12/heapster/tree/feature/use-cm-types, which should turn into actual PRs shortly). @kubernetes/sig-api-machinery-misc I wasn't sure how to avoid depending on |
|
Can we split this PR into 2 separate ones: with custom metrics and metrics? Overall looks good for me, thought I'd like some from api-machinery team to take a look. |
|
cc @kubernetes/sig-api-machinery-pr-reviews PTAL for general sanity |
We have kubernetes/kubernetes#41486 open and lgtm'ed. Google US is out today, so we'll given Daniel a chance to take a look tomorrow. |
|
ack, I'll pull that into kubernetes/repo-infra#7 once it's merged, and then regen this PR |
DirectXMan12
force-pushed
the
feature/api-clients
branch
from
8985a95
to
1666ed3
Compare
|
This should be all set with regards to kubernetes/kubernetes#41486 -- I've regenerated with that code. |
|
After a quick glance-over from someone on @kubernetes/sig-api-machinery-pr-reviews, I'd like to get this merged so that the Kubernetes PRs depending on it can move forward |
pkg/apis/custom_metrics/register.go
Outdated
| ) | ||
|
|
||
| // GroupName is the group name use in this package | ||
| const GroupName = "custom-metrics" |
There was a problem hiding this comment.
group names should be domain qualified. This one looks like custom-metrics.metrics.k8s.io based on the repo name.
pkg/apis/custom_metrics/doc.go
Outdated
|
|
||
| // +k8s:deepcopy-gen=package,register | ||
|
|
||
| package custom_metrics // import "k8s.io/metrics/pkg/apis/custom_metrics" |
There was a problem hiding this comment.
You'll need to specify a group name. See https://github.com/kubernetes/sample-apiserver/blob/master/pkg/apis/wardle/doc.go#L20 as an example.
There was a problem hiding this comment.
doesn't that tag only apply for generated clients? None-the-less, you're right, I should probably have it here.
| ) | ||
|
|
||
| // GroupName is the group name use in this package | ||
| const GroupName = "metrics" |
There was a problem hiding this comment.
domain qualify. "metrics.k8s.io" is legal for you if you want it.
There was a problem hiding this comment.
domain qualify. "metrics.k8s.io" is legal for you if you want it.
But it could get really confusing when deciding if you're referring to a type or a group.
There was a problem hiding this comment.
This is a pre-existing API, so it probably shouldn't change names, right?
There was a problem hiding this comment.
This is a pre-existing API, so it probably shouldn't change names, right?
What version was your API? At an alpha or beta level, I'd consider cross registering and sunset the old.
|
|
||
| // +genclient=true | ||
| // +resourceName=nodes | ||
| // +readonly=true |
There was a problem hiding this comment.
Don't you need a "nonNamespaced" tag like here: https://github.com/kubernetes/kubernetes/blob/master/pkg/api/types.go#L2862 ?
Also, care to open an issue for us to add an example of this to the sample-apiserver?
There was a problem hiding this comment.
Yep, missed that tag (it wasn't present in the original types in Heapster). As for the resourceName and readonly tags, I have to submit a PR to get them into Kube's client-gen first probably, but yeah, I'll open an issue.
| // Container name corresponding to the one from pod.spec.containers. | ||
| Name string | ||
| // The memory usage is the memory working set. | ||
| Usage api.ResourceList |
There was a problem hiding this comment.
Since you've done this, you'll need to be sure that you register the ResourceList and all transitive conversions and deep copies in the appropriate versions.
There was a problem hiding this comment.
ResourceList doesn't have a deepcopy, Resource has deepcopy as a method, ResourceList's conversion is marked copy-only, as is Resource's, which AFAICT, means it's not needed for nested conversions.
|
You need fuzzer tests. See https://github.com/kubernetes/sample-apiserver/blob/master/pkg/apis/wardle/install/install_test.go |
DirectXMan12
force-pushed
the
feature/api-clients
branch
2 times, most recently
from
dda58ab
to
fc35d23
Compare
|
Alright, I've addressed all the comments here, I believe (with the exception of cross-registering |
This commit includes non-generated custom metrics API types.
This commit updates the generated files for the custom metrics API.
This commit introduces the client for the custom metrics API. The API is written by hand, since the structure of the custom metrics API does not lend itself to auto-generation.
This commit imports the resource metrics API types from Heapster. The generated conversions and clients are in a separate commit.
This commit checks in the generated code (clientsets, informers, listers, deepcopy, conversion, etc) that go with the resource metrics API types.
DirectXMan12
force-pushed
the
feature/api-clients
branch
from
fc35d23
to
5fc9906
Compare
Automatic merge from submit-queue HPA Controller: Use Custom Metrics API This commit switches over the HPA controller to use the custom metrics API. It also converts the HPA controller to use the generated client in k8s.io/metrics for the resource metrics API. In order to enable support, you must enable `--horizontal-pod-autoscaler-use-rest-clients` on the controller-manager, which will switch the HPA controller's MetricsClient implementation over to use the standard rest clients for both custom metrics and resource metrics. This requires that at the least resource metrics API is registered with kube-aggregator, and that the controller manager is pointed at kube-aggregator. For this to work, Heapster must be serving the new-style API server (`--api-server=true`). Before this merges, this will need kubernetes/metrics#2 to merge, and a godeps update to pull that in. It's also semi-dependent on kubernetes-retired/heapster#1537, but that is not required in order for this to merge. **Release note**: ```release-note Allow the Horizontal Pod Autoscaler controller to talk to the metrics API and custom metrics API as standard APIs. ```
This PR introduces types and clients for both the custom metrics API and the resource metrics API.
The resource metrics API uses autogenerated clients, while the custom metrics API, due the nature of it's URLs, uses a custom client.