[occm] Multi region openstack cluster #2595

sergelogvinov · 2024-05-14T14:42:15Z

What this PR does / why we need it:

Openstack CCM multi region support, if it has one Identity provider.

Which issue this PR fixes(if applicable):
fixes #1924

Special notes for reviewers:

CCM config changes:

[Global]
auth-url=https://auth.openstack.example.com/v3/
region=REGION1
# new param 'regions' can be specified multiple times
regions=REGION1
regions=REGION2
regions=REGION3

Optionally can be set in cloud.conf

clouds:
  kubernetes:
    auth:
      auth_url: https://auth.openstack.example.com/v3
    region_name: "REGION1"
    regions:
      - REGION1
      - REGION2
      - REGION3

During the initialization process, OCCM checks for the existence of providerID. If providerID does not exist, it defaults to using node.name, as it did previously. Additionally, if the node has the label topology.kubernetes.io/region, OCCM will prioritize using this region as the first one to check. This approach ensures that in the event of a region outage, OCCM can continue to function.

In addition, we can assist CCM in locating the node by providing kubelet parameters:

--provider-id=openstack:///$InstanceID - InstanceID exists in metadata
--provider-id=openstack://$REGION/$InstanceID - if you can define the region (by default meta server does not have this information)
--node-labels=topology.kubernetes.io/region=$REGION set preferred REGION in label, OCCM will then prioritize searching for the node in this specified region

Release note:

NONE

k8s-ci-robot · 2024-05-14T14:42:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kayrus for approval. For more information see the Kubernetes Code Review Process.

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

k8s-ci-robot · 2024-05-14T14:42:25Z

Hi @sergelogvinov. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

sergelogvinov · 2024-05-14T14:43:47Z

@mdbooth can you take a look on this PR.
Probably I need to add more configuration checks.

Thanks.

mdbooth

I haven't taken a deep look into this, but I much prefer this in principal: it only tells cloud-provider things we know to be true. This makes me much more confident that this will continue to work correctly as cloud-provider evolves.

Would you still run multiple CCMs, or switch to a single active CCM?

mdbooth · 2024-05-14T15:29:52Z

pkg/openstack/instancesv2.go

+	for _, region := range os.regions {
+		opt := os.epOpts
+		opt.Region = region
+
+		compute[region], err = client.NewComputeV2(os.provider, opt)
+		if err != nil {
+			klog.Errorf("unable to access compute v2 API : %v", err)
+			return nil, false
+		}
+
+		network[region], err = client.NewNetworkV2(os.provider, opt)
+		if err != nil {
+			klog.Errorf("unable to access network v2 API : %v", err)
+			return nil, false
+		}


@pierreprinetti how much work is performed when initialising a new service client? Is it local-only, or do we have to go back to keystone?

I might be inclined to intialise this lazily anyway, tbh.

similar thought, maybe init them until real usage?

I've had one issue in proxmox with lazy initialization. The regions cannot exist, and during the rollout of OCCM, it starts without errors. The kubernetes administrator will think that all configuration is correct.

So we can check all regions here and crush if needed. What do you think?

mdbooth · 2024-05-14T15:52:29Z

pkg/openstack/instancesv2.go

+	if node.Spec.ProviderID == "" {
+		return i.getInstanceByName(node)
 	}

 	instanceID, instanceRegion, err := instanceIDFromProviderID(node.Spec.ProviderID)
 	if err != nil {
-		return nil, err
+		return nil, "", err
+	}
+
+	if instanceRegion == "" {
+		return i.getInstanceByID(instanceID, node.Labels[v1.LabelTopologyRegion])


I'd probably be a bit more explicit with where we're looking for stuff here. IIUC there are 2 possible places we can get a specific region from:

providerID

LabelTopologyRegion

Both may be unset because the node has not yet been adopted by the node-controller.

providerID may not contain a region either because it was set before we became multi-region, or because it was set by kubelet without a region and it's immutable.

But the end result is that either we know the region or we don't. If we know the region we should look only in that region. If we don't know the region we should look everywhere.

How about logic something like:

instanceID, instanceRegion, err := instanceIDFromProviderID(node.Spec.ProviderID) ..err omitted... if instanceRegion == "" { instanceRegion = node.Labels[v1.LabelTopologyRegion] } var searchRegions []string if instanceRegion != "" { if !slices.Contains(i.regions, instanceRegion) { return ...bad region error... } searchRegions = []string{instanceRegion} } else { searchRegions = ..all the regions, preferred first... } for region := range searchRegions { mc := ... if instanceID != "" { getInstanceByID() } else { getInstanceByName() } mc.ObserveRequest() }

Thank you, this very very good idea.
I've changed the implementation. But one thought - i cannot trust LabelTopologyRegion, something can change it and node-lifecycle will remove node (for instance on reboot/upgrade event)...

So i can use LabelTopologyRegion only as prefered-region. And check this region first.
Thanks.

MatthieuFin · 2024-05-14T19:38:23Z

Hi @sergelogvinov
I propose an implementation of multi cloud support for cinder-csi-plugin, which offer multiple openstack clusters support, not only multiple regions, I haven't take look of occm implementation yet, but is it possible to adapt it to support multiple cloud definitions instead of only multiple regions ?

sergelogvinov · 2024-05-15T05:09:39Z

Hi @sergelogvinov I propose an implementation of multi cloud support for cinder-csi-plugin, which offer multiple openstack clusters support, not only multiple regions, I haven't take look of occm implementation yet, but is it possible to adapt it to support multiple cloud definitions instead of only multiple regions ?

Thank you for this PR, it is very interesting. Can we have a call/chat in slack #provider-openstack (Serge Logvinov)?

jichenjc · 2024-05-15T08:08:50Z

/ok-to-test

jichenjc · 2024-05-15T08:09:56Z

/ok-to-test

docs/openstack-cloud-controller-manager/using-openstack-cloud-controller-manager.md

jichenjc · 2024-05-15T08:50:57Z

pkg/openstack/instancesv2.go

+	for _, region := range os.regions {
+		opt := os.epOpts
+		opt.Region = region
+
+		compute[region], err = client.NewComputeV2(os.provider, opt)
+		if err != nil {
+			klog.Errorf("unable to access compute v2 API : %v", err)
+			return nil, false
+		}
+
+		network[region], err = client.NewNetworkV2(os.provider, opt)
+		if err != nil {
+			klog.Errorf("unable to access network v2 API : %v", err)
+			return nil, false
+		}


similar thought, maybe init them until real usage?

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label May 14, 2024

k8s-ci-robot requested review from anguslees and dulek May 14, 2024 14:42

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 14, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 14, 2024

mdbooth reviewed May 14, 2024

View reviewed changes

sergelogvinov force-pushed the multi-regionl-only-openstack branch from aee43df to fa2bd50 Compare May 15, 2024 07:59

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 15, 2024

jichenjc reviewed May 15, 2024

View reviewed changes

support multi region cluster

46aebfb

sergelogvinov force-pushed the multi-regionl-only-openstack branch from fa2bd50 to 46aebfb Compare May 15, 2024 09:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[occm] Multi region openstack cluster #2595

[occm] Multi region openstack cluster #2595

sergelogvinov commented May 14, 2024 •

edited

k8s-ci-robot commented May 14, 2024

k8s-ci-robot commented May 14, 2024

sergelogvinov commented May 14, 2024

mdbooth left a comment

mdbooth May 14, 2024

jichenjc May 15, 2024

sergelogvinov May 15, 2024

mdbooth May 14, 2024

sergelogvinov May 15, 2024

MatthieuFin commented May 14, 2024

sergelogvinov commented May 15, 2024

jichenjc commented May 15, 2024

jichenjc commented May 15, 2024

jichenjc May 15, 2024

[occm] Multi region openstack cluster #2595

Are you sure you want to change the base?

[occm] Multi region openstack cluster #2595

Conversation

sergelogvinov commented May 14, 2024 • edited

k8s-ci-robot commented May 14, 2024

k8s-ci-robot commented May 14, 2024

sergelogvinov commented May 14, 2024

mdbooth left a comment

Choose a reason for hiding this comment

mdbooth May 14, 2024

Choose a reason for hiding this comment

jichenjc May 15, 2024

Choose a reason for hiding this comment

sergelogvinov May 15, 2024

Choose a reason for hiding this comment

mdbooth May 14, 2024

Choose a reason for hiding this comment

sergelogvinov May 15, 2024

Choose a reason for hiding this comment

MatthieuFin commented May 14, 2024

sergelogvinov commented May 15, 2024

jichenjc commented May 15, 2024

jichenjc commented May 15, 2024

jichenjc May 15, 2024

Choose a reason for hiding this comment

sergelogvinov commented May 14, 2024 •

edited