From 7e79b8b162fbb773e3d9f15a6d37d2ffb2c1333c Mon Sep 17 00:00:00 2001 From: chiragkyal Date: Thu, 28 Mar 2024 23:14:56 +0530 Subject: [PATCH] :sparkles: Add support to specify PlacementGroupPartition of placement group --- api/v1beta1/awscluster_conversion.go | 1 + api/v1beta1/awsmachine_conversion.go | 2 + api/v1beta1/zz_generated.conversion.go | 2 + api/v1beta2/awsmachine_types.go | 8 + api/v1beta2/types.go | 8 + ...ster.x-k8s.io_awsmanagedcontrolplanes.yaml | 18 ++ ...tructure.cluster.x-k8s.io_awsclusters.yaml | 9 + ...tructure.cluster.x-k8s.io_awsmachines.yaml | 9 + ....cluster.x-k8s.io_awsmachinetemplates.yaml | 10 + pkg/cloud/services/ec2/instances.go | 9 + pkg/cloud/services/ec2/instances_test.go | 243 ++++++++++++++++++ 11 files changed, 319 insertions(+) diff --git a/api/v1beta1/awscluster_conversion.go b/api/v1beta1/awscluster_conversion.go index a3763d22c8..96163be1e4 100644 --- a/api/v1beta1/awscluster_conversion.go +++ b/api/v1beta1/awscluster_conversion.go @@ -56,6 +56,7 @@ func (src *AWSCluster) ConvertTo(dstRaw conversion.Hub) error { if restored.Status.Bastion != nil { dst.Status.Bastion.InstanceMetadataOptions = restored.Status.Bastion.InstanceMetadataOptions dst.Status.Bastion.PlacementGroupName = restored.Status.Bastion.PlacementGroupName + dst.Status.Bastion.PlacementGroupPartition = restored.Status.Bastion.PlacementGroupPartition dst.Status.Bastion.PrivateDNSName = restored.Status.Bastion.PrivateDNSName } dst.Spec.Partition = restored.Spec.Partition diff --git a/api/v1beta1/awsmachine_conversion.go b/api/v1beta1/awsmachine_conversion.go index 55856591a9..3cd84b20a9 100644 --- a/api/v1beta1/awsmachine_conversion.go +++ b/api/v1beta1/awsmachine_conversion.go @@ -38,6 +38,7 @@ func (src *AWSMachine) ConvertTo(dstRaw conversion.Hub) error { dst.Spec.Ignition = restored.Spec.Ignition dst.Spec.InstanceMetadataOptions = restored.Spec.InstanceMetadataOptions dst.Spec.PlacementGroupName = restored.Spec.PlacementGroupName + dst.Spec.PlacementGroupPartition = restored.Spec.PlacementGroupPartition dst.Spec.PrivateDNSName = restored.Spec.PrivateDNSName dst.Spec.SecurityGroupOverrides = restored.Spec.SecurityGroupOverrides @@ -87,6 +88,7 @@ func (r *AWSMachineTemplate) ConvertTo(dstRaw conversion.Hub) error { dst.Spec.Template.Spec.Ignition = restored.Spec.Template.Spec.Ignition dst.Spec.Template.Spec.InstanceMetadataOptions = restored.Spec.Template.Spec.InstanceMetadataOptions dst.Spec.Template.Spec.PlacementGroupName = restored.Spec.Template.Spec.PlacementGroupName + dst.Spec.Template.Spec.PlacementGroupPartition = restored.Spec.Template.Spec.PlacementGroupPartition dst.Spec.Template.Spec.PrivateDNSName = restored.Spec.Template.Spec.PrivateDNSName dst.Spec.Template.Spec.SecurityGroupOverrides = restored.Spec.Template.Spec.SecurityGroupOverrides diff --git a/api/v1beta1/zz_generated.conversion.go b/api/v1beta1/zz_generated.conversion.go index 6fab23cc8a..3ee8fee3ba 100644 --- a/api/v1beta1/zz_generated.conversion.go +++ b/api/v1beta1/zz_generated.conversion.go @@ -1428,6 +1428,7 @@ func autoConvert_v1beta2_AWSMachineSpec_To_v1beta1_AWSMachineSpec(in *v1beta2.AW } out.SpotMarketOptions = (*SpotMarketOptions)(unsafe.Pointer(in.SpotMarketOptions)) // WARNING: in.PlacementGroupName requires manual conversion: does not exist in peer-type + // WARNING: in.PlacementGroupPartition requires manual conversion: does not exist in peer-type out.Tenancy = in.Tenancy // WARNING: in.PrivateDNSName requires manual conversion: does not exist in peer-type return nil @@ -2022,6 +2023,7 @@ func autoConvert_v1beta2_Instance_To_v1beta1_Instance(in *v1beta2.Instance, out out.AvailabilityZone = in.AvailabilityZone out.SpotMarketOptions = (*SpotMarketOptions)(unsafe.Pointer(in.SpotMarketOptions)) // WARNING: in.PlacementGroupName requires manual conversion: does not exist in peer-type + // WARNING: in.PlacementGroupPartition requires manual conversion: does not exist in peer-type out.Tenancy = in.Tenancy out.VolumeIDs = *(*[]string)(unsafe.Pointer(&in.VolumeIDs)) // WARNING: in.InstanceMetadataOptions requires manual conversion: does not exist in peer-type diff --git a/api/v1beta2/awsmachine_types.go b/api/v1beta2/awsmachine_types.go index 10d8ce0dcb..b232cf7969 100644 --- a/api/v1beta2/awsmachine_types.go +++ b/api/v1beta2/awsmachine_types.go @@ -172,6 +172,14 @@ type AWSMachineSpec struct { // +optional PlacementGroupName string `json:"placementGroupName,omitempty"` + // PlacementGroupPartition is the partition number within the placement group in which to launch the instance. + // This value is only valid if the placement group, referred in `PlacementGroupName`, was created with + // strategy set to partition. + // +kubebuilder:validation:Minimum:=1 + // +kubebuilder:validation:Maximum:=7 + // +optional + PlacementGroupPartition int64 `json:"placementGroupPartition,omitempty"` + // Tenancy indicates if instance should run on shared or single-tenant hardware. // +optional // +kubebuilder:validation:Enum:=default;dedicated;host diff --git a/api/v1beta2/types.go b/api/v1beta2/types.go index 545c4f320c..1d6dd0a595 100644 --- a/api/v1beta2/types.go +++ b/api/v1beta2/types.go @@ -221,6 +221,14 @@ type Instance struct { // +optional PlacementGroupName string `json:"placementGroupName,omitempty"` + // PlacementGroupPartition is the partition number within the placement group in which to launch the instance. + // This value is only valid if the placement group, referred in `PlacementGroupName`, was created with + // strategy set to partition. + // +kubebuilder:validation:Minimum:=1 + // +kubebuilder:validation:Maximum:=7 + // +optional + PlacementGroupPartition int64 `json:"placementGroupPartition,omitempty"` + // Tenancy indicates if instance should run on shared or single-tenant hardware. // +optional Tenancy string `json:"tenancy,omitempty"` diff --git a/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml b/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml index 3227df4d81..b412f754a9 100644 --- a/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml +++ b/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml @@ -1109,6 +1109,15 @@ spec: description: PlacementGroupName specifies the name of the placement group in which to launch the instance. type: string + placementGroupPartition: + description: PlacementGroupPartition is the partition number within + the placement group in which to launch the instance. This value + is only valid if the placement group, referred in `PlacementGroupName`, + was created with strategy set to partition. + format: int64 + maximum: 7 + minimum: 1 + type: integer privateDnsName: description: PrivateDNSName is the options for the instance hostname. properties: @@ -2961,6 +2970,15 @@ spec: description: PlacementGroupName specifies the name of the placement group in which to launch the instance. type: string + placementGroupPartition: + description: PlacementGroupPartition is the partition number within + the placement group in which to launch the instance. This value + is only valid if the placement group, referred in `PlacementGroupName`, + was created with strategy set to partition. + format: int64 + maximum: 7 + minimum: 1 + type: integer privateDnsName: description: PrivateDNSName is the options for the instance hostname. properties: diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml index a74b9c7e82..d348bda899 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml @@ -1884,6 +1884,15 @@ spec: description: PlacementGroupName specifies the name of the placement group in which to launch the instance. type: string + placementGroupPartition: + description: PlacementGroupPartition is the partition number within + the placement group in which to launch the instance. This value + is only valid if the placement group, referred in `PlacementGroupName`, + was created with strategy set to partition. + format: int64 + maximum: 7 + minimum: 1 + type: integer privateDnsName: description: PrivateDNSName is the options for the instance hostname. properties: diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachines.yaml index e356896c1b..c956ae76bf 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachines.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachines.yaml @@ -802,6 +802,15 @@ spec: description: PlacementGroupName specifies the name of the placement group in which to launch the instance. type: string + placementGroupPartition: + description: PlacementGroupPartition is the partition number within + the placement group in which to launch the instance. This value + is only valid if the placement group, referred in `PlacementGroupName`, + was created with strategy set to partition. + format: int64 + maximum: 7 + minimum: 1 + type: integer privateDnsName: description: PrivateDNSName is the options for the instance hostname. properties: diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinetemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinetemplates.yaml index 00b85b4969..61b17506d2 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinetemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinetemplates.yaml @@ -758,6 +758,16 @@ spec: description: PlacementGroupName specifies the name of the placement group in which to launch the instance. type: string + placementGroupPartition: + description: PlacementGroupPartition is the partition number + within the placement group in which to launch the instance. + This value is only valid if the placement group, referred + in `PlacementGroupName`, was created with strategy set to + partition. + format: int64 + maximum: 7 + minimum: 1 + type: integer privateDnsName: description: PrivateDNSName is the options for the instance hostname. diff --git a/pkg/cloud/services/ec2/instances.go b/pkg/cloud/services/ec2/instances.go index 36e6661f29..d5f7c537be 100644 --- a/pkg/cloud/services/ec2/instances.go +++ b/pkg/cloud/services/ec2/instances.go @@ -236,6 +236,8 @@ func (s *Service) CreateInstance(scope *scope.MachineScope, userData []byte, use input.PlacementGroupName = scope.AWSMachine.Spec.PlacementGroupName + input.PlacementGroupPartition = scope.AWSMachine.Spec.PlacementGroupPartition + input.PrivateDNSName = scope.AWSMachine.Spec.PrivateDNSName s.scope.Debug("Running instance", "machine-role", scope.Role()) @@ -610,11 +612,18 @@ func (s *Service) runInstance(role string, i *infrav1.Instance) (*infrav1.Instan } } + if i.PlacementGroupName == "" && i.PlacementGroupPartition != 0 { + return nil, errors.Errorf("placementGroupPartition is set but placementGroupName is empty") + } + if i.PlacementGroupName != "" { if input.Placement == nil { input.Placement = &ec2.Placement{} } input.Placement.GroupName = &i.PlacementGroupName + if i.PlacementGroupPartition != 0 { + input.Placement.PartitionNumber = &i.PlacementGroupPartition + } } out, err := s.EC2Client.RunInstancesWithContext(context.TODO(), input) diff --git a/pkg/cloud/services/ec2/instances_test.go b/pkg/cloud/services/ec2/instances_test.go index f68e4a5f5e..6a6d0c031e 100644 --- a/pkg/cloud/services/ec2/instances_test.go +++ b/pkg/cloud/services/ec2/instances_test.go @@ -3204,6 +3204,249 @@ func TestCreateInstance(t *testing.T) { } }, }, + { + name: "with custom placement group and partition number", + machine: &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"set": "node"}, + Namespace: "default", + Name: "machine-aws-test1", + }, + Spec: clusterv1.MachineSpec{ + Bootstrap: clusterv1.Bootstrap{ + DataSecretName: ptr.To[string]("bootstrap-data"), + }, + }, + }, + machineConfig: &infrav1.AWSMachineSpec{ + AMI: infrav1.AMIReference{ + ID: aws.String("abc"), + }, + InstanceType: "m5.large", + PlacementGroupName: "placement-group1", + PlacementGroupPartition: 2, + UncompressedUserData: &isUncompressedFalse, + }, + awsCluster: &infrav1.AWSCluster{ + Spec: infrav1.AWSClusterSpec{ + NetworkSpec: infrav1.NetworkSpec{ + Subnets: infrav1.Subnets{ + infrav1.SubnetSpec{ + ID: "subnet-1", + IsPublic: false, + }, + infrav1.SubnetSpec{ + IsPublic: false, + }, + }, + }, + }, + Status: infrav1.AWSClusterStatus{ + Network: infrav1.NetworkStatus{ + SecurityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{ + infrav1.SecurityGroupControlPlane: { + ID: "1", + }, + infrav1.SecurityGroupNode: { + ID: "2", + }, + infrav1.SecurityGroupLB: { + ID: "3", + }, + }, + APIServerELB: infrav1.LoadBalancer{ + DNSName: "test-apiserver.us-east-1.aws", + }, + }, + }, + }, + expect: func(m *mocks.MockEC2APIMockRecorder) { + m. // TODO: Restore these parameters, but with the tags as well + RunInstancesWithContext(context.TODO(), gomock.Eq(&ec2.RunInstancesInput{ + ImageId: aws.String("abc"), + InstanceType: aws.String("m5.large"), + KeyName: aws.String("default"), + MaxCount: aws.Int64(1), + MinCount: aws.Int64(1), + Placement: &ec2.Placement{ + GroupName: aws.String("placement-group1"), + PartitionNumber: aws.Int64(2), + }, + SecurityGroupIds: []*string{aws.String("2"), aws.String("3")}, + SubnetId: aws.String("subnet-1"), + TagSpecifications: []*ec2.TagSpecification{ + { + ResourceType: aws.String("instance"), + Tags: []*ec2.Tag{ + { + Key: aws.String("MachineName"), + Value: aws.String("default/machine-aws-test1"), + }, + { + Key: aws.String("Name"), + Value: aws.String("aws-test1"), + }, + { + Key: aws.String("kubernetes.io/cluster/test1"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test1"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("node"), + }, + }, + }, + }, + UserData: aws.String(base64.StdEncoding.EncodeToString(userDataCompressed)), + })). + Return(&ec2.Reservation{ + Instances: []*ec2.Instance{ + { + State: &ec2.InstanceState{ + Name: aws.String(ec2.InstanceStateNamePending), + }, + IamInstanceProfile: &ec2.IamInstanceProfile{ + Arn: aws.String("arn:aws:iam::123456789012:instance-profile/foo"), + }, + InstanceId: aws.String("two"), + InstanceType: aws.String("m5.large"), + SubnetId: aws.String("subnet-1"), + ImageId: aws.String("ami-1"), + RootDeviceName: aws.String("device-1"), + BlockDeviceMappings: []*ec2.InstanceBlockDeviceMapping{ + { + DeviceName: aws.String("device-1"), + Ebs: &ec2.EbsInstanceBlockDevice{ + VolumeId: aws.String("volume-1"), + }, + }, + }, + Placement: &ec2.Placement{ + AvailabilityZone: &az, + GroupName: aws.String("placement-group1"), + PartitionNumber: aws.Int64(2), + }, + }, + }, + }, nil) + m. + DescribeInstanceTypesWithContext(context.TODO(), gomock.Eq(&ec2.DescribeInstanceTypesInput{ + InstanceTypes: []*string{ + aws.String("m5.large"), + }, + })). + Return(&ec2.DescribeInstanceTypesOutput{ + InstanceTypes: []*ec2.InstanceTypeInfo{ + { + ProcessorInfo: &ec2.ProcessorInfo{ + SupportedArchitectures: []*string{ + aws.String("x86_64"), + }, + }, + }, + }, + }, nil) + m. + DescribeNetworkInterfacesWithContext(context.TODO(), gomock.Any()). + Return(&ec2.DescribeNetworkInterfacesOutput{ + NetworkInterfaces: []*ec2.NetworkInterface{}, + NextToken: nil, + }, nil) + }, + check: func(instance *infrav1.Instance, err error) { + if err != nil { + t.Fatalf("did not expect error: %v", err) + } + }, + }, + { + name: "expect error when placementGroupPartition is set, but placementGroupName is empty", + machine: &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"set": "node"}, + Namespace: "default", + Name: "machine-aws-test1", + }, + Spec: clusterv1.MachineSpec{ + Bootstrap: clusterv1.Bootstrap{ + DataSecretName: ptr.To[string]("bootstrap-data"), + }, + }, + }, + machineConfig: &infrav1.AWSMachineSpec{ + AMI: infrav1.AMIReference{ + ID: aws.String("abc"), + }, + InstanceType: "m5.large", + PlacementGroupPartition: 2, + UncompressedUserData: &isUncompressedFalse, + }, + awsCluster: &infrav1.AWSCluster{ + Spec: infrav1.AWSClusterSpec{ + NetworkSpec: infrav1.NetworkSpec{ + Subnets: infrav1.Subnets{ + infrav1.SubnetSpec{ + ID: "subnet-1", + IsPublic: false, + }, + infrav1.SubnetSpec{ + IsPublic: false, + }, + }, + }, + }, + Status: infrav1.AWSClusterStatus{ + Network: infrav1.NetworkStatus{ + SecurityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{ + infrav1.SecurityGroupControlPlane: { + ID: "1", + }, + infrav1.SecurityGroupNode: { + ID: "2", + }, + infrav1.SecurityGroupLB: { + ID: "3", + }, + }, + APIServerELB: infrav1.LoadBalancer{ + DNSName: "test-apiserver.us-east-1.aws", + }, + }, + }, + }, + expect: func(m *mocks.MockEC2APIMockRecorder) { + m. + DescribeInstanceTypesWithContext(context.TODO(), gomock.Eq(&ec2.DescribeInstanceTypesInput{ + InstanceTypes: []*string{ + aws.String("m5.large"), + }, + })). + Return(&ec2.DescribeInstanceTypesOutput{ + InstanceTypes: []*ec2.InstanceTypeInfo{ + { + ProcessorInfo: &ec2.ProcessorInfo{ + SupportedArchitectures: []*string{ + aws.String("x86_64"), + }, + }, + }, + }, + }, nil) + }, + check: func(instance *infrav1.Instance, err error) { + expectedErrMsg := "placementGroupPartition is set but placementGroupName is empty" + if err == nil { + t.Fatalf("Expected error, but got nil") + } + if !strings.Contains(err.Error(), expectedErrMsg) { + t.Fatalf("Expected error: %s\nInstead got: `%s", expectedErrMsg, err.Error()) + } + }, + }, { name: "expect the default SSH key when none is provided", machine: &clusterv1.Machine{