-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Cloudprovider hetzner #3640
Cloudprovider hetzner #3640
Conversation
…0c19d0cf90b756c3e08e32c6495b91e0aeed (3eb90c1)
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. |
Welcome @Fgruntjes! |
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.
We don't accept any new dependencies in vendor
. Please contain your dependencies inside cloudprovider/hetzner
. See Alibaba cloudprovider for reference. https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler/cloudprovider/alicloud.
Thank you, solved that issue. |
Some high-level comments:
/check-cla |
@MaciekPytel Thank you for the suggestions Yes, I have signed it but somehow it is not recognizing I now updated our organization with the email domain and set it to public. Let's see if that's enough. Will add the OWNERS and linter exclude. /check-cla |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Fgruntjes 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 |
I feel like this is not a scalable way to manage all cloud providers in-tree now. |
@alexanderkjeldaas Yeah, this PR should not be blocked for sure. Seems for the past half year, 2-3 cloud providers are added in the repo, I am thinking sig-autoscaler should prioritize pluggable solutions. |
The following users are mentioned in OWNERS file(s) but are untrusted for the following reasons. One way to make the user trusted is to add them as members of the kubernetes org. You can then trigger verification by writing
|
I have written an alternative Hetzner autoscaler implementation we have been using successfully since April. Glad to see someone went through to open a pull request, I will have a look at your code and see if I can come up with any useful improvements before this is merged. :) @Fgruntjes |
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.
Really well done! Especially nice to see a fully working node group implementation, my own implementation was limitted to a single node group. I added a few comments, I hope they are helpful.
cluster-autoscaler/cloudprovider/hetzner/hetzner_cloud_provider.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/cloudprovider/hetzner/hetzner_cloud_provider.go
Outdated
Show resolved
Hide resolved
@Fgruntjes: PR needs rebase. 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. |
Is there any reason for that? How it's intended to be updated? |
Co-authored-by: Anton Bessonov <anton.bessonov.public@gmail.com>
…r.go Co-authored-by: Niklas Korz <niklas@niklaskorz.de>
…r.go Co-authored-by: Niklas Korz <niklas@niklaskorz.de>
You are responsible for updating the dependencies once/if they are needed. We don't want to onboard any new libraries as they may have conflicting dependencies with other libraries. Imagine that both cloud provider A and B SDK requires library X. However A is only compatible with 1.0. B SDK has been migrated to 2.0. But Cluster Autoscaler still is on old B SDK with X in version 1.0. Now you want to update B due to security issues. How would you do that? There is no easy solution. You cannot kick out cloud provider A. You cannot have 2 libraries in the same binary. You have to upgrade B. |
Please resolve conflicts and squash commits to max 3 and we are good to go. |
Hello guys, @Fgruntjes asked us (Hetzner Cloud Team) to take over this MR. We (my colleague @fhofherr and I) want to take this over and further support it and also update everything in the future. So the plan was that we (@fhofherr and i (@LKaemmerling)) are set within the code owners file. @Fgruntjes thank you again for the time you invested into this! You did a great job and we are really happy that we can take it over and make it officially supported by our team. Thank you! |
@LKaemmerling thnx for your comment. Ill close this PR so you guys can create a new one to take over. |
So I made a very basic implementation of a cloud provider for Hetzner Cloud. Now I am not sure if I did everything correctly so please let me know where you see any issues / improvements.
Once you are satisfied with the implementation I will add test cases to finalize.