Add functionality to get instance id from node name by regexp

[mfranczy]

This PR allows to fetch instance id (VM name) from node name by regexp.
It eliminates the hardcoded assumption https://github.com/kubevirt/cloud-provider-kubevirt/compare/main...mfranczy:instance-regexp?expand=1#diff-f1870dea0d991df0bf3f60c36b21ec968cfbc5a0e77a6e55e17c587dfaf16811L236

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign nirarg after the PR has been reviewed.
You can assign the PR to them by writing /assign @nirarg in a comment when ready.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[rmohr]

@mfranczy did you see #48 ? Intuitively I would expect things to be solved by labels and not by regexp. I am however not entirely up-to-date regarding to this repo.

[mfranczy]

@mfranczy did you see #48 ? Intuitively I would expect things to be solved by labels and not by regexp. I am however not entirely up-to-date regarding to this repo.

yes I saw the issue, but this PR is not about load balancers and network traffic. It's about node initialization stage, once kubelet creates a node object the cloud provider has to find the node's instance (Virtual Machine), so based on the node name there is an attempt to fetch a VM.

Currently there is an assumption in the code

// instanceIDFromNodeName extracts the instance ID from a given node name. In
// case the node name is a FQDN the hostname will be extracted as instance ID.
// If the regexp is empty instance id will have the same value as a node name.
func instanceIDFromNodeName(nodeName string) string {
	data := strings.SplitN(nodeName, ".", 2)
	if instanceRegexp == "" {
	return data[0]

that works well with Gardener but not with other kubernetes providers like Kubermatic and probably could break at some point with Hypershift too.

For Kubermatic, a node name is equal to virtual machine name so I introduced this regexp field to offer some flexibility in terms of matching a node with virtual machine if other providers has a different VM naming pattern (other than node name = vm name).

[mfranczy]

@rmohr speaking about the issue with LB, I think we will have to write another controller that watches endpoints on a tenant cluster and updates labels accordingly on infra cluster to route the traffic correctly. Although this probably should be discussed on some meeting with you, however, I am currently preparing a PoC.

[rmohr]

that works well with Gardener but not with other kubernetes providers like Kubermatic and probably could break at some point with Hypershift too.

For Kubermatic, a node name is equal to virtual machine name so I introduced this regexp field to offer some flexibility in terms of matching a node with virtual machine if other providers has a different VM naming pattern (other than node name = vm name).

I think I kind of understand the issue. Could you add examples of the node-name and vm name correlation? If possible for both, gardener and kubermatic. I still wonder if there would be a better way, e.g. based on labels to find correlating objects.

[mfranczy]

Could you add examples of the node-name and vm name correlation? If possible for both, gardener and kubermatic.

yes, I will provide examples

[rmohr]

/cc @nirarg

Adding Nir, he spent more time on the repo than I did :)

[mfranczy]

I still wonder if there would be a better way, e.g. based on labels to find correlating objects.

you're right, seems like there will be a better way than regexp to identify a node. Based on labels it could work. One potential solution could be that k8s providers (once they start kubelet) could pass a label node.kubernetes.io/instance-id=<vm-name> (or something like that) to kubelet's --node-labels arg. Later in the cloud provider we could easily identify the vm. Altough let me check that and other ideas deeply.

[mfranczy]

After investigating the issue more deeply i discovered that cloud provider kubevirt implements Instances interface (that is for in-tree cloud providers).

Because of that we have some methods that are never called like CurrentNodeName (kubelet calls this only for in-tree).
We should drop the Instances and implement InstancesV2 which is an interface for external cloud providers.

I was wrong by proposing a regexp for node parsing.

Generally speaking by default kubelet registers nodes with name equal to the OS hostname or takes the value from --hostname-override argument.

Considering the above statement, I think we should follow the checks below:

1. Node name is the same as a VM name - simple scenario we should just fetch a VM based on the node name
2. Node name is the same as a VM hostname - in case somebody sets a spec.hostname in a VM template, the node will get registered with the name equal to the vm hostname. As @rmohr suggested we could filter VMs over a label, for instance over kubevirt.io/vm-hostname
3. Node name is different than a VM name and hostname - that can happen if kubelet has a hostname-override argument set.
   We could fetch a vm over a custom node label (when kubelet register a node it allows to add custom labels over node-labels arg, so we could add node.kubernetes.io/instance-id=<vm-name> to the node)
4. Anything different results in "Node instance not found"

@rmohr @nirarg do you have any concerns about things I wrote above? If not I will close this PR and start implementing InstancesV2 interface.

[nirarg]

@rmohr @nirarg do you have any concerns about things I wrote above? If not I will close this PR and start implementing InstancesV2 interface.

Hi @mfranczy ,
That sound good for me, I think you can start with implementing InstancesV2