search

syself / hetzner-cloud-controller-manager

Kubernetes cloud-controller-manager for Hetzner Cloud & Hetzner Robot. Enables the usage of Hetzner Dedicated Servers and Hetzner Cloud Servers
Apache License 2.0
10 stars 7 forks source link

[WIP] Support vSwitch subnet #6

Closed sergelogvinov closed 2 years ago

sergelogvinov commented 2 years ago

Hello, Thank you for what you have done.

I am using the local network for kubernetes communication. I've added definition local-ip in this case (controllers: cloud-node,cloud-node-lifecycle)

So if you have any ideas, please leave me a message...

janiskemper commented 2 years ago

Hi @sergelogvinov, Thank you very much for your PR! I want to apologize for answering so late, I just didn't see it somehow.

In general, I would say that your work looks good!

You made me realize that I messed up regions and failure domains which I have changed in our master branch. In general, I would say that making this PR against the master branch would be the way to go. dev branch I created only to test some stuff that I don't want to have in master.

Unfortunately, you have changed too many parts of the code. The part with the "bm-" prefix has to stay for us unless you also change the respective code in our Cluster API Provider Hetzner. This is where this CCM mainly comes from.

We also want to have regions like eu-central because that corresponds with Hetzner's networking. Used with Kubenretes topology, that can be quite powerful. Have a look at my latest changes in master branch to see whether that suits you.

Could you please create a new branch with all of your changes regarding the core functionality you want to add and make a PR against master branch? Then we could test it also with Cluster API!

sergelogvinov commented 2 years ago

Thank you for your feedback.)

Yep, the last commits are not about this case. I agree. I'll create a new one soon.

But I still cannot understand hetzner's region/zone changes. Usually, the zone includes region string, which I did (revert from upstream).

janiskemper commented 2 years ago

Hetzner has the region nbg1 and zone nbg1-dc1. We have region eu-central and zone nbg1 (for HCloud) or nbg1-dc1 (for Bare metal). This makes more sense according to us - as network zones are also eu-central.

sergelogvinov commented 2 years ago

What do you think if CCM will label the baremetal servers by key instance.hetzner.cloud/is-root-server="true" and it helps to avoid the "bm-" prefix?

janiskemper commented 2 years ago

We have some API requests to the CCM that give a provider ID and want to know some information about the server, right? So if we have only the providerID, we have only two options:

  1. Have different kinds of providerIDs so that we can differentiate bm and hcloud.
  2. Make always requests to HCloud and only if the server is not found then try making requests to robot. I think the label won't help much in either of the cases (although it also doesn't do any bad of course). We chose the first option to avoid useless API calls, but I would also be open to the second if somebody implemented the whole thing in that way 😃
sergelogvinov commented 2 years ago

Yep, API calls is the issue... We can add "node list cache" for robot-servers to avoid it.

The label instance.hetzner.cloud/is-root-server helps only for pod affinity. For instance you want to run daemonsets only on baremetal or use hetzner CSI only in the hcloud (https://github.com/sergelogvinov/terraform-talos/blob/ebc25c29f2edfa73955995d5686fa1b742a9ffa4/hetzner/deployments/hcloud-csi.yaml#L237).

janiskemper commented 2 years ago

That's true. A cache would avoid that issue in most of the cases! And yes, I'm sure the label makes sense in multiple cases!

github-actions[bot] commented 2 years ago

This PR has been marked as stale because it has not had recent activity. The bot will close the PR if no further action occurs.