CA expander plugin proposal #4134
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @evansheng! |
k8s-ci-robot
added
the
size/M
|
I signed it |
|
@evansheng you probably want to squash your commits. I think the CLA bot is complaining because some still have another email tied to them: |
| } | ||
| ``` | ||
|
|
||
| To communicate with the external gRPC server, CA needs new flags to expose details about the server. |
There was a problem hiding this comment.
How are we planning on handling errors? Just fallback to the random expander on failures?
There was a problem hiding this comment.
yep, was planning on handling similarly to the other current expanders, and falling back to the random strategy
https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/expander/priority/priority.go#L59
evansheng
force-pushed
the
es--expander-plugin-proposal
branch
from
79d6001 to
a956b9c
Compare
k8s-ci-robot
added
cncf-cla: yes
|
/assign @MaciekPytel |
There was a problem hiding this comment.
i think the external grpc expander plugin sounds really cool. i'm curious though, given this discussion about a new node processor, if we wouldn't want to follow a similar pattern with expanders (assuming adding more is acceptable) ?
if we had something similar that allowed for custom expanders to be added and maintained in repo, then adding the grpc-based expander could follow that pattern. (similar in some respects to this proposal for a grpc provider)
curious to hear what others think
|
hey, i talked with @evansheng on slack, just wanted to clarify my comment. i like the idea for the grpc expander, no major objection to the proposal. when it comes to implementation details though, i think we should be following the patterns in the cloud providers and node processors to ensure we have a nice way of doing this in the future if/when others wish to contribute in this area. |
|
@MaciekPytel bump! any updates on this proposal review? |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
There was a problem hiding this comment.
this generally makes sense to me and i can see the value in adding it, i have a few suggestions to the text and i am also curious about alternatives. has there been any discussion about approaches that don't use grpc? and if so, i think it would be nice to enumerate other ideas that might have come up when thinking about this.
|
|
||
| ## Proposal | ||
|
|
||
| We will extend CA to utilize a pluggable external expander. The design for this expander plugin is heavily based off of this [proposal](https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/proposals/plugable-provider-grpc.md) to CA, for a pluggable cloud provider interface. |
There was a problem hiding this comment.
i would depersonalize this a little, perhaps something like this:
| We will extend CA to utilize a pluggable external expander. The design for this expander plugin is heavily based off of this [proposal](https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/proposals/plugable-provider-grpc.md) to CA, for a pluggable cloud provider interface. | |
| Extend the cluster autoscaler to utilize a pluggable external expander. The design for this expander plugin is heavily based off of this [proposal](https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/proposals/plugable-provider-grpc.md) to CA, for a pluggable cloud provider interface. |
|  | ||
|
|
||
| The gRPC server must implement the API of the [expander strategy interface](https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/expander/expander.go#L50) in CA, which only has one method. | ||
|
|
There was a problem hiding this comment.
i think it would be really cool to see a small example of the main function for an expander server
|
|
||
| To communicate with the external gRPC server, CA needs new flags to expose details about the server. | ||
|
|
||
| We’ll add a new option to the expander flag: `--expander=externalgrpc`, and inntroduce a new flag `--expander-plugin-url=https://external-grpc-url/server` to reach the gRPC server. |
There was a problem hiding this comment.
i would depersonalize here as well, and also a small spelling nit
| We’ll add a new option to the expander flag: `--expander=externalgrpc`, and inntroduce a new flag `--expander-plugin-url=https://external-grpc-url/server` to reach the gRPC server. | |
| Add a new option to the expander flag: `--expander=externalgrpc`, and introduce a new flag `--expander-plugin-url=https://external-grpc-url/server` to reach the gRPC server. |
|
|
||
| We’ll add a new option to the expander flag: `--expander=externalgrpc`, and inntroduce a new flag `--expander-plugin-url=https://external-grpc-url/server` to reach the gRPC server. | ||
|
|
||
| Additionally, we’ll need to use TLS for secure communication. An additional flag `--external-expander-cert=~/path/to/cert` will be exposed to specify the path to the certificate authority bundle used to validate the TLS cert used by the server. |
There was a problem hiding this comment.
i would clean this up a little too
| Additionally, we’ll need to use TLS for secure communication. An additional flag `--external-expander-cert=~/path/to/cert` will be exposed to specify the path to the certificate authority bundle used to validate the TLS cert used by the server. | |
| When needing to use TLS for secure communications, an additional flag `--external-expander-cert=~/path/to/cert` will be exposed to specify the path to the certificate authority bundle used to validate the TLS cert used by the server. |
|
Update on this proposal - I have a working implementation version in PR here: #4452 And a corresponding internal gRPC expander running as a separate application. |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
thanks for the reminder @evansheng, i think we should merge the enhancement if only to add the documentation as @gjtempleton mentioned. |
|
/reopen |
|
@evansheng: Reopened this PR. In response to this:
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. |
|
@gjtempleton @elmiko re-opened, and should be good to merge once approved |
|
adding lgtm to get this merged as we already have the implementation. i think it would be nice to clean this up a little, but having it is better than not having it. thanks again @evansheng /lgtm |
k8s-ci-robot
added
the
lgtm
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
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. |
|
/reopen |
|
@evansheng: Reopened this PR. In response to this:
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. |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
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. |
|
/reopen @gjtempleton @mwielgus could we merge this so that we can capture the documentation? |
|
@elmiko: Reopened this PR. In response to this:
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. |
|
/remove-lifecycle rotten |
k8s-ci-robot
removed
the
lifecycle/rotten
|
Agree with most of elmiko's suggestion/nits, however given this is already implemented, let's get it merged and do any clean-up in a follow-up PR. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: evansheng, gjtempleton 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 |
k8s-ci-robot
added
the
approved
…oposal CA expander plugin proposal
Proposal for an expander plugin to allow users to iterate on custom expander strategies out of band with Cluster Autoscaler
Heavily based off of https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/proposals/plugable-provider-grpc.md for a plugable cloud provider.