Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨Make machine's providerID consistent with node providerID #1730

Merged
merged 1 commit into from
May 26, 2020
Merged

✨Make machine's providerID consistent with node providerID #1730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/v1alpha2/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions api/v1alpha3/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,9 @@ type Instance struct {

// The tags associated with the instance.
Tags map[string]string `json:"tags,omitempty"`

alexander-demicev marked this conversation as resolved.
Show resolved Hide resolved
// Availability zone of instance
AvailabilityZone string `json:"availabilityZone,omitempty"`
}

// RootVolume encapsulates the configuration options for the root volume
Expand Down
3 changes: 3 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,9 @@ spec:
- type
type: object
type: array
availabilityZone:
description: Availability zone of instance
type: string
ebsOptimized:
description: Indicates whether the instance is optimized for Amazon
EBS I/O.
Expand Down
3 changes: 1 addition & 2 deletions controllers/awsmachine_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package controllers

import (
"context"
"fmt"

"github.com/go-logr/logr"
"github.com/pkg/errors"
Expand Down Expand Up @@ -411,7 +410,7 @@ func (r *AWSMachineReconciler) reconcileNormal(_ context.Context, machineScope *
}

// Make sure Spec.ProviderID is always set.
machineScope.SetProviderID(fmt.Sprintf("aws:////%s", instance.ID))
machineScope.SetProviderID(instance.ID, instance.AvailabilityZone)

// See https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-lifecycle.html

Expand Down
6 changes: 4 additions & 2 deletions pkg/cloud/scope/machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package scope
import (
"context"
"encoding/base64"
"fmt"

"github.com/go-logr/logr"
"github.com/pkg/errors"
Expand Down Expand Up @@ -138,8 +139,9 @@ func (m *MachineScope) GetProviderID() string {
}

// SetProviderID sets the AWSMachine providerID in spec.
func (m *MachineScope) SetProviderID(v string) {
m.AWSMachine.Spec.ProviderID = pointer.StringPtr(v)
func (m *MachineScope) SetProviderID(instanceID, availabilityZone string) {
Copy link
Member

@enxebre enxebre May 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd appreciate if we can put some units for this?

Copy link
Member

@enxebre enxebre May 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might seem trivial enough as to not deserve a test, but the func is actually relying in a handwritten fmt "arbitrary" format and an external given implementation for StringPtr. It’s very cheap fo us to put a unit and it becomes fairly valuable if some one considers to change the impl details in the future. Also it makes way easier to visualise at a glange what provider should look like for the different cases for someone with less context.

providerID := fmt.Sprintf("aws:///%s/%s", availabilityZone, instanceID)
m.AWSMachine.Spec.ProviderID = pointer.StringPtr(providerID)
}

// GetInstanceID returns the AWSMachine instance state from the status.
Expand Down
21 changes: 21 additions & 0 deletions pkg/cloud/scope/machine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,3 +201,24 @@ func TestSetSecretARN(t *testing.T) {
t.Fatalf("prefix does not equal %s: %s", prefix, val)
}
}

func TestSetProviderID(t *testing.T) {
scope, err := setupMachineScope()
if err != nil {
t.Fatal(err)
}

scope.SetProviderID("test-id", "test-zone-1a")
providerID := *scope.AWSMachine.Spec.ProviderID
expectedProviderID := "aws:///test-zone-1a/test-id"
if providerID != expectedProviderID {
t.Fatalf("Expected providerID %s, got %s", expectedProviderID, providerID)
}

scope.SetProviderID("test-id", "")
providerID = *scope.AWSMachine.Spec.ProviderID
expectedProviderID = "aws:////test-id"
if providerID != expectedProviderID {
t.Fatalf("Expected providerID %s, got %s", expectedProviderID, providerID)
}
}
2 changes: 2 additions & 0 deletions pkg/cloud/services/ec2/instances.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,8 @@ func (s *Service) SDKToInstance(v *ec2.Instance) (*infrav1.Instance, error) {

i.Addresses = s.getInstanceAddresses(v)

i.AvailabilityZone = aws.StringValue(v.Placement.AvailabilityZone)

return i, nil
}

Expand Down
21 changes: 21 additions & 0 deletions pkg/cloud/services/ec2/instances_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ func TestInstanceIfExists(t *testing.T) {
name: "instance exists",
instanceID: "id-1",
expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
az := "test-zone-1a"
m.DescribeInstances(gomock.Eq(&ec2.DescribeInstancesInput{
InstanceIds: []*string{aws.String("id-1")},
})).
Expand Down Expand Up @@ -116,6 +117,9 @@ func TestInstanceIfExists(t *testing.T) {
},
},
},
Placement: &ec2.Placement{
AvailabilityZone: &az,
},
},
},
},
Expand Down Expand Up @@ -269,6 +273,8 @@ func TestCreateInstance(t *testing.T) {
},
}

az := "test-zone-1a"

testcases := []struct {
name string
machine clusterv1.Machine
Expand Down Expand Up @@ -362,6 +368,9 @@ func TestCreateInstance(t *testing.T) {
},
},
},
Placement: &ec2.Placement{
AvailabilityZone: &az,
},
},
},
}, nil)
Expand Down Expand Up @@ -475,6 +484,9 @@ func TestCreateInstance(t *testing.T) {
},
},
},
Placement: &ec2.Placement{
AvailabilityZone: &az,
},
},
},
}, nil)
Expand Down Expand Up @@ -606,6 +618,9 @@ func TestCreateInstance(t *testing.T) {
},
},
},
Placement: &ec2.Placement{
AvailabilityZone: &az,
},
},
},
}, nil)
Expand Down Expand Up @@ -733,6 +748,9 @@ func TestCreateInstance(t *testing.T) {
},
},
},
Placement: &ec2.Placement{
AvailabilityZone: &az,
},
},
},
}, nil)
Expand Down Expand Up @@ -861,6 +879,9 @@ func TestCreateInstance(t *testing.T) {
},
},
},
Placement: &ec2.Placement{
AvailabilityZone: &az,
},
},
},
}, nil)
Expand Down