Skip to content

Commit

Permalink
[occm] multizone race condition
Browse files Browse the repository at this point in the history
We cannot definitively determine whether a node exists within our zone without the provider ID string.
  • Loading branch information
sergelogvinov committed May 10, 2024
1 parent 2f186d6 commit 68fbb88
Show file tree
Hide file tree
Showing 5 changed files with 221 additions and 94 deletions.
21 changes: 0 additions & 21 deletions pkg/openstack/instances.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
72 changes: 0 additions & 72 deletions pkg/openstack/instances_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"reflect"
"testing"

"github.com/stretchr/testify/assert"
v1 "k8s.io/api/core/v1"
)

Expand Down Expand Up @@ -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)
}
})
}
}
25 changes: 24 additions & 1 deletion pkg/openstack/instancesv2.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -94,13 +105,25 @@ 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
}

// 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
}

Expand Down
53 changes: 53 additions & 0 deletions pkg/openstack/utils.go
Original file line number Diff line number Diff line change
@@ -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
}
144 changes: 144 additions & 0 deletions pkg/openstack/utils_test.go
Original file line number Diff line number Diff line change
@@ -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)
})
}
}

0 comments on commit 68fbb88

Please sign in to comment.