From 33c39175dfd8592fa805af8876a30edeec9a2642 Mon Sep 17 00:00:00 2001 From: Serge Logvinov Date: Wed, 8 May 2024 17:25:22 +0300 Subject: [PATCH] [occm] multizone race condition We cannot definitively determine whether a node exists within our zone without the provider ID string. --- pkg/openstack/instances.go | 21 ----- pkg/openstack/instances_test.go | 72 ---------------- pkg/openstack/instancesv2.go | 25 +++++- pkg/openstack/utils.go | 53 ++++++++++++ pkg/openstack/utils_test.go | 144 ++++++++++++++++++++++++++++++++ 5 files changed, 221 insertions(+), 94 deletions(-) create mode 100644 pkg/openstack/utils.go create mode 100644 pkg/openstack/utils_test.go diff --git a/pkg/openstack/instances.go b/pkg/openstack/instances.go index 60c7745411..b8c124810f 100644 --- a/pkg/openstack/instances.go +++ b/pkg/openstack/instances.go @@ -448,27 +448,6 @@ func isValidLabelValue(v string) bool { return true } -// If Instances.InstanceID or cloudprovider.GetInstanceProviderID is changed, the regexp should be changed too. -var providerIDRegexp = regexp.MustCompile(`^` + ProviderName + `://([^/]*)/([^/]+)$`) - -// instanceIDFromProviderID splits a provider's id and return instanceID. -// A providerID is build out of '${ProviderName}:///${instance-id}' which contains ':///'. -// or '${ProviderName}://${region}/${instance-id}' which contains '://'. -// See cloudprovider.GetInstanceProviderID and Instances.InstanceID. -func instanceIDFromProviderID(providerID string) (instanceID string, region string, err error) { - - // https://github.com/kubernetes/kubernetes/issues/85731 - if providerID != "" && !strings.Contains(providerID, "://") { - providerID = ProviderName + "://" + providerID - } - - matches := providerIDRegexp.FindStringSubmatch(providerID) - if len(matches) != 3 { - return "", "", fmt.Errorf("ProviderID \"%s\" didn't match expected format \"openstack://region/InstanceID\"", providerID) - } - return matches[2], matches[1], nil -} - // AddToNodeAddresses appends the NodeAddresses to the passed-by-pointer slice, // only if they do not already exist func AddToNodeAddresses(addresses *[]v1.NodeAddress, addAddresses ...v1.NodeAddress) { diff --git a/pkg/openstack/instances_test.go b/pkg/openstack/instances_test.go index 0d4b2e1c92..eb3955c891 100644 --- a/pkg/openstack/instances_test.go +++ b/pkg/openstack/instances_test.go @@ -22,7 +22,6 @@ import ( "reflect" "testing" - "github.com/stretchr/testify/assert" v1 "k8s.io/api/core/v1" ) @@ -190,74 +189,3 @@ func TestSortNodeAddressesWithMultipleCIDRs(t *testing.T) { executeSortNodeAddressesTest(t, addressSortOrder, want) } - -func Test_instanceIDFromProviderID(t *testing.T) { - type args struct { - providerID string - } - tests := []struct { - name string - args args - wantInstanceID string - wantRegion string - wantErr bool - }{ - { - name: "it parses region & instanceID correctly from providerID", - args: args{ - providerID: "openstack://us-east-1/testInstanceID", - }, - wantInstanceID: "testInstanceID", - wantRegion: "us-east-1", - wantErr: false, - }, - { - name: "it parses instanceID if providerID has empty protocol & no region", - args: args{ - providerID: "/testInstanceID", - }, - wantInstanceID: "testInstanceID", - wantRegion: "", - wantErr: false, - }, - { - name: "it returns error in case of invalid providerID format with no region", - args: args{ - providerID: "openstack://us-east-1-testInstanceID", - }, - wantInstanceID: "", - wantRegion: "", - wantErr: true, - }, - { - name: "it parses correct instanceID in case the region name is the empty string", - args: args{ - providerID: "openstack:///testInstanceID", - }, - wantInstanceID: "testInstanceID", - wantRegion: "", - wantErr: false, - }, - { - name: "it appends openstack:// in case of missing protocol in providerID", - args: args{ - providerID: "us-east-1/testInstanceID", - }, - wantInstanceID: "testInstanceID", - wantRegion: "us-east-1", - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - gotInstanceID, gotRegion, err := instanceIDFromProviderID(tt.args.providerID) - assert.Equal(t, tt.wantInstanceID, gotInstanceID) - assert.Equal(t, tt.wantRegion, gotRegion) - if tt.wantErr == true { - assert.ErrorContains(t, err, "didn't match expected format") - } else { - assert.NoError(t, err) - } - }) - } -} diff --git a/pkg/openstack/instancesv2.go b/pkg/openstack/instancesv2.go index 3e6f770f7e..f61a4e3978 100644 --- a/pkg/openstack/instancesv2.go +++ b/pkg/openstack/instancesv2.go @@ -79,9 +79,20 @@ func (os *OpenStack) instancesv2() (*InstancesV2, bool) { // InstanceExists indicates whether a given node exists according to the cloud provider func (i *InstancesV2) InstanceExists(ctx context.Context, node *v1.Node) (bool, error) { + if i.regionProviderID { + if node.Spec.ProviderID == "" { + return false, fmt.Errorf("node %s has empty ProviderID, it should be initialized first", node.Name) + } + + if isNodeUnmanaged(node.Spec.ProviderID) { + klog.V(4).InfoS("Omitting unmanaged node", "node", klog.KObj(node)) + return true, nil + } + } + _, err := i.getInstance(ctx, node) if err == cloudprovider.InstanceNotFound { - klog.V(6).Infof("instance not found for node: %s", node.Name) + klog.V(6).InfoS("Node is not found in cloud provider", "node", klog.KObj(node)) return false, nil } @@ -94,6 +105,17 @@ func (i *InstancesV2) InstanceExists(ctx context.Context, node *v1.Node) (bool, // InstanceShutdown returns true if the instance is shutdown according to the cloud provider. func (i *InstancesV2) InstanceShutdown(ctx context.Context, node *v1.Node) (bool, error) { + if i.regionProviderID { + if node.Spec.ProviderID == "" { + return false, fmt.Errorf("node %s has empty ProviderID, it should be initialized first", node.Name) + } + + if isNodeUnmanaged(node.Spec.ProviderID) { + klog.V(4).InfoS("Omitting unmanaged node", "node", klog.KObj(node)) + return false, nil + } + } + server, err := i.getInstance(ctx, node) if err != nil { return false, err @@ -101,6 +123,7 @@ func (i *InstancesV2) InstanceShutdown(ctx context.Context, node *v1.Node) (bool // SHUTOFF is the only state where we can detach volumes immediately if server.Status == instanceShutoff { + klog.V(6).InfoS("Node is shutoff", "node", klog.KObj(node)) return true, nil } diff --git a/pkg/openstack/utils.go b/pkg/openstack/utils.go new file mode 100644 index 0000000000..e81b460981 --- /dev/null +++ b/pkg/openstack/utils.go @@ -0,0 +1,53 @@ +/* +Copyright 2014 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package openstack + +import ( + "fmt" + "regexp" + "strings" +) + +// If Instances.InstanceID or cloudprovider.GetInstanceProviderID is changed, the regexp should be changed too. +var providerIDRegexp = regexp.MustCompile(`^` + ProviderName + `://([^/]*)/([^/]+)$`) + +// instanceIDFromProviderID splits a provider's id and return instanceID. +// A providerID is build out of '${ProviderName}:///${instance-id}' which contains ':///'. +// or '${ProviderName}://${region}/${instance-id}' which contains '://'. +// See cloudprovider.GetInstanceProviderID and Instances.InstanceID. +func instanceIDFromProviderID(providerID string) (instanceID string, region string, err error) { + + // https://github.com/kubernetes/kubernetes/issues/85731 + if providerID != "" && !strings.Contains(providerID, "://") { + providerID = ProviderName + "://" + providerID + } + + matches := providerIDRegexp.FindStringSubmatch(providerID) + if len(matches) != 3 { + return "", "", fmt.Errorf("ProviderID \"%s\" didn't match expected format \"openstack://region/InstanceID\"", providerID) + } + return matches[2], matches[1], nil +} + +// Return true if instance is not openstack. +func isNodeUnmanaged(providerID string) bool { + if providerID != "" { + return strings.Contains(providerID, "://") && !strings.HasPrefix(providerID, ProviderName+"://") + } + + return false +} diff --git a/pkg/openstack/utils_test.go b/pkg/openstack/utils_test.go new file mode 100644 index 0000000000..241cf5d5d7 --- /dev/null +++ b/pkg/openstack/utils_test.go @@ -0,0 +1,144 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package openstack + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func Test_instanceIDFromProviderID(t *testing.T) { + type args struct { + providerID string + } + tests := []struct { + name string + args args + wantInstanceID string + wantRegion string + wantErr bool + }{ + { + name: "it parses region & instanceID correctly from providerID", + args: args{ + providerID: "openstack://us-east-1/testInstanceID", + }, + wantInstanceID: "testInstanceID", + wantRegion: "us-east-1", + wantErr: false, + }, + { + name: "it parses instanceID if providerID has empty protocol & no region", + args: args{ + providerID: "/testInstanceID", + }, + wantInstanceID: "testInstanceID", + wantRegion: "", + wantErr: false, + }, + { + name: "it returns error in case of invalid providerID format with no region", + args: args{ + providerID: "openstack://us-east-1-testInstanceID", + }, + wantInstanceID: "", + wantRegion: "", + wantErr: true, + }, + { + name: "it parses correct instanceID in case the region name is the empty string", + args: args{ + providerID: "openstack:///testInstanceID", + }, + wantInstanceID: "testInstanceID", + wantRegion: "", + wantErr: false, + }, + { + name: "it appends openstack:// in case of missing protocol in providerID", + args: args{ + providerID: "us-east-1/testInstanceID", + }, + wantInstanceID: "testInstanceID", + wantRegion: "us-east-1", + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotInstanceID, gotRegion, err := instanceIDFromProviderID(tt.args.providerID) + assert.Equal(t, tt.wantInstanceID, gotInstanceID) + assert.Equal(t, tt.wantRegion, gotRegion) + if tt.wantErr == true { + assert.ErrorContains(t, err, "didn't match expected format") + } else { + assert.NoError(t, err) + } + }) + } +} + +func Test_instanceNodeUnmanaged(t *testing.T) { + tests := []struct { + name string + providerID string + wantResult bool + }{ + { + name: "openstack node, providerID not set yet", + providerID: "", + wantResult: false, + }, + { + name: "openstack node in case of invalid providerID format with no region", + providerID: "openstack://us-east-1-testInstanceID", + wantResult: false, + }, + { + name: "openstack node, it parses instanceID has empty protocol & no region", + providerID: "/testInstanceID", + wantResult: false, + }, + { + name: "openstack node, it parses correct instanceID in case the region name is the empty string", + providerID: "openstack:///testInstanceID", + wantResult: false, + }, + { + name: "openstack node, it parses correct instanceID with region name", + providerID: "openstack://region/testInstanceID", + wantResult: false, + }, + { + name: "openstack node in case of missing protocol in providerID", + providerID: "us-east-1/testInstanceID", + wantResult: false, + }, + { + name: "non openstack node, providerID has non openstack protocol", + providerID: "provider:///us-east-1-testInstanceID", + wantResult: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + res := isNodeUnmanaged(tt.providerID) + assert.Equal(t, tt.wantResult, res) + }) + } +}