vmware / cloud-provider-for-cloud-director

Kubernetes External Cloud Provider for VMware Cloud Director
Other
20 stars 30 forks source link

VKCI-254,255,257: look at multiple OVDCs in order to register the VM #335

Closed arunmk closed 10 months ago

arunmk commented 10 months ago

This change brings in a the multiAZ aspect to the CPI. The CPI registers nodes, and the nodes are in multiple vApps. So the vApps need to be discovered in multiple OVDCs. The representation of OVDCs is in a configmap which has the following example format

---
apiVersion: v1
kind: ConfigMap
metadata:
  name: vcloud-capvcd-zones
  namespace: kube-system
data:
  vcloud-cse-zones.yaml: |+
    zoneType: dcgroup
    zones:
    - name: zone1
      ovdcName: <ovdc of zone1>
    - name: zone2
      ovdcName: <ovdc of zone2>
    - name: zone3
      ovdcName: <ovdc of zone3>
---

To trigger usage of this zoneMap, and also to ensure backward compatibility, this feature is triggered by a flag in the CPI configmap variable isZoneEnabledCluster:

apiVersion: v1
kind: ConfigMap
metadata:
  name: vcloud-ccm-configmap
  namespace: kube-system
data:
  vcloud-ccm-config.yaml: |+
    vcd:
      host: <vcd api endpoint>
      org: orgName
      vdc: ovdcName
      isZoneEnabledCluster: true

In order to be able to add the label topology.k8s.io/zones, CPI needs the InstancesV2 interface to be implemented. That is part of this PR as well.


This change is Reviewable

arunmk commented 10 months ago

pkg/ccm/vminfocache.go line 139 at r1 (raw file):

Previously, sahithi (Sahithi Ayloo) wrote…
I think this is also not needed. There should be query API to just get the VM record, given vmname and vAppname. No need to get the vApp record list.

The vAppName is not known because it also includes the nodepool name. The nodepool name is not available here.

However, as mentioned earlier, there is a regular-expression-based search that can be performed. That is covered by another ticket.

arunmk commented 10 months ago

pkg/ccm/vminfocache.go line 142 at r1 (raw file):

Previously, Anirudh9794 wrote…
Can we not use the API http://vcd/api/query?type=vApp to retrieve all the VApps with the prefix?

This is handled by ticket https://jira.eng.vmware.com/browse/VKCI-283

arunmk commented 10 months ago

pkg/ccm/instancesv2.go line 132 at r1 (raw file):

Previously, Anirudh9794 wrote…
Is it the responsibility of the user to update the configmap when OVDCs are renamed ? or will it be handled within CPI in a separate change set ?

It is the responsibility of CAPVCD. However that portion is not yet implemented.

arunmk commented 10 months ago

pkg/ccm/vminfocache.go line 107 at r1 (raw file):

Previously, sahithi (Sahithi Ayloo) wrote…
Did we check if there exists simpler queryVM call in an org or ovdc? Like how we see in our UI. UI has vApp page and also VM page that only shows just VMs. It must be using some query API. If VM match found we can just check if its parent vapp matches our vapp name prefix

This will be handled by ticket https://jira.eng.vmware.com/browse/VKCI-283 and will not be handled in this commit.

arunmk commented 10 months ago

pkg/ccm/vminfocache.go line 125 at r1 (raw file):

Previously, sahithi (Sahithi Ayloo) wrote…
+1

This is not clusterName. This is specifically vApp name which is clusterName

This can become vAppName, but then we lose the detail that this is related to the cluster.

arunmk commented 10 months ago

pkg/ccm/vminfocache.go line 125 at r1 (raw file):

Previously, arunmk (Arun M. Krishnakumar) wrote…
This is not clusterName. This is specifically vApp name which is clusterName__ This can become vAppName, but then we lose the detail that this is related to the cluster.

clusterName_<OVDCID>_<nodePool>

arunmk commented 10 months ago

pkg/ccm/vminfocache.go line 139 at r1 (raw file):

Previously, arunmk (Arun M. Krishnakumar) wrote…
The vAppName is not known because it also includes the nodepool name. The nodepool name is not available here. However, as mentioned earlier, there is a regular-expression-based search that can be performed. That is covered by another ticket.

https://jira.eng.vmware.com/browse/VKCI-283

arunmk commented 10 months ago
  1. CAPVCD is expected to create the zoneMap. However that is not yet implemented in the PoC and has open tickets regarding it.
  2. isZoneEnabledCluster=true will be set up by CSE backend, which is the one that installs CPI. CSE backend also configures the configmap into CPI.
  3. The zoneMap can, in some distant future, have a mapping of one zone to many OVDCs. So it is structured that way. Having a map for that will make things less readable in that structure.
arunmk commented 10 months ago

pkg/ccm/vminfocache.go line 194 at r1 (raw file):

Previously, sahithi (Sahithi Ayloo) wrote…
Reviewing the CPI code for the first time, excuse my lack of knowledge here. This call SearchVMsAcrossVdcs() - doesn't it need to be conditional, based on whether the cluster is MZ enabled?

Within the function, the zoneMap is used if it is not nil (which is set up only if the isZoneEnabledCluster is set up). If the zoneMap is nil, the ovdcName is used (as specified earlier).

arunmk commented 10 months ago

@sahithi I had some merge issues and have fixed them. Can you please take a look again.