Skip to content
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

Added KEP for custom metrics router #2581

Closed
wants to merge 5 commits into from

Conversation

arjunrn
Copy link
Contributor

@arjunrn arjunrn commented Mar 21, 2021

For #2580

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 21, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @arjunrn. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 21, 2021
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 21, 2021
@arjunrn arjunrn mentioned this pull request Mar 21, 2021
4 tasks
Signed-off-by: Arjun Naik <arjun.rn@gmail.com>
![Router Architecture](custom_metrics_router.png)

#### Metrics Discovery Controller
`CustomMetricsSource` is the proposed CRD which should be used to register

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current name could give users an impression, that it servers custom metrics only. What about MetricsSourceProvider or a similar name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to other names. I wanted to keep the name similar to the APIService object already present and with the custom-metrics-apiserver.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I just would like to avoid the Custom prefix, since this should deal with external metrics as well, otherwise I don't have a strong opinion on the name.
APIService objects are named v1beta[12].custom.metrics.k8s.io or v1beta1.external.metrics.k8s.io, that depends on the type of metrics.

@brancz
Copy link
Member

brancz commented Mar 22, 2021

The intention of this is sound. Last time I proposed something like this apimachinery folks weren't sure how they felt about this approach. Maybe someone can weigh in @kubernetes/sig-api-machinery-misc . In some sense, as you mention in the proposal this is basically a per-namespace APIService. I do wonder how we expect this to work especially with the intention of adding the watch API to the metrics APIs.

cc @logicalhan

@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 22, 2021
arjunrn and others added 2 commits March 22, 2021 12:03
Co-authored-by: Zbynek Roubalik <726523+zroubalik@users.noreply.github.com>
Signed-off-by: Arjun Naik <anaik@redhat.com>
@arjunrn
Copy link
Contributor Author

arjunrn commented Mar 22, 2021

@brancz Thanks for inputs. I'm not sure what you mean by "this is basically a per-namespace APIService" because the new CRD I'm proposing is not namespaced.
And about the metrics API offering watch. The way I see when the metrics-client is extended to support it the router can implement it as well proxying the watch.

@lavalamp
Copy link
Member

This can be a drop-in replacement of the existing custom metric apiserver, right? no apiserver or controller manager changes needed?

@arjunrn
Copy link
Contributor Author

arjunrn commented Mar 25, 2021

@lavalamp Yes, this requires no changes in the apiserver or the controller manager. But it's not a replacement but a proxy for several custom metric apiservers. There's a diagram in the KEP but it doesn't get rendered correctly in the PR. Here's what it will look like.
custom_metrics_router

@brancz
Copy link
Member

brancz commented Mar 26, 2021

I'm generally happy with this. Apimachinery should be as well though and perform an API review.

@ehashman
Copy link
Member

ehashman commented Apr 8, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 8, 2021
Signed-off-by: Arjun Naik <anaik@redhat.com>
@ehashman
Copy link
Member

/assign @logicalhan

SIG Instrumentation approver

@lavalamp
Copy link
Member

requires no changes in the apiserver or the controller manager

Sorry for slow response, I have no objections from API Machinery.

@ehashman ehashman added the tracked/out-of-tree Denotes an out-of-tree enhancement issue, which does not need to be tracked by the Release Team label May 11, 2021
@ehashman
Copy link
Member

We discussed this at our last instrumentation meeting. I believe that this enhancement is out of tree, so following the KEP cycle is optional and the release team does not need to track this PR. PRR is also optional.

@arjunrn
Copy link
Contributor Author

arjunrn commented May 14, 2021

@serathius So how do we proceed here?

Copy link

@barkbay barkbay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR !
I just left a few questions.


#### API Discovery
API servers expose endpoints which can be used by a client to find the
correct resource version and discover information about which API resources
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the "correct version" the one in the preferredVersion field ?

{
  "kind": "APIGroup",
  "apiVersion": "v1",
  "name": "custom.metrics.k8s.io",
  "versions": [
    {
      "groupVersion": "custom.metrics.k8s.io/v1beta2",
      "version": "v1beta2"
    },
    {
      "groupVersion": "custom.metrics.k8s.io/v1beta1",
      "version": "v1beta1"
    }
  ],
  "preferredVersion": {
    "groupVersion": "custom.metrics.k8s.io/v1beta2",
    "version": "v1beta2"
  }
}

Do we want to allow the user to force a version as it is possible in the APIServiceSpec ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The router would rely on the official metrics-client and I assume that the client uses the discovery API to find the preferred version and then query that. I might be wrong. Is there any use case you can think of which would require an override of the default behavior.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any use case you can think of which would require an override of the default behavior.

No, I think you're right. Thanks for the clarification 👍

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The router would rely on the official metrics-client and I assume that the client uses the discovery API to find the preferred version and then query that

Sorry, I remember now 😄 : I'm not sure it is possible to use the metrics-client to discover the list of metrics.
For example in your proof concept you use the discovery client:

c.discoveryClient.ServerResourcesForGroupVersion(customMetricsAPI.SchemeGroupVersion.String())

In the snippet above customMetricsAPI.SchemeGroupVersion.String() would be statically resolved according to the imported version of customMetricsAPI. Which might not be the same than the one selected by the metrics client.

But I don't think it's a problem since we can reuse the AvailableAPIsGetter from the metrics client, for example if we keep cmClient.NewAvailableAPIsGetter(discoveryClient) in the Client structure:

version, err := c.customMetricsAvailableAPIsGetter.PreferredVersion()
if err != nil {
	return nil, err
}
resources, err := c.discoveryClient.ServerResourcesForGroupVersion(version.String())
if err != nil {
	return nil, fmt.Errorf("failed to get resource for %s: %v", customMetricsAPI.SchemeGroupVersion, err)
}

But this is an implementation detail, and you can still ignore my initial comment in the context of the KEP.

be sent to either service with equal probability. In the case of the metrics
service it might not make sense to do that same because the returned metric
values may differ causing the replica count of the target to fluctuate.
Instead the age of the metrics API service registration object can be used as
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate on what object age is used here ? And how ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All kubernetes resources(custom or otherwise) have a creation timestamp was is set when the resource is created. The routing table would contain entries of providers which expose a given metric. This would be sorted in increasing order based on the creation timestamp.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern with the creation timestamp is that it is not obvious for the user to get it, even displayed in a column it is hard to figure out which one is the most recent among a bunch of resources.

I was wondering if sorting resources by name would be a better choice:

  • metadata.name is an immutable field, if a resource is deleted and then recreated with the same name the creation timestamp will change, but not its name. It makes the behaviour more predictable.
  • I don't have a good example for K8S but on OpenShift this is how SCC are prioritized. My experience as a user is that it is a nice way to solve that problem.

What do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for using timestamp is to reduce the chances of accidentally changing the provider which exposes a given metric. This might make it harder to debug but the metric value would be stable. Assume the user registers a new provider and is unaware that it exposes a metric which is currently in use. The new metric value is different to the existing metric value. This means the scaling target would suddenly scale up or down probably causing an outage.
Also to improve debugging I think the routing table could be written out to a configmap which can be examined for debugging. There is also a need to provide overrides for specific metric which this KEP doesn't address but we can cross that bridge when we get there.

Signed-off-by: Arjun Naik <arjun.rn@gmail.com>
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: arjunrn
To complete the pull request process, please ask for approval from logicalhan after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@djamaile
Copy link

djamaile commented Jun 16, 2021

I am interested in the feature. Any progress on getting the merged? looks like it is almost ready.

@arjunrn
Copy link
Contributor Author

arjunrn commented Jun 18, 2021

@ehashman Maybe you could help here. What should we do with this KEP? Merge/close? I can transfer the ownership of the router I have implemented and become a maintainer for it also with someone from sig-instrumentation.

@dgrisonnet
Copy link
Member

If I remember correctly from the sig-instrumentation meeting where this KEP was discussed, the path forward was to move the KEP to either https://github.com/kubernetes-sigs/instrumentation or https://github.com/kubernetes-sigs/custom-metrics-apiserver since it isn't related to k/k.
The original reason for the KEP to be opened here was that we wanted to discuss if it was possible to have aggregated APIs namespaced, but now that this solution has been dismissed, I am fine with moving the KEP to the custom-metrics-apiserver repo.

I personally think that it makes sense to have the router living in its own repo in kubernetes-sigs as part of the project owned by sig-instrumentation since it is an essential add-on to the metrics API story for users who want to use multiple metrics servers. wdyt @serathius @brancz @s-urbaniak?

Copy link

@barkbay barkbay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if the KEP should provide some details about what happens if it is not possible to get the metrics list from one of the MetricsSource. For example, say that a MetricsSource with a high priority has a temporarily failure and it is not possible to initialize or refresh the list of the metrics. Does it prevent the router to serve requests ? Should we add a parameter somewhere to configure the expected behaviour in that situation ?

@brancz
Copy link
Member

brancz commented Jun 24, 2021

I think what @dgrisonnet said makes sense to me.

@djamaile
Copy link

@arjunrn are you going to move the PR?

@mastersingh24
Copy link

Did this get moved?

@djamaile
Copy link

djamaile commented Oct 3, 2021

Any update?

@arjunrn
Copy link
Contributor Author

arjunrn commented Nov 14, 2021

Sorry everyone, I have not had the time to work on this and won't have be able to any time soon. If someone wants to pick this let me know. I can also transfer the ownership of the prototype repo to you or to the sigs org.

@mastersingh24
Copy link

@arjunrn - Happy to try to take this forward. Is this supposed to move to https://github.com/kubernetes-sigs/custom-metrics-apiserver ?

@arjunrn
Copy link
Contributor Author

arjunrn commented Nov 15, 2021

@mastersingh24 It can also be moved to https://github.com/kubernetes-sigs/instrumentation according to this comment. I'm closing this PR because it should be added this repo.

@arjunrn arjunrn closed this Nov 15, 2021
@scarby
Copy link

scarby commented Feb 18, 2022

@mastersingh24 Did you move anywhere with this? Can take a look if not

@mastersingh24
Copy link

@scarby - I have not. Would love to try to move this forward, but could use some help moving it somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tracked/out-of-tree Denotes an out-of-tree enhancement issue, which does not need to be tracked by the Release Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet