allow ELB Healthcheck configuration via Service annotations #56024

dimpavloff · 2017-11-19T22:36:44Z

What this PR does / why we need it:
The default settings which are set on the ELB HC work well but there are cases when it would be better to tweak its parameters -- for example, faster detection of unhealthy backends. This PR makes it possible to override any of the healthcheck's parameters via annotations on the Service, with the exception of the Target setting which continues to be inferred from the Service's spec.

Release note:

It is now possible to override the healthcheck parameters for AWS ELBs via annotations on the corresponding service. The new annotations are `healthy-threshold`, `unhealthy-threshold`, `timeout`, `interval` (all prefixed with `service.beta.kubernetes.io/aws-load-balancer-healthcheck-`)

dimpavloff · 2017-11-19T22:42:14Z

Hi!
I couldn't find a corresponding GH issue for my PR, hopefully my change makes sense to you. While writing the PR, the following crossed my mind:

Azure and GKE also use hard-coded values for their healthchecks. It would be nicer than my current PR to have cloud-agnostic annotations to control the HC. However, I don't know whether the parameters can match exactly and, if they don't, whether it will be acceptable to have each cloud provider implementation interpret these annotations (slightly) differently. So in the interest of smaller change sets I've stuck with with AWS-specific annotations. It should be straightforward to adapt my implementation to handle an addition of cloud-agnostic annotations.
I am on the fence whether to add an override for the HC's Target. Having an override can be handy when the Service has multiple ports -- relying on the order in which the ports are declared in the Service is hackier, IMO. OTOH, it is useful for the HC to be derived from the Service's spec because this ensures there's no drift between the HC Target and the ports exposed by the Service, which won't be the case if there's an annotation. Let me know what you think about this, I would be happy to make the addition for Target if you think it would be overall better.

This is my first PR, let me know if I could be reusing some functionality or if my style is inconsistent. Thanks! :)

justinsb · 2017-11-21T11:54:29Z

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

+		}
+		return &i64, nil
+	}
+	if i, err := getOrDefault(ServiceAnnotationLoadBalancerHCHealthyThreshold, defaultHCHealthyThreshold); err != nil {


This might be easier as:

var err error healthcheck.HealthyThreshold, err = getOrDefault(ServiceAnnotationLoadBalancerHCHealthyThreshold, defaultHCHealthyThreshold) if err != nil { return nil, err } ...

Oh, of course! It looks much nicer now, thanks!

justinsb · 2017-11-21T11:56:12Z

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

+	if err != nil {
+		return fmt.Errorf("cannot update health check for load balancer %q: %q", name, err)
+	}
+	if reflect.DeepEqual(expected, actual) {


This is dangerous, because it breaks if another field is added to the struct in future. Safer to use the explicit field-by-field comparison.

Good catch! I've fixed it, please take another look. I've opted for using aws.Int64Value instead of the less verbose orZero because it's docs say it is deprecated.

justinsb · 2017-11-21T11:59:33Z

/ok-to-test

This is really great, thank you @dimpavloff . I agree with just allowing the 4 "timing" values to be set - at least initially.

I had a suggestion on a simpler way (maybe) to write the get-or-default calls, but that was only an idea. But that's only a suggestion, your way works great.

If you can fix the use of reflect.DeepEqual though I think this looks good. If you can do it today we can get it into 1.9 I think.

The constants which have been used so far have been set as default in case the annotations have not been set.

dimpavloff

Thanks for reviewing, @justinsb :) I've updated the PR and fixed the bazel test dependency, please take another look.

If you can do it today we can get it into 1.9 I think.

Amazing!

dimpavloff · 2017-11-21T14:08:01Z

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

+		}
+		return &i64, nil
+	}
+	if i, err := getOrDefault(ServiceAnnotationLoadBalancerHCHealthyThreshold, defaultHCHealthyThreshold); err != nil {


Oh, of course! It looks much nicer now, thanks!

dimpavloff · 2017-11-21T14:18:38Z

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

+	if err != nil {
+		return fmt.Errorf("cannot update health check for load balancer %q: %q", name, err)
+	}
+	if reflect.DeepEqual(expected, actual) {


Good catch! I've fixed it, please take another look. I've opted for using aws.Int64Value instead of the less verbose orZero because it's docs say it is deprecated.

justinsb · 2017-11-22T15:44:09Z

/lgtm

/approve

dimpavloff · 2017-11-22T15:55:36Z

/kind kind/feature

justinsb · 2017-11-22T16:00:52Z

/approve no-issue

k8s-github-robot · 2017-11-22T16:02:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dimpavloff, justinsb

Associated issue requirement bypassed by: justinsb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cloudprovider/providers/aws/OWNERS~~ [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-22T16:06:03Z

[MILESTONENOTIFIER] Milestone Pull Request Needs Attention

@dimpavloff @jsafrane @justinsb @zmerlynn @kubernetes/sig-aws-misc

Action required: During code slush, pull requests in the milestone should be in progress.
If this pull request is not being actively worked on, please remove it from the milestone.
If it is being worked on, please add the status/in-progress label so it can be tracked with other in-flight pull requests.

Note: This pull request is marked as priority/critical-urgent, and must be updated every 3 days during code slush.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required

Pull Request Labels

sig/aws: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/feature: New functionality.

Help

k8s-github-robot · 2017-11-22T16:48:43Z

Automatic merge from submit-queue (batch tested with PRs 56211, 56024). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 19, 2017

k8s-github-robot assigned jsafrane and zmerlynn Nov 19, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 19, 2017

dimpavloff force-pushed the aws-elb-set-hc-params branch from 091d8a4 to 9237af6 Compare November 19, 2017 22:55

justinsb reviewed Nov 21, 2017

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 21, 2017

dimpavloff force-pushed the aws-elb-set-hc-params branch 2 times, most recently from 1bbfae0 to b1526be Compare November 21, 2017 14:26

allow ELB HC configuration via Service annotations

8cf7c5e

The constants which have been used so far have been set as default in case the annotations have not been set.

dimpavloff force-pushed the aws-elb-set-hc-params branch from b1526be to 8cf7c5e Compare November 21, 2017 14:34

dimpavloff commented Nov 21, 2017

View reviewed changes

k8s-ci-robot assigned justinsb Nov 22, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2017

justinsb added this to the v1.9 milestone Nov 22, 2017

justinsb added status/approved-for-milestone priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 22, 2017

k8s-github-robot added the milestone/incomplete-labels label Nov 22, 2017

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 22, 2017

k8s-github-robot added milestone/needs-attention and removed milestone/incomplete-labels labels Nov 22, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2017

justinsb added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 22, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2017

k8s-github-robot added milestone/incomplete-labels and removed milestone/needs-attention labels Nov 22, 2017

justinsb removed the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Nov 22, 2017

k8s-github-robot added the milestone/needs-attention label Nov 22, 2017

k8s-github-robot removed the milestone/incomplete-labels label Nov 22, 2017

k8s-github-robot merged commit 6b1b6d5 into kubernetes:master Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow ELB Healthcheck configuration via Service annotations #56024

allow ELB Healthcheck configuration via Service annotations #56024

dimpavloff commented Nov 19, 2017

dimpavloff commented Nov 19, 2017 •

edited

justinsb Nov 21, 2017

dimpavloff Nov 21, 2017

justinsb Nov 21, 2017

dimpavloff Nov 21, 2017

justinsb commented Nov 21, 2017

dimpavloff left a comment

dimpavloff Nov 21, 2017

dimpavloff Nov 21, 2017

justinsb commented Nov 22, 2017

dimpavloff commented Nov 22, 2017

justinsb commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

allow ELB Healthcheck configuration via Service annotations #56024

allow ELB Healthcheck configuration via Service annotations #56024

Conversation

dimpavloff commented Nov 19, 2017

dimpavloff commented Nov 19, 2017 • edited

justinsb Nov 21, 2017

Choose a reason for hiding this comment

dimpavloff Nov 21, 2017

Choose a reason for hiding this comment

justinsb Nov 21, 2017

Choose a reason for hiding this comment

dimpavloff Nov 21, 2017

Choose a reason for hiding this comment

justinsb commented Nov 21, 2017

dimpavloff left a comment

Choose a reason for hiding this comment

dimpavloff Nov 21, 2017

Choose a reason for hiding this comment

dimpavloff Nov 21, 2017

Choose a reason for hiding this comment

justinsb commented Nov 22, 2017

dimpavloff commented Nov 22, 2017

justinsb commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

dimpavloff commented Nov 19, 2017 •

edited