Update healthcheck Description on BackendConfig removal #2181

DamianSawicki · 2023-06-29T11:23:35Z

In #2008 and #2068, we guarantee that the health check Description is updated when a BackendConfig is added to the Service (behind the flag --enable-update-hc-description). When a BackendConfig is removed, typically a health check update is triggered, which includes resetting the Description. However, under some circumstances the health check parameters update won't happen on BackendConfig removal, but it is still desirable to reset the Description to reflect the lack of BackendConfig. This reset is added in the present PR.

The changes are behind the abovementioned flag --enable-update-hc-description. This is a temporary fix and corresponding code will be removed with the subsequent planned changes.

k8s-ci-robot · 2023-06-29T11:23:43Z

Hi @DamianSawicki. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

DamianSawicki · 2023-06-29T11:44:10Z

/assign aojea
/cc swetharepakula

pkg/healthchecks/healthchecks.go

pkg/healthchecks/healthchecks_test.go

swetharepakula · 2023-06-29T19:14:59Z

/ok-to-test

DamianSawicki · 2023-06-30T13:01:23Z

/cc mag-kol
/assign swetharepakula

k8s-ci-robot · 2023-06-30T13:01:26Z

@DamianSawicki: GitHub didn't allow me to request PR reviews from the following users: mag-kol.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc mag-kol
/assign swetharepakula

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.

cezarygerard · 2023-06-30T11:42:36Z

pkg/healthchecks/healthchecks_test.go

@@ -1743,6 +1792,25 @@ func TestSyncServicePort(t *testing.T) {
 				t.Errorf("hcs.SyncServicePort(tc.sp, tc.probe) = %q, _; want = %q", gotSelfLink, tc.wantSelfLink)
 			}
 			verify()
+
+			if tc.expectedEvent > 0 {


it's better to create a new test func then extend this with new cases and special assertion just for your cases

cezarygerard · 2023-06-30T11:48:55Z

pkg/healthchecks/healthchecks.go

+	if flags.F.EnableUpdateCustomHealthCheckDescription {
+		desc := &healthcheck.HealthcheckDesc{}
+		err := json.Unmarshal([]byte(existingHC.Description), desc)
+		if err == nil && desc.Config == healthcheck.BackendConfigHC && bchcc == nil {


bchcc?
pelase name this properly
it has been declared 50+ lines earlier

maybe healthCheckConfig is a good name

BC stands for BackendConfig
HC must stand for Health Check
C is probably Config

Renaming this is a topic for a refactor, not a quick bug fix.

cezarygerard · 2023-06-30T11:59:38Z

pkg/healthchecks/healthchecks.go

+			if err != nil {
+				klog.Errorf("Health check %q update error: %v", existingHC.Name, err)
+			}
+			h.notifyAboutTHC(hc, thcConf.THCEvents)


notifyAboutTHC -> emitTHCEvents

event is well defined concept

how many times notifyAboutTHC func can be called in one execution? I may happen a few times from what I see

Just once, it's always shortly before a return statement.

cezarygerard · 2023-06-30T12:05:10Z

pkg/healthchecks/healthchecks.go

+	// The contents of the following 'if' are temporary, see https://github.com/kubernetes/ingress-gce/pull/2181 for details.
+	if flags.F.EnableUpdateCustomHealthCheckDescription {
+		desc := &healthcheck.HealthcheckDesc{}
+		err := json.Unmarshal([]byte(existingHC.Description), desc)


you have to do something if this error happens
maybe logging error is enough (with warning severity?)

Error means that the Description is not a JSON. We are in the process of switching from plain strings to JSON, depending on health check types and flags enabled, so it's not really an error to see a non-jsonified Description.

for debugging purposes it would be good to have a log that says that this was not a JSON description

cezarygerard · 2023-06-30T12:33:01Z

pkg/healthchecks/healthchecks.go

@@ -262,6 +263,28 @@ func (h *HealthChecks) sync(hc *translator.HealthCheck, bchcc *backendconfigv1.H
 		return existingHC.SelfLink, err
 	}

+	// The contents of the following 'if' are temporary, see https://github.com/kubernetes/ingress-gce/pull/2181 for details.


please add more info here

Discussed this with Antonio already, there are Google-internal resources with details.

I second Cezary's comment. Since we cannot link the internal issue, it would be good to have some description in the code.

It would be a good idea to describe what this trying to do as in we are only updating the HC description and nothing else

swetharepakula

The changes look good to me. I do agree with the existing feedback that we should have a more detailed comment on what this is fixing. Though this will be changed in the future, we will still be supporting this version of the code for a long time and may need the comment to understand past context.

kl52752 · 2023-07-03T12:56:05Z

pkg/healthchecks/healthchecks.go

+		desc := &healthcheck.HealthcheckDesc{}
+		err := json.Unmarshal([]byte(existingHC.Description), desc)
+		if err != nil {
+			klog.V(3).Info("Description for healthcheck %s is not a JSON (probably a plain-text description): %s.", existingHC.Name, existingHC.Description)


Can you do early return here and remove else. Also you can mention in this log that the update is omitted because the description is not a JSON.

Also you can mention in this log that the update is omitted because the description is not a JSON.

Done.

Can you do early return here and remove else.

I would prefer not to to avoid copy-pasting the following lines.

klog.V(2).Infof("Health check %q already exists and needs no update", hc.Name) return existingHC.SelfLink, nil

kl52752 · 2023-07-03T13:01:34Z

pkg/healthchecks/healthchecks.go

+	// even if changes.hasDiff() above is false. No other health check field is modified. The purpose is for the Description to accurately
+	// reflect the existence of a backendconfigv1.HealthCheckConfig for the service. This is temporary, see
+	// https://github.com/kubernetes/ingress-gce/pull/2181 for details.
+	if flags.F.EnableUpdateCustomHealthCheckDescription {


I don't like the idea to postpone the update if it will not happened earlier. I think it would be better to put all logic related to checking against backend config description to a function fg isDescriptionUpdateNeeded and add condition to line 247

if changes.hasDiff() || isDescriptionUpdateNeeded() {}

WDYT?

Discussed outside GitHub, done. @kl52752 PTAL.

kl52752 · 2023-07-03T13:12:31Z

pkg/healthchecks/healthchecks_test.go

+	shallowCopy := func(x *compute.HealthCheck) *compute.HealthCheck { y := *x; return &y }
+
+	// Don't recalculate health check on BackendConfig removal (legacy behaviour, to be changed), but update Description.
+	chc := fixture.hc()


instead of using ficture.hc() and overriding almost everything can you create new function healthCheckFromBackendConfig and reuse it below? (I don't see any difference here)

Good idea, done.

kl52752 · 2023-07-03T13:14:13Z

pkg/healthchecks/healthchecks_test.go

+					t.Fatalf("Got %d healthchecks, want 1\n%s", len(computeHCs), pretty.Sprint(computeHCs))
+				}
+
+				// Filter out SelfLink because it is hard to deal with in the mock and


And description if flag is disabled

This is a temporary solution under the flag --enable-update-hc-description.

kl52752 · 2023-07-05T08:51:25Z

/lgtm

k8s-ci-robot · 2023-07-05T08:51:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DamianSawicki, kl52752

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~~ [kl52752]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 29, 2023

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 Jun 29, 2023

k8s-ci-robot requested review from code-elinka and gauravkghildiyal June 29, 2023 11:23

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 29, 2023

DamianSawicki marked this pull request as ready for review June 29, 2023 11:33

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2023

k8s-ci-robot requested a review from aojea June 29, 2023 11:34

k8s-ci-robot assigned aojea Jun 29, 2023

k8s-ci-robot requested a review from swetharepakula June 29, 2023 11:44

aojea reviewed Jun 29, 2023

View reviewed changes

pkg/healthchecks/healthchecks.go Outdated Show resolved Hide resolved

aojea reviewed Jun 29, 2023

View reviewed changes

pkg/healthchecks/healthchecks.go Show resolved Hide resolved

aojea reviewed Jun 29, 2023

View reviewed changes

pkg/healthchecks/healthchecks_test.go Outdated Show resolved Hide resolved

DamianSawicki force-pushed the fix-backendconfig-removal branch 2 times, most recently from a567521 to 08777ff Compare June 29, 2023 12:33

DamianSawicki requested a review from aojea June 29, 2023 12:36

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 29, 2023

k8s-ci-robot assigned swetharepakula Jun 30, 2023

cezarygerard reviewed Jun 30, 2023

View reviewed changes

swetharepakula reviewed Jun 30, 2023

View reviewed changes

DamianSawicki force-pushed the fix-backendconfig-removal branch from 08777ff to 31157e7 Compare July 3, 2023 10:16

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 3, 2023

kl52752 reviewed Jul 3, 2023

View reviewed changes

Update healthcheck Description on BackendConfig removal

b3e0bdf

This is a temporary solution under the flag --enable-update-hc-description.

DamianSawicki force-pushed the fix-backendconfig-removal branch from 31157e7 to b3e0bdf Compare July 3, 2023 14:50

Unified flow for Description-only healthcheck update

d8e429a

DamianSawicki requested a review from kl52752 July 4, 2023 16:49

k8s-ci-robot assigned kl52752 Jul 5, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 5, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 5, 2023

k8s-ci-robot merged commit 01fcfee into kubernetes:master Jul 5, 2023
5 checks passed

DamianSawicki mentioned this pull request Jul 7, 2023

Recalculate health check params on BC/THC removal #2192

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update healthcheck Description on BackendConfig removal #2181

Update healthcheck Description on BackendConfig removal #2181

DamianSawicki commented Jun 29, 2023

k8s-ci-robot commented Jun 29, 2023

DamianSawicki commented Jun 29, 2023

swetharepakula commented Jun 29, 2023

DamianSawicki commented Jun 30, 2023

k8s-ci-robot commented Jun 30, 2023

cezarygerard Jun 30, 2023

DamianSawicki Jul 3, 2023

cezarygerard Jun 30, 2023

DamianSawicki Jun 30, 2023

DamianSawicki Jul 3, 2023

cezarygerard Jun 30, 2023

cezarygerard Jun 30, 2023

DamianSawicki Jun 30, 2023

cezarygerard Jun 30, 2023

DamianSawicki Jun 30, 2023

swetharepakula Jun 30, 2023

DamianSawicki Jul 3, 2023

cezarygerard Jun 30, 2023

DamianSawicki Jun 30, 2023 •

edited

Loading

swetharepakula Jun 30, 2023

swetharepakula Jun 30, 2023

DamianSawicki Jul 3, 2023

swetharepakula left a comment

kl52752 Jul 3, 2023

DamianSawicki Jul 3, 2023

kl52752 Jul 3, 2023

DamianSawicki Jul 4, 2023 •

edited

Loading

kl52752 Jul 3, 2023

DamianSawicki Jul 3, 2023

kl52752 Jul 3, 2023

DamianSawicki Jul 3, 2023

kl52752 commented Jul 5, 2023

k8s-ci-robot commented Jul 5, 2023

Update healthcheck Description on BackendConfig removal #2181

Update healthcheck Description on BackendConfig removal #2181

Conversation

DamianSawicki commented Jun 29, 2023

k8s-ci-robot commented Jun 29, 2023

DamianSawicki commented Jun 29, 2023

swetharepakula commented Jun 29, 2023

DamianSawicki commented Jun 30, 2023

k8s-ci-robot commented Jun 30, 2023

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

DamianSawicki Jun 30, 2023 • 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

swetharepakula 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

DamianSawicki Jul 4, 2023 • 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

Choose a reason for hiding this comment

kl52752 commented Jul 5, 2023

k8s-ci-robot commented Jul 5, 2023

DamianSawicki Jun 30, 2023 •

edited

Loading

DamianSawicki Jul 4, 2023 •

edited

Loading