Skip to content

Commit

Permalink
refactor(aws): minor refactors and changes to existing features (#5362)
Browse files Browse the repository at this point in the history 
spinnaker/spinnaker#5989
  • Loading branch information
@pdk27
pdk27 authored May 25, 2021
1 parent 07186a0 commit 7fe9ddc
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 43 deletions.
5 changes: 3 additions & 2 deletions ...ovy/com/netflix/spinnaker/clouddriver/aws/deploy/handlers/BasicAmazonDeployHandler.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,7 @@ class BasicAmazonDeployHandler implements DeployHandler<BasicAmazonDeployDescrip
return newAsgDescription.blockDevices
}

if (newAsgDescription.getAllowedInstanceTypes() != AsgConfigHelper.getAllowedInstanceTypesForAsg(sourceAsg, sourceAsgRegionScopedProvider)) {
if (newAsgDescription.getInstanceType() != AsgConfigHelper.getTopLevelInstanceTypeForAsg(sourceAsg, sourceAsgRegionScopedProvider)) {
// If instance type(s) being requested is NOT the same as those in source ASG,
// get default mapping for the new type ONLY IF that same logic was applied for source ASG.
// For the case of multiple instance types in request, top-level instance type is used to derive defaults.
Expand All @@ -607,7 +607,8 @@ class BasicAmazonDeployHandler implements DeployHandler<BasicAmazonDeployDescrip
.collect { [deviceName: it.deviceName, virtualName: it.virtualName, size: it.size] }
.sort { it.deviceName }

if (blockDevicesForSourceAsg == defaultBlockDevicesForSourceInsType) {
boolean isDefaultMappingUsedInSourceAsg = blockDevicesForSourceAsg == defaultBlockDevicesForSourceInsType
if (isDefaultMappingUsedInSourceAsg) {
return blockDeviceConfig.getBlockDevicesForInstanceType(newAsgDescription.getInstanceType())
}
}
Expand Down
6 changes: 3 additions & 3 deletions ...n/groovy/com/netflix/spinnaker/clouddriver/aws/provider/view/AmazonClusterProvider.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -599,13 +599,13 @@ class AmazonClusterProvider implements ClusterProvider<AmazonCluster>, ServerGro
}

// launchTemplate#instanceType is ignored when it is overridden. So, remove it to prevent accidental misuse / ambiguity.
Map ec2LtMinusType = getLaunchTemplateForVersion(launchData, ltVersionStr)
ec2LtMinusType["launchTemplateData"].remove("instanceType")
Map ec2LtDataMinusType = ec2Lt?."launchTemplateData".findAll {it.key != "instanceType"}
ec2Lt?.replace("launchTemplateData", ec2LtDataMinusType)

serverGroup.mixedInstancesPolicy = new AmazonServerGroup.MixedInstancesPolicySettings().tap {
allowedInstanceTypes = types.sort()
instancesDiversification = serverGroup.asg.mixedInstancesPolicy["instancesDistribution"]
launchTemplates = [ ec2LtMinusType ]
launchTemplates = [ ec2Lt ]
launchTemplateOverridesForInstanceType = overrides
}
}
Expand Down
10 changes: 5 additions & 5 deletions ...r-aws/src/main/java/com/netflix/spinnaker/clouddriver/aws/deploy/asg/AsgConfigHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,8 @@ private static String createDefaultSuffix() {
}

/**
* Transform AWS BlockDeviceMapping (found in ASG LaunchConfiguration) to {@link
* AmazonBlockDevice}. Used while extracting launch settings from AWS AutoScalingGroup.
* Transform AWS BlockDeviceMapping to {@link AmazonBlockDevice}. Used while extracting launch
* settings from AWS AutoScalingGroup or AMI.
*
* @param blockDeviceMappings AWS BlockDeviceMappings
* @return list of AmazonBlockDevice
Expand Down Expand Up @@ -329,7 +329,7 @@ protected static List<AmazonBlockDevice> transformBlockDeviceMapping(
}
return amzBd;
})
.collect(Collectors.toList());
.collect(Collectors.toUnmodifiableList());
}

/**
Expand Down Expand Up @@ -377,7 +377,7 @@ protected static List<AmazonBlockDevice> convertBlockDevices(
* @param launchTemplateBlockDeviceMappings AWS LaunchTemplate BlockDeviceMappings
* @return list of AmazonBlockDevice
*/
protected static List<AmazonBlockDevice> transformLaunchTemplateBlockDeviceMapping(
public static List<AmazonBlockDevice> transformLaunchTemplateBlockDeviceMapping(
List<LaunchTemplateBlockDeviceMapping> launchTemplateBlockDeviceMappings) {
return launchTemplateBlockDeviceMappings.stream()
.map(
Expand All @@ -402,6 +402,6 @@ protected static List<AmazonBlockDevice> transformLaunchTemplateBlockDeviceMappi
}
return amzBd;
})
.collect(Collectors.toList());
.collect(Collectors.toUnmodifiableList());
}
}
4 changes: 2 additions & 2 deletions ...louddriver/aws/deploy/converters/BasicAmazonDeployAtomicOperationConverterUnitSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ class BasicAmazonDeployAtomicOperationConverterUnitSpec extends Specification {
def input = [application: "kato", credentials: 'test',
setLaunchTemplate: true, requireIMDSv2: true, associateIPv6Address: true, unlimitedCpuCredits: true,
placement: [groupName: "test-placement"], licenseSpecifications: [[arn: "test-arn"]],
onDemandAllocationStrategy: "prioritized", onDemandBaseCapacity: 2, onDemandPercentageAboveBaseCapacity: 50, spotAllocationStrategy: "capacity-optimized",
onDemandAllocationStrategy: "prioritized", onDemandBaseCapacity: 2, onDemandPercentageAboveBaseCapacity: 50, spotAllocationStrategy: "lowest-price",
spotInstancePools: 3, spotPrice: "0.5", launchTemplateOverridesForInstanceType: [[instanceType: "some.type.large", weightedCapacity: 2]]]

when:
Expand All @@ -148,7 +148,7 @@ class BasicAmazonDeployAtomicOperationConverterUnitSpec extends Specification {
description.onDemandAllocationStrategy == "prioritized"
description.onDemandBaseCapacity == 2
description.onDemandPercentageAboveBaseCapacity == 50
description.spotAllocationStrategy == "capacity-optimized"
description.spotAllocationStrategy == "lowest-price"
description.spotInstancePools == 3
description.spotPrice == "0.5"
description.launchTemplateOverridesForInstanceType == [new BasicAmazonDeployDescription.LaunchTemplateOverridesForInstanceType(instanceType: "some.type.large", weightedCapacity: 2)]
Expand Down
46 changes: 15 additions & 31 deletions ...netflix/spinnaker/clouddriver/aws/deploy/handlers/BasicAmazonDeployHandlerUnitSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -664,7 +664,6 @@ class BasicAmazonDeployHandlerUnitSpec extends Specification {
handler.copySourceAttributes(
sourceRegionScopedProvider, "sourceAsg", useSource, description
)

then:
thrown(IllegalStateException)

Expand Down Expand Up @@ -884,9 +883,9 @@ class BasicAmazonDeployHandlerUnitSpec extends Specification {
.withLaunchTemplateId(launchTemplateVersion.launchTemplateId)
.withLaunchTemplateName(launchTemplateVersion.launchTemplateName)
.withVersion(launchTemplateVersion.versionNumber.toString()))
.withOverrides(sourceLtOverrides.collect{
new LaunchTemplateOverrides().withInstanceType(it.instanceType).withWeightedCapacity(it.wgtCap)
}))
.withOverrides(
new LaunchTemplateOverrides().withInstanceType("c3.large").withWeightedCapacity("2"),
new LaunchTemplateOverrides().withInstanceType("c3.xlarge").withWeightedCapacity("4")))

def sourceAsg = new AutoScalingGroup().withMixedInstancesPolicy(mixedInstancesPolicy)

Expand All @@ -907,10 +906,10 @@ class BasicAmazonDeployHandlerUnitSpec extends Specification {
and:
def description = new BasicAmazonDeployDescription(
instanceType: descInstanceType,
launchTemplateOverridesForInstanceType: descLtOverrides.collect{
new BasicAmazonDeployDescription.LaunchTemplateOverridesForInstanceType(
instanceType: it.instanceType,
weightedCapacity: it.wgtCap)},
launchTemplateOverridesForInstanceType: [
new BasicAmazonDeployDescription.LaunchTemplateOverridesForInstanceType(instanceType: "c4.large", weightedCapacity: "2"),
new BasicAmazonDeployDescription.LaunchTemplateOverridesForInstanceType(instanceType: "c4.xlarge", weightedCapacity: "4")
],
blockDevices: descBlockDevices
)

Expand All @@ -921,29 +920,14 @@ class BasicAmazonDeployHandlerUnitSpec extends Specification {
blockDeviceMappings == expectedTargetBlockDevices

where:
sourceInstanceType| descInstanceType| sourceLtOverrides | descLtOverrides | sourceBlockDevices | descBlockDevices || expectedTargetBlockDevices
"c3.xlarge" | "c4.xlarge" | [[instanceType: "c3.large", wgtCap: "2"],
[instanceType: "c3.xlarge", wgtCap: "4"]] | [[instanceType: "c4.large", wgtCap: "2"],
[instanceType: "c4.xlarge", wgtCap: "4"]] | bD("c3.xlarge") | bD("c3.xlarge") || bD("c3.xlarge") // use the explicitly provided block devices even if instance type has changed
"c3.xlarge" | "c4.xlarge" | [[instanceType: "c3.large", wgtCap: "2"],
[instanceType: "c3.xlarge", wgtCap: "4"]] | [[instanceType: "c4.large", wgtCap: "2"],
[instanceType: "c4.xlarge", wgtCap: "4"]] | bD("c3.xlarge") | [] || [] // use the explicitly provided block devices even if an empty list
"c3.xlarge" | "c4.xlarge" | [[instanceType: "c3.large", wgtCap: "2"],
[instanceType: "c3.xlarge", wgtCap: "4"]] | [[instanceType: "c4.large", wgtCap: "2"],
[instanceType: "c4.xlarge", wgtCap: "4"]] | bD("c3.xlarge") | null || bD("c4.xlarge") // source ASG used default block devices, so use default block devices for top-level instance type in description i.e. descInstanceType
"c3.xlarge" | "c4.xlarge" | [[instanceType: "c3.large", wgtCap: "2"],
[instanceType: "c3.xlarge", wgtCap: "4"]] | [[instanceType: "c4.large", wgtCap: "2"],
[instanceType: "c4.xlarge", wgtCap: "4"]] | [new AmazonBlockDevice(
deviceName: "/dev/xxx")] | null || [new AmazonBlockDevice(deviceName: "/dev/xxx")] // custom block devices should be preserved
"c3.xlarge" | "c4.100xlarge" | [[instanceType: "c3.large", wgtCap: "2"],
[instanceType: "c3.xlarge", wgtCap: "4"]] | [[instanceType: "c4.large", wgtCap: "2"],
[instanceType: "c4.100xlarge", wgtCap: "4"]] | bD("c3.xlarge") | null || [deployDefaults.unknownInstanceTypeBlockDevice] // source ASG used default bD, so use default bD but no mapping for c4.200xlarge, use the default for unknown instance types
"c3.xlarge" | "c3.xlarge" | [[instanceType: "c3.large", wgtCap: "2"],
[instanceType: "c3.xlarge", wgtCap: "4"]] | [[instanceType: "c3.large", wgtCap: "2"],
[instanceType: "c3.xlarge", wgtCap: "4"]] | bD("c3.xlarge") | null || bD("c3.xlarge") // allowed instance types match, use source ASG's block devices
"c3.xlarge" | "r4.xlarge" | [[instanceType: "c3.large", wgtCap: "2"],
[instanceType: "c3.xlarge", wgtCap: "4"]] | [[instanceType: "c3.large", wgtCap: "2"],
[instanceType: "c3.xlarge", wgtCap: "4"]] | bD("c3.xlarge") | null || bD("c3.xlarge") // allowed instance types match, use source ASG's block devices
sourceInstanceType| descInstanceType| sourceBlockDevices | descBlockDevices || expectedTargetBlockDevices
"c3.xlarge" | "c4.xlarge" | bD("c3.xlarge") | bD("c3.xlarge") || bD("c3.xlarge") // use the explicitly provided block devices even if instance type has changed
"c3.xlarge" | "c4.xlarge" | bD("c3.xlarge") | [] || [] // use the explicitly provided block devices even if an empty list
"c3.xlarge" | "c4.xlarge" | bD("c3.xlarge") | null || bD("c4.xlarge") // source ASG used default block devices, so use default block devices for top-level instance type in description i.e. descInstanceType
"c3.xlarge" | "c4.xlarge" |[new AmazonBlockDevice(
deviceName: "/dev/xxx")] | null || [new AmazonBlockDevice(deviceName: "/dev/xxx")] // custom block devices should be preserved
"c3.xlarge" | "c4.100xlarge" | bD("c3.xlarge") | null || [deployDefaults.unknownInstanceTypeBlockDevice] // source ASG used default bD, so use default bD but no mapping for c4.200xlarge, use the default for unknown instance types
"c3.xlarge" | "c3.xlarge" | bD("c3.xlarge") | null || bD("c3.xlarge") // top-level instance types match, use source ASG's block devices
}

@Unroll
Expand Down

0 comments on commit 7fe9ddc

Please sign in to comment.