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

feat(autoscaling): health check configuration #3390

Merged
merged 12 commits into from
Jul 25, 2019
Merged

feat(autoscaling): health check configuration #3390

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5944005
feat(autoscaling): health check configuration
Jul 23, 2019
ef18ded
chore(auto-scaling): remove consecutive blan kline
nmussy Jul 23, 2019
837f97f
fix(autoscaling): replace health check object with class
Jul 23, 2019
1978b98
Update auto-scaling-group.ts
Jul 23, 2019
a68f32c
fix(autoscaling): remove unused export
Jul 23, 2019
50f9e06
fix(autoscaling): convert gracePeriod in constructor
Jul 23, 2019
430812c
fix(autoscaling): add EC2 healthCheck test
Jul 23, 2019
e254ac6
fix(autoscaling): explicit healthCheck JSDoc default
Jul 23, 2019
10a41d0
fix(autoscaling): specify Ec2HealthCheckOptions JSDOC default grace
Jul 23, 2019
621c076
Merge branch 'master' into feat-autoscaling-health-check-type
Jul 23, 2019
0b8a6a7
Merge branch 'master' into feat-autoscaling-health-check-type
Jul 24, 2019
5cf98c4
Merge branch 'master' into feat-autoscaling-health-check-type
Jul 24, 2019
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
44 changes: 43 additions & 1 deletion packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,13 @@ export interface CommonAutoScalingGroupProps {
* @default none
*/
readonly spotPrice?: string;

/**
* Configuration for health checks
*
* @default - HealthCheckConfiguration with defaults.
Copy link
Contributor

Choose a reason for hiding this comment

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

If healthCheck is not specified, then there are no health checks, right? Fix the default description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the health check is mandatory. The CloudFormation documentation says EC2 is the default, and the API reference says the value is required.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, so we should say that EC2 with no grace period is the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

*/
readonly healthCheck?: HealthCheckConfiguration;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will lends itself nicely to a "union-like" API:

export interface Ec2HealthCheckOptions {
  readonly grace?: Duration;
}

export interface ElbHealthCheckOptions {
  readonly grace: Duration; // <-- required here
}

export class HealthCheck {
  public static ec2(options: Ec2HealthCheckOptions = {}): HealthCheck {
    return new HealthCheck('EC2', options.grace && options.grace.toSeconds());
  }

  public static elb(options: ElbHealthCheckOptions): HealthCheck {
    return new HealthCheck('ELB', options.grace.toSeconds());
  }

  private constructor(public readonly type: string, public readonly gradePeriod?: number) { }
}

And then the usage is:

healthCheck: HealthCheck.ec2();
healthCheck: HealthCheck.ec2({ grace: Duration.minutes(5) });
healthCheck: HealthCheck.elb({ grace: Duration.minutes(5) }); // <-- required grace enforced by compiler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a much cleaner API, thanks.

How would you feel about using a Partial<HealthCheckOptions> instead of repeating the Options Interface?

export interface HealthCheckOptions {
  readonly grace: Duration;
}

export class HealthCheck {
  public static ec2(options: Partial<HealthCheckOptions> = {}): HealthCheck {
    return new HealthCheck('EC2', options.grace && options.grace.toSeconds());
  }

  public static elb(options: HealthCheckOptions): HealthCheck {
    return new HealthCheck('ELB', options.grace.toSeconds());
  }

  private constructor(public readonly type: string, public readonly gradePeriod?: number) { }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, it seems like JSII doesn't support the Partial keyword:

09:46:31 - File change detected. Starting incremental compilation...
@aws-cdk/aws-autoscaling: lib/auto-scaling-group.ts:712:21 - error TS9999: JSII: Only string index maps are supported
@aws-cdk/aws-autoscaling: 712   public static ec2(options: Partial<HealthCheckOptions> = {}): HealthCheck {
@aws-cdk/aws-autoscaling:                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks

}

/**
Expand Down Expand Up @@ -424,6 +431,10 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
throw new Error(`Should have minCapacity (${minCapacity}) <= desiredCapacity (${desiredCapacity}) <= maxCapacity (${maxCapacity})`);
}

if (props.healthCheck && props.healthCheck.type === HealthCheckType.ELB && !props.healthCheck.gracePeriod) {
throw new Error('The healthCheck.gracePeriod property must be set with healthCheck.type === HealthCheckType.ELB');
}

const { subnetIds, hasPublic } = props.vpc.selectSubnets(props.vpcSubnets);
const asgProps: CfnAutoScalingGroupProps = {
cooldown: props.cooldown !== undefined ? props.cooldown.toSeconds().toString() : undefined,
Expand All @@ -444,7 +455,9 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
],
}
],
vpcZoneIdentifier: subnetIds
vpcZoneIdentifier: subnetIds,
healthCheckType: props.healthCheck && props.healthCheck.type,
healthCheckGracePeriod: props.healthCheck && props.healthCheck.gracePeriod ? props.healthCheck.gracePeriod.toSeconds() : undefined,
};

if (!hasPublic && props.associatePublicIpAddress) {
Expand Down Expand Up @@ -673,6 +686,35 @@ export enum ScalingProcess {
ADD_TO_LOAD_BALANCER = 'AddToLoadBalancer'
}

/**
* Health check settings
*/
export interface HealthCheckConfiguration {
/**
* Specifies the service to use for the health checks
*
* If you configure an Auto Scaling group to use ELB health checks, it considers the instance unhealthy
* if it fails either the EC2 status checks or the load balancer health checks.
*
* @default HealthCheckType.EC2
*/
readonly type?: HealthCheckType;

/**
* Specified the time Auto Scaling waits before checking the health status of an EC2 instance that has come into service
*
* This property required with ELB health check type.
*
* @default -
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find a current default value in the CloudFormation documentation. The API Reference mentions a default of 0. Should I use Duration.seconds(0) in the JSDoc?

*/
readonly gracePeriod?: Duration;
}

export enum HealthCheckType {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to export this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

EC2 = 'EC2',
ELB = 'ELB',
}

/**
* Render the rolling update configuration into the appropriate object
*/
Expand Down
48 changes: 48 additions & 0 deletions packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,54 @@ export = {
test.done();
},

'can configure health check'(test: Test) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add a test for ec2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// GIVEN
const stack = new cdk.Stack(undefined, 'MyStack', { env: { region: 'us-east-1', account: '1234' } });
const vpc = mockVpc(stack);

// WHEN
new autoscaling.AutoScalingGroup(stack, 'MyFleet', {
instanceType: ec2.InstanceType.of(ec2.InstanceClass.M4, ec2.InstanceSize.MICRO),
machineImage: new ec2.AmazonLinuxImage(),
vpc,
healthCheck: {
type: autoscaling.HealthCheckType.ELB,
gracePeriod: cdk.Duration.minutes(15)
}
});

// THEN
expect(stack).to(haveResourceLike("AWS::AutoScaling::AutoScalingGroup", {
HealthCheckType: 'ELB',
HealthCheckGracePeriod: 900
}));

test.done();
},

'throws when healthCheck.type is ELB without a gracePeriod'(test: Test) {
// GIVEN
const stack = new cdk.Stack(undefined, 'MyStack', { env: { region: 'us-east-1', account: '1234' } });
const vpc = mockVpc(stack);

// WHEN
const toThrow = () => {
new autoscaling.AutoScalingGroup(stack, 'MyFleet', {
instanceType: ec2.InstanceType.of(ec2.InstanceClass.M4, ec2.InstanceSize.MICRO),
machineImage: new ec2.AmazonLinuxImage(),
vpc,
healthCheck: {
type: autoscaling.HealthCheckType.ELB,
}
});
};

// THEN
test.throws(() => toThrow(), /gracePeriod property must be set/);

test.done();
},

'can add Security Group to Fleet'(test: Test) {
// GIVEN
const stack = new cdk.Stack(undefined, 'MyStack', { env: { region: 'us-east-1', account: '1234' } });
Expand Down