feat(manifest): add HTTP Health Check hidden fields #1592

huanjani · 2020-10-30T00:09:24Z

This enables the customization of four Load Balanced Web Service health check variables: HealthyThreshold, UnhealthyThreshold, Interval, and Timeout are now hidden fields in the manifest.

Resolves #1195.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

efekarakus · 2020-10-30T01:06:06Z

Heya @huanjani I haven't read the PR but is it possible to update the fields in the manifest to be separated by underscores instead of camelCase (for consistency) and be under the healthcheck field?

# Option 1. Provide only a path and assume Copilot's default
healthcheck: '/'

# Option 2. Advanced configuration:
healthcheck:
  path: '/'
  healthy_threshold: 5
  unhealthy_threshold: 5
  interval: 30s # Uses golang time.Duration type
  timeout: 5s  # Uses golang time.Duration type

We do this sort of advanced configuration for both the image.build field as well as count for autoscaling if you'd like to take a look at that as a starting point!

huanjani · 2020-11-02T05:30:24Z

manifest option 1:

http:
  # Requests to this path will be forwarded to your service. 
  # To match all requests you can use the "/" path. 
  path: 'skittles'
  # You can specify a custom health check path. The default is "/".
  # For additional configuration: https://aws.github.io/copilot-cli/docs/manifest/lb-web-service/#http-healthcheck
  healthcheck: ./inline/path
  # You can enable sticky sessions.
  # stickiness: true

stack for option 1:

  TargetGroup:
    Type: AWS::ElasticLoadBalancingV2::TargetGroup
    Properties:
      #  Check if your service is healthy within 20 = 10*2 seconds, compared to 2.5 mins = 30*5 seconds.
      HealthCheckPath: ./inline/path # Default is '/'.
      HealthyThresholdCount: 2 # Default is 2.
      UnhealthyThresholdCount: 2 # Default is 2.
      HealthCheckIntervalSeconds: 10 # Default is 10.
      HealthCheckTimeoutSeconds: 5 # Default is 5.

manifest option 2:

  healthcheck:
    path: ./outline/path
    healthy_threshold: 3
    unhealthy_threshold: 4
    interval: 55s
    timeout: 7s

stack for option 2

      HealthCheckPath: ./outline/path # Default is '/'.
      HealthyThresholdCount: 3 # Default is 2.
      UnhealthyThresholdCount: 4 # Default is 2.
      HealthCheckIntervalSeconds: 55 # Default is 10.
      HealthCheckTimeoutSeconds: 7 # Default is 5.

bvtujo

This looks great! This will be so helpful to fix some of those annoying flapping container deployments due to failed healthchecks on startup!

bvtujo · 2020-11-02T19:28:48Z

internal/pkg/manifest/lb_web_svc_test.go

+		"error if unmarshalable": {
+			inContent: []byte(`  healthcheck:
+    bath: to ruin
+    unwealthy_threshold: berry`),


internal/pkg/manifest/lb_web_svc.go

efekarakus · 2020-11-02T19:39:56Z

internal/pkg/manifest/lb_web_svc.go

+)
+
+var (
+	errUnmarshalHealthCheckArgs = errors.New("can't unmarshal healthcheck field into string or compose-style map")


nit: let's remove words like cannot from the error message unless it's in the cli pkg (the root pkg)

When adding context to returned errors, keep the context succinct by avoiding phrases like "failed to", which state the obvious and pile up as the error percolates up through the stack:

https://github.com/uber-go/guide/blob/master/style.md#error-wrapping

I think it is fine to keep can't in this scenario since it is already the root error instead of wrapping the other error. Agree if we are wrapping error we want to avoid piling up like failed to get health check: can't unmarshal health check. And instead we want get health check: can't unmarshal health check. What do you all think?

More the opposite, cannot get health check: unmarshal health check. Where the root error that wraps others can use "cannot" to make it more human friendly but "library" pkgs shouldn't put cannot in their error messages

But according to https://github.com/uber-go/guide/blob/master/style.md#error-wrapping, isn't cannot get health check: unmarshal health check a bad example?

internal/pkg/manifest/lb_web_svc.go

efekarakus · 2020-11-02T19:48:09Z

internal/pkg/manifest/lb_web_svc.go

 	// TargetContainer is the container load balancer routes traffic to.
-	TargetContainer *string `yaml:"targetContainer"`
+	TargetContainer *string `yaml:"target_container"`


Ah I wish we could change this :'( but we would need to support both targetContainer and target_container to maintain backwards compatibility.

Right now this will break users that have targetContainer as a field. We'd need to figure out a way of assigning two yaml fields to TargetContainer.
Maybe this works?

Suggested change

TargetContainer *string `yaml:"target_container"`

TargetContainer *string `yaml:"target_container" yaml:"targetContainer"` // "targetContainer" for backwards compatibility

If it does great, otherwise can we punt this change to a separate PR?

Okay, so that ☝️ didn't work and some digging/reading makes me think there isn't a built-in way to do this, but this little hack works:
In manifest/lb_web_svc.go, I added another field to the struct for camel case.
In stack/lb_web_svc.go:

if s.manifest.TargetContainer == nil && s.manifest.TargetContainerCamelCase != nil { s.manifest.TargetContainer = s.manifest.TargetContainerCamelCase }

brilliant! that sounds good to me!

samesies thanks for diving deep on this!

efekarakus · 2020-11-02T19:49:29Z

site/content/docs/manifest/lb-web-service.md

+<span class="parent-field">http.</span><a id="http-healthcheck" href="#http-healthcheck" class="field">`healthcheck`</a> <span class="type">String or Map</span>  
+If you specify a string, Copilot interprets it as the path exposed in your container to handle target group health check requests. The default is "/".


😍 thank you for updating the docs!

iamhopaul123 · 2020-11-02T20:05:30Z

site/content/docs/manifest/lb-web-service.md

+You can also specify healthcheck as a map:
+```yaml
+http:
+  healthcheck:
+    path: '/'
+    healthy_threshold: 3
+    unhealthy_threshold: 2
+    interval: 15s
+    timeout: 10s
+```


Awesome! This is gorgeous!

iamhopaul123 · 2020-11-02T21:45:45Z

internal/pkg/manifest/lb_web_svc.go

+)
+
+var (
+	errUnmarshalHealthCheckArgs = errors.New("can't unmarshal healthcheck field into string or compose-style map")


But according to https://github.com/uber-go/guide/blob/master/style.md#error-wrapping, isn't cannot get health check: unmarshal health check a bad example?

RaulTsc · 2020-11-08T18:40:38Z

Great job @huanjani looking forward to the release!

This enables the customization of four Load Balanced Web Service health check variables: HealthyThreshold, UnhealthyThreshold, Interval, and Timeout are now hidden fields in the manifest. Resolves aws#1195. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

huanjani added 4 commits October 28, 2020 13:44

wip

b09f875

chore: get template building properly; start tests

3647310

chore: finish unit tests

f72c8ba

Merge remote-tracking branch 'archer/mainline' into healthcheck

2af5287

huanjani requested a review from a team as a code owner October 30, 2020 00:09

huanjani requested a review from uttarasridhar October 30, 2020 00:09

amazon-ecs-cli-v2-pr-manager requested a review from kohidave October 30, 2020 00:09

amazon-ecs-cli-v2-pr-manager added this to In review in Sprint 🏃‍♀️ Oct 30, 2020

huanjani removed request for kohidave and uttarasridhar October 30, 2020 00:11

huanjani added 2 commits October 29, 2020 17:27

fix: uc HTTP in template yaml

9017edf

fix: reorder fields

945c67c

huanjani added 2 commits November 1, 2020 21:09

fix: refactor to enable ArgsOrString, change ints to duration, etc.

dba23a2

fix: remove unused method

fa2bfeb

huanjani and others added 2 commits November 1, 2020 21:30

fix: remove 's' for seconds from template

3876af8

Merge branch 'mainline' into healthcheck

ebcdb9c

bvtujo approved these changes Nov 2, 2020

View reviewed changes

efekarakus reviewed Nov 2, 2020

View reviewed changes

iamhopaul123 reviewed Nov 2, 2020

View reviewed changes

fix: default path (/) in manifest so not nil

6f0d31c

iamhopaul123 approved these changes Nov 2, 2020

View reviewed changes

iamhopaul123 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Nov 2, 2020

huanjani and others added 3 commits November 2, 2020 16:13

chore: allow camel case and snake case for target container yaml field

ccb5a5a

Merge branch 'mainline' into healthcheck

89969dd

Merge branch 'mainline' into healthcheck

da4df77

huanjani removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Nov 3, 2020

mergify bot merged commit 95344bc into aws:mainline Nov 3, 2020

Sprint 🏃‍♀️ automation moved this from In review to Pending release Nov 3, 2020

This was referenced Nov 4, 2020

ECS health check grace period #1626

Closed

Target group - Time out and Interval parameters configuration #1627

Closed

huanjani deleted the healthcheck branch November 7, 2020 00:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(manifest): add HTTP Health Check hidden fields #1592

feat(manifest): add HTTP Health Check hidden fields #1592

huanjani commented Oct 30, 2020 •

edited by efekarakus

efekarakus commented Oct 30, 2020 •

edited

huanjani commented Nov 2, 2020

bvtujo left a comment

bvtujo Nov 2, 2020

efekarakus Nov 2, 2020

iamhopaul123 Nov 2, 2020

efekarakus Nov 2, 2020

iamhopaul123 Nov 2, 2020 •

edited

efekarakus Nov 2, 2020

huanjani Nov 3, 2020

iamhopaul123 Nov 3, 2020

efekarakus Nov 3, 2020

efekarakus Nov 2, 2020

iamhopaul123 Nov 2, 2020

kohidave Nov 2, 2020

iamhopaul123 Nov 2, 2020 •

edited

RaulTsc commented Nov 8, 2020

	TargetContainer *string `yaml:"target_container"`
	TargetContainer *string `yaml:"target_container" yaml:"targetContainer"` // "targetContainer" for backwards compatibility

		<span class="parent-field">http.</span><a id="http-healthcheck" href="#http-healthcheck" class="field">`healthcheck`</a> <span class="type">String or Map</span>
		If you specify a string, Copilot interprets it as the path exposed in your container to handle target group health check requests. The default is "/".

feat(manifest): add HTTP Health Check hidden fields #1592

feat(manifest): add HTTP Health Check hidden fields #1592

Conversation

huanjani commented Oct 30, 2020 • edited by efekarakus

efekarakus commented Oct 30, 2020 • edited

huanjani commented Nov 2, 2020

bvtujo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamhopaul123 Nov 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamhopaul123 Nov 2, 2020 • edited

Choose a reason for hiding this comment

RaulTsc commented Nov 8, 2020

huanjani commented Oct 30, 2020 •

edited by efekarakus

efekarakus commented Oct 30, 2020 •

edited

iamhopaul123 Nov 2, 2020 •

edited

iamhopaul123 Nov 2, 2020 •

edited