Set healthcheck from BackendConfig #1029

bowei · 2020-02-17T08:38:45Z

    Take healthcheck configuration overrides from the backendconfig

    This enables taking the healthcheck configuration from a backendconfig
    associated with the given Service.

    As part of this PR, we also clean up the override handling to be easier
    to understand where the settings come from.

    For a given ServicePort `sp` and its associated healthcheck `hc`:

    1. `sp` has some default settings depending on its type (e.g. IG, NEG, ILB).
    2. SyncServicePort() will override those settings if the Probe exists.
         Path, Host, TimeoutSec, CheckIntervalSec.
    3. We then sync with what exists in GCE:
        3.1 If the healthcheck is does not exist, it will be CREATED.
            3.1.1 If it has a backendconfig, then the following settings are used:
                    CheckIntervalSec, TimeoutSec, HealthyThreshold,
                    UnhealthyThreshold, Type, RequestPath
                  We have `Port` in the struct, but it does not seem to be a valid
                  field to use so it is currently ignored.
        3.2 Otherwise the healthcheck will need to be UPDATED. The criteria
            for update can be found in `calculateDiff()`:
            3.2.1 Services w/out backendconfig will only update when `Protocol`,
                  `PortSpecification` changes.
            3.2.2 Services w/ backendconfig will also consider any fields
                  covered by the backendconfig (see above)
            3.2.3 We merge in the settings from the existing healthcheck. As
                  users have (in the past) done edits to their HCs directly, we
                  do not want to break any previous behavior. Thus we merge in
                  all of the settings on HTTPHealthCheck as well as any fields on
                  HealthCheck itself, with the exception of: `Port`, `PortSpecification`
            3.2.2 We now consider the backendconfig to be a final override.
                  See list above for settings from the backendconfig.

    Several situations do not seem to be handled currently:

    * There is no way to change HC based on probe after it is created.
      This is because there is no way to tell if the difference is due
      to a user changing the setting directly in GCE or the HC should
      be updated.
    * Changing types to/from NEG, ILB will leak the old healthcheck.
      User healthchecksettings will not propagate to the new healthcheck.
    * backendconfig seems to be missing `Host`

TestSyncServicePort does the following:

1. Setup any preexisting objects (tc.setup) that should exist in the
   mocked GCE.
2. Run one sync given a ServicePort.
3. Attempt to re-run the sync with the same ServicePort, checking that
   no update will be needed.

k8s-ci-robot · 2020-02-17T08:39:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bowei]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rramkumar1

Just a couple questions, will do a more in-depth review once this is rebased on the dependent PRs

rramkumar1 · 2020-02-17T17:24:32Z

pkg/healthchecks/healthchecks.go

-	if needToUpdate(existingHC, hc) {
-		err = h.update(existingHC, hc)
-		return existingHC.SelfLink, err
+	// First, merge in the configuration from the existing healthcheck to cover


Is this to ensure a smooth migration path to BackendConfig?

This is behavior that we have to preserve (unfortunately)

rramkumar1 · 2020-02-17T17:25:24Z

pkg/healthchecks/healthchecks.go

 	}
-	return false
+	// TODO: why don't we check Port?


What does this mean?

We omit quite a few fields when checking for update, which is intentional but not documented (nor reasoning)

bowei · 2020-02-17T20:46:19Z

/assign @rramkumar1 @freehan @spencerhance

k8s-ci-robot · 2020-02-17T20:46:21Z

@bowei: GitHub didn't allow me to assign the following users: spencerhance.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @rramkumar1 @freehan @spencerhance

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

bowei · 2020-02-17T21:20:29Z

This should be ready for review.

rramkumar1 · 2020-02-17T22:08:19Z

pkg/healthchecks/healthchecks.go

 			return "", err
 		}
-
-		return h.getHealthCheckLink(hc.Name, hc.Version(), scope)
+		// TODO(bowei) -- we don't need to fetch the self-link here as it is


I think you can use NewHealthChecksResourceID in k8s_cloud_provider to generate this link.

Will do this in a follow-up PR

rramkumar1 · 2020-02-17T22:10:40Z

pkg/healthchecks/healthchecks.go

+	// GKE.
+	premergeHC := hc
+	hc = mergeUserSettings(existingHC, hc)
+	klog.V(3).Infof("mergeHealthcheck(%v,%v) = %v", existingHC, premergeHC, hc)


I did this kind of on purpose to make the log line shorter...

actuall, let me fix this.

rramkumar1 · 2020-02-17T22:11:28Z

pkg/healthchecks/healthchecks.go

-func (h *HealthChecks) create(hc *HealthCheck) error {
+func (h *HealthChecks) create(hc *HealthCheck, bchcc *backendconfigv1.HealthCheckConfig) error {
+	if bchcc != nil {
+		klog.V(2).Infof("Health check %q has backendconfig override (%+v)", hc.Name, bchcc)


You log this same thing in line 165 as well. Maybe remove one of them?

It's not the same. This is on the create path, that instance is on the update path.

Might make sense to differentiate the log lines

You can distinguish from the source line in klog.

Good point, seems reasonable to leave it as is then

rramkumar1 · 2020-02-17T22:17:07Z

pkg/healthchecks/healthchecks.go

+
+	if existing.HealthCheck.LogConfig != nil {
+		l := *existing.HealthCheck.LogConfig
+		hc.HealthCheck.LogConfig = &l


Is this an Alpha-only field? If that is the case, where do we configure this HC to be Alpha?

Doing this handles all resource versions due to how version conversion via JSON drops fields when it gets converted.
This is proactive. I don't expect to see this field right now.

rramkumar1 · 2020-02-17T22:19:00Z

pkg/healthchecks/healthchecks.go

+	// Cannot specify both portSpecification and port field.
+	if hc.PortSpecification != "" {
+		hc.Port = 0
+	}


Why was this added here?

This was done on all paths that ended up at merge() previously.
I consolidated it here and remove the other instances.
Really, this should not exist, but people have been writing imprecise code, so I am keeping it for now.

rramkumar1 · 2020-02-17T22:19:31Z

pkg/healthchecks/healthchecks.go

+	if c.RequestPath != nil {
+		hc.RequestPath = *c.RequestPath
+	}
+	if c.Port != nil {


I'm a little confused by this? Shouldn't setting port be allowed for IG backends?

Doesn't make sense. Port is set from NodePort.
NEG should take port from the neg itself.

Would be good to hear about a use case though...

spencerhance

There are a lot of TODOs and this is a pretty large diff, but overall lgtm. Have you run any e2e tests against this?

spencerhance · 2020-02-18T20:51:06Z

pkg/healthchecks/healthchecks.go

+}
+
+// fieldDiffs encapsulate which fields are different between health checks.
+type fieldDiffs struct {


This might be easier to read if in a new file

I will send a PR to split the file.
The diff gets very hard to read when I did the split...

bowei · 2020-02-18T20:59:04Z

Some of the TODOs can't be done right now (esp the ones around bugs).
I left them so we can file issues around them and track them long-term.

This allows us to check for HTTPStatusCode == 404 directly without guarding it with a nil check.

This enables taking the healthcheck configuration from a backendconfig associated with the given Service. As part of this PR, we also clean up the override handling to be easier to understand where the settings come from. For a given ServicePort `sp` and its associated healthcheck `hc`: 1. `sp` has some default settings depending on its type (e.g. IG, NEG, ILB). 2. SyncServicePort() will override those settings if the Probe exists. Path, Host, TimeoutSec, CheckIntervalSec. 3. We then sync with what exists in GCE: 3.1 If the healthcheck is does not exist, it will be CREATED. 3.1.1 If it has a backendconfig, then the following settings are used: CheckIntervalSec, TimeoutSec, HealthyThreshold, UnhealthyThreshold, Type, RequestPath We have `Port` in the struct, but it does not seem to be a valid field to use so it is currently ignored. 3.2 Otherwise the healthcheck will need to be UPDATED. The criteria for update can be found in `calculateDiff()`: 3.2.1 Services w/out backendconfig will only update when `Protocol`, `PortSpecification` changes. 3.2.2 Services w/ backendconfig will also consider any fields covered by the backendconfig (see above) 3.2.3 We merge in the settings from the existing healthcheck. As users have (in the past) done edits to their HCs directly, we do not want to break any previous behavior. Thus we merge in all of the settings on HTTPHealthCheck as well as any fields on HealthCheck itself, with the exception of: `Port`, `PortSpecification` 3.2.2 We now consider the backendconfig to be a final override. See list above for settings from the backendconfig. Several situations do not seem to be handled currently: * There is no way to change HC based on probe after it is created. This is because there is no way to tell if the difference is due to a user changing the setting directly in GCE or the HC should be updated. * Changing types to/from NEG, ILB will leak the old healthcheck. User healthchecksettings will not propagate to the new healthcheck. * backendconfig seems to be missing `Host` merge

TestSyncServicePort does the following: 1. Setup any preexisting objects (tc.setup) that should exist in the mocked GCE. 2. Run one sync given a ServicePort. 3. Attempt to re-run the sync with the same ServicePort, checking that no update will be needed.

bowei · 2020-02-23T03:52:59Z

I just tested this by manually running the e2e test and am getting a PR together for the e2e test.
Going to merge this for now...

mark-church · 2020-03-06T22:41:52Z

@bowei based off of how users are desiring to configure health checks for istio-ingressgateway, we'll need to support the host field as well.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 17, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 17, 2020

k8s-ci-robot requested review from rramkumar1 and thockin February 17, 2020 08:39

bowei changed the title ~~Hc from bc~~ Implement getting healthcheck from BackendConfig Feb 17, 2020

rramkumar1 reviewed Feb 17, 2020

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 17, 2020

k8s-ci-robot assigned freehan and rramkumar1 Feb 17, 2020

bowei force-pushed the hc-from-bc branch from 32c9079 to ebdb52d Compare February 17, 2020 20:47

bowei changed the title ~~Implement getting healthcheck from BackendConfig~~ Set healthcheck from BackendConfig Feb 17, 2020

bowei force-pushed the hc-from-bc branch from ebdb52d to 365736a Compare February 17, 2020 21:16

bowei force-pushed the hc-from-bc branch 2 times, most recently from 2337b2f to 897edfa Compare February 17, 2020 21:36

rramkumar1 reviewed Feb 17, 2020

View reviewed changes

spencerhance approved these changes Feb 18, 2020

View reviewed changes

bowei mentioned this pull request Feb 18, 2020

Healthcheck configuration behavior #1009

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 20, 2020

bowei added 3 commits February 22, 2020 14:19

Add support for nil to HTTPStatusCode

fb5edf1

This allows us to check for HTTPStatusCode == 404 directly without guarding it with a nil check.

Add testing for SyncServicePort

13844f7

TestSyncServicePort does the following: 1. Setup any preexisting objects (tc.setup) that should exist in the mocked GCE. 2. Run one sync given a ServicePort. 3. Attempt to re-run the sync with the same ServicePort, checking that no update will be needed.

bowei force-pushed the hc-from-bc branch from 897edfa to 13844f7 Compare February 22, 2020 22:20

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2020

Update code generation (was missing healthcheck fields)

168a7bc

bowei merged commit 9c53eaf into kubernetes:master Feb 23, 2020

bowei deleted the hc-from-bc branch April 21, 2020 18:30

bowei mentioned this pull request Apr 25, 2020

Ingress Healthcheck Configuration #42

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set healthcheck from BackendConfig #1029

Set healthcheck from BackendConfig #1029

bowei commented Feb 17, 2020 •

edited

Loading

k8s-ci-robot commented Feb 17, 2020

rramkumar1 left a comment

rramkumar1 Feb 17, 2020

bowei Feb 17, 2020

rramkumar1 Feb 17, 2020

bowei Feb 17, 2020

bowei commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

bowei commented Feb 17, 2020

rramkumar1 Feb 17, 2020

bowei Feb 22, 2020

rramkumar1 Feb 17, 2020

bowei Feb 18, 2020

bowei Feb 22, 2020

rramkumar1 Feb 17, 2020

bowei Feb 18, 2020

spencerhance Feb 18, 2020

bowei Feb 18, 2020

spencerhance Feb 18, 2020

rramkumar1 Feb 17, 2020

bowei Feb 18, 2020

rramkumar1 Feb 17, 2020

bowei Feb 18, 2020 •

edited

Loading

rramkumar1 Feb 17, 2020

bowei Feb 18, 2020

bowei Feb 18, 2020

spencerhance left a comment •

edited

Loading

spencerhance Feb 18, 2020 •

edited

Loading

bowei Feb 18, 2020

spencerhance Feb 18, 2020

bowei commented Feb 18, 2020

bowei commented Feb 23, 2020

mark-church commented Mar 6, 2020

Set healthcheck from BackendConfig #1029

Set healthcheck from BackendConfig #1029

Conversation

bowei commented Feb 17, 2020 • edited Loading

k8s-ci-robot commented Feb 17, 2020

rramkumar1 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

bowei commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

bowei commented Feb 17, 2020

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

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

bowei Feb 18, 2020 • edited Loading

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

spencerhance left a comment • edited Loading

Choose a reason for hiding this comment

spencerhance Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowei commented Feb 18, 2020

bowei commented Feb 23, 2020

mark-church commented Mar 6, 2020

bowei commented Feb 17, 2020 •

edited

Loading

bowei Feb 18, 2020 •

edited

Loading

spencerhance left a comment •

edited

Loading

spencerhance Feb 18, 2020 •

edited

Loading