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.

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

fix(deadline): allow zero-sized WorkerInstanceFleet #451

Merged
merged 2 commits into from
May 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 14 additions & 3 deletions packages/aws-rfdk/lib/deadline/lib/worker-fleet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
BlockDevice,
CfnAutoScalingGroup,
HealthCheck,
Signals,
} from '@aws-cdk/aws-autoscaling';
import {IMetric, Metric} from '@aws-cdk/aws-cloudwatch';
import {
Expand Down Expand Up @@ -445,6 +446,14 @@ export class WorkerInstanceFleet extends WorkerInstanceFleetBase {

this.validateProps(props);

const minCapacity = props.minCapacity ?? 1;
const signals = minCapacity > 0 ? Signals.waitForMinCapacity({
timeout: WorkerInstanceFleet.RESOURCE_SIGNAL_TIMEOUT,
}) : undefined;
if (signals === undefined) {
Annotations.of(this).addWarning('Deploying with 0 minimum capacity. If there is an error in the EC2 UserData for this fleet, then your stack deployment will not fail. Watch for errors in your CloudWatch logs.');
}

// Launching the fleet with deadline workers.
this.fleet = new AutoScalingGroup(this, 'Default', {
vpc: props.vpc,
Expand All @@ -455,10 +464,10 @@ export class WorkerInstanceFleet extends WorkerInstanceFleetBase {
subnetType: SubnetType.PRIVATE,
},
securityGroup: props.securityGroup,
minCapacity: props.minCapacity,
minCapacity,
maxCapacity: props.maxCapacity,
desiredCapacity: props.desiredCapacity,
resourceSignalTimeout: WorkerInstanceFleet.RESOURCE_SIGNAL_TIMEOUT,
signals,
healthCheck: HealthCheck.elb({
grace: WorkerInstanceFleet.DEFAULT_HEALTH_CHECK_INTERVAL,
}),
Expand Down Expand Up @@ -514,7 +523,9 @@ export class WorkerInstanceFleet extends WorkerInstanceFleetBase {
);

// Updating the user data with successful cfn-signal commands.
this.fleet.userData.addSignalOnExitCommand(this.fleet);
if (signals) {
this.fleet.userData.addSignalOnExitCommand(this.fleet);
}

// Tag deployed resources with RFDK meta-data
tagConstruct(this);
Expand Down
83 changes: 70 additions & 13 deletions packages/aws-rfdk/lib/deadline/test/worker-fleet.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import {
expect as expectCDK,
haveResource,
haveResourceLike,
objectLike,
ResourcePart,
} from '@aws-cdk/assert';
import {
BlockDeviceVolume,
Expand Down Expand Up @@ -130,10 +132,10 @@ test('default worker fleet is created correctly', () => {
RetentionInDays: 3,
LogGroupName: '/renderfarm/workerFleet',
}));
expect(fleet.node.metadata[0].type).toMatch(ArtifactMetadataEntryType.WARN);
expect(fleet.node.metadata[0].data).toMatch('being created without being provided any block devices so the Source AMI\'s devices will be used. Workers can have access to sensitive data so it is recommended to either explicitly encrypt the devices on the worker fleet or to ensure the source AMI\'s Drives are encrypted.');
expect(fleet.node.metadata[1].type).toMatch(ArtifactMetadataEntryType.WARN);
expect(fleet.node.metadata[1].data).toContain('being created without a health monitor attached to it. This means that the fleet will not automatically scale-in to 0 if the workers are unhealthy');
expect(fleet.node.metadataEntry[0].type).toMatch(ArtifactMetadataEntryType.WARN);
expect(fleet.node.metadataEntry[0].data).toMatch('being created without being provided any block devices so the Source AMI\'s devices will be used. Workers can have access to sensitive data so it is recommended to either explicitly encrypt the devices on the worker fleet or to ensure the source AMI\'s Drives are encrypted.');
expect(fleet.node.metadataEntry[1].type).toMatch(ArtifactMetadataEntryType.WARN);
expect(fleet.node.metadataEntry[1].data).toContain('being created without a health monitor attached to it. This means that the fleet will not automatically scale-in to 0 if the workers are unhealthy');
});

test('security group is added to fleet after its creation', () => {
Expand Down Expand Up @@ -1580,8 +1582,8 @@ describe('Block Device Tests', () => {
renderQueue,
healthMonitor,
});
expect(fleet.node.metadata[0].type).toMatch(ArtifactMetadataEntryType.WARN);
expect(fleet.node.metadata[0].data).toMatch('being created without being provided any block devices so the Source AMI\'s devices will be used. Workers can have access to sensitive data so it is recommended to either explicitly encrypt the devices on the worker fleet or to ensure the source AMI\'s Drives are encrypted.');
expect(fleet.node.metadataEntry[0].type).toMatch(ArtifactMetadataEntryType.WARN);
expect(fleet.node.metadataEntry[0].data).toMatch('being created without being provided any block devices so the Source AMI\'s devices will be used. Workers can have access to sensitive data so it is recommended to either explicitly encrypt the devices on the worker fleet or to ensure the source AMI\'s Drives are encrypted.');
});

test('No Warnings if Encrypted BlockDevices Provided', () => {
Expand Down Expand Up @@ -1613,7 +1615,7 @@ describe('Block Device Tests', () => {
],
}));

expect(fleet.node.metadata).toHaveLength(0);
expect(fleet.node.metadataEntry).toHaveLength(0);
});

test('Warnings if non-Encrypted BlockDevices Provided', () => {
Expand Down Expand Up @@ -1646,8 +1648,8 @@ describe('Block Device Tests', () => {
],
}));

expect(fleet.node.metadata[0].type).toMatch(ArtifactMetadataEntryType.WARN);
expect(fleet.node.metadata[0].data).toMatch(`The BlockDevice \"${DEVICE_NAME}\" on the worker-fleet workerFleet is not encrypted. Workers can have access to sensitive data so it is recommended to encrypt the devices on the worker fleet.`);
expect(fleet.node.metadataEntry[0].type).toMatch(ArtifactMetadataEntryType.WARN);
expect(fleet.node.metadataEntry[0].data).toMatch(`The BlockDevice \"${DEVICE_NAME}\" on the worker-fleet workerFleet is not encrypted. Workers can have access to sensitive data so it is recommended to encrypt the devices on the worker fleet.`);
});

test('Warnings for BlockDevices without encryption specified', () => {
Expand Down Expand Up @@ -1679,8 +1681,8 @@ describe('Block Device Tests', () => {
],
}));

expect(fleet.node.metadata[0].type).toMatch(ArtifactMetadataEntryType.WARN);
expect(fleet.node.metadata[0].data).toMatch(`The BlockDevice \"${DEVICE_NAME}\" on the worker-fleet workerFleet is not encrypted. Workers can have access to sensitive data so it is recommended to encrypt the devices on the worker fleet.`);
expect(fleet.node.metadataEntry[0].type).toMatch(ArtifactMetadataEntryType.WARN);
expect(fleet.node.metadataEntry[0].data).toMatch(`The BlockDevice \"${DEVICE_NAME}\" on the worker-fleet workerFleet is not encrypted. Workers can have access to sensitive data so it is recommended to encrypt the devices on the worker fleet.`);
});

test('No warnings for Ephemeral blockDeviceVolumes', () => {
Expand Down Expand Up @@ -1710,7 +1712,7 @@ describe('Block Device Tests', () => {
],
}));

expect(fleet.node.metadata).toHaveLength(0);
expect(fleet.node.metadataEntry).toHaveLength(0);
});

test('No warnings for Suppressed blockDeviceVolumes', () => {
Expand Down Expand Up @@ -1739,7 +1741,7 @@ describe('Block Device Tests', () => {
],
}));

expect(fleet.node.metadata).toHaveLength(0);
expect(fleet.node.metadataEntry).toHaveLength(0);
});
});

Expand Down Expand Up @@ -1884,3 +1886,58 @@ describe('tagging', () => {
},
});
});

test('worker fleet signals when non-zero minCapacity', () => {
// WHEN
const fleet = new WorkerInstanceFleet(wfstack, 'workerFleet', {
vpc,
workerMachineImage: new GenericWindowsImage({
'us-east-1': 'ami-any',
}),
renderQueue,
minCapacity: 1,
});

// WHEN
const userData = fleet.fleet.userData.render();

// THEN
expect(userData).toContain('cfn-signal');
expectCDK(wfstack).to(haveResourceLike('AWS::AutoScaling::AutoScalingGroup', {
CreationPolicy: {
ResourceSignal: {
Count: 1,
},
},
}, ResourcePart.CompleteDefinition));
// [0] = warning about block devices. [1] = warning about no health monitor
expect(fleet.node.metadataEntry).toHaveLength(2);
});

test('worker fleet does not signal when zero minCapacity', () => {
// WHEN
const fleet = new WorkerInstanceFleet(wfstack, 'workerFleet', {
vpc,
workerMachineImage: new GenericWindowsImage({
'us-east-1': 'ami-any',
}),
renderQueue,
minCapacity: 0,
});

// WHEN
const userData = fleet.fleet.userData.render();

// THEN
ddneilson marked this conversation as resolved.
Show resolved Hide resolved
// There should be no cfn-signal call in the UserData.
expect(userData).not.toContain('cfn-signal');
// Make sure we don't have a CreationPolicy
expectCDK(wfstack).notTo(haveResourceLike('AWS::AutoScaling::AutoScalingGroup', {
CreationPolicy: objectLike({}),
}, ResourcePart.CompleteDefinition));
// There should be a warning in the construct's metadata about deploying with no capacity.
expect(fleet.node.metadataEntry).toHaveLength(3);
// [0] = warning about block devices. [2] = warning about no health monitor
expect(fleet.node.metadataEntry[1].type).toMatch(ArtifactMetadataEntryType.WARN);
expect(fleet.node.metadataEntry[1].data).toMatch(/Deploying with 0 minimum capacity./);
ddneilson marked this conversation as resolved.
Show resolved Hide resolved
});