Emit event on Description-only healthcheck update #2072

DamianSawicki · 2023-04-07T14:02:37Z

Since #2008, the health check description is modified if the service health check is configured with a BackendConfig CRD. The purpose of the present PR is to emit an event on the service on this occasion.

The changes are guarded with the flag flags.F.EnableUpdateCustomHealthCheckDescription from #2018.

k8s-ci-robot · 2023-04-07T14:02:47Z

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-04-07T14:07:20Z

/assign aojea

pkg/healthchecks/healthchecks_test.go

vendor/k8s.io/cloud-provider-gcp/providers/gce/gce.go

aojea · 2023-04-12T08:42:03Z

pkg/translator/healthchecks.go

@@ -338,3 +341,32 @@ func ApplyProbeSettingsToHC(p *v1.Probe, hc *HealthCheck) {

 	hc.Description = DescriptionForHealthChecksFromReadinessProbe
 }
+
+// GetObjectKind implements runtime.Object


is this change related?

Yes. EventRecorder.Event requires the first argument to be runtime.Object, and this interface contains two methods: GetObjectKind() schema.ObjectKind and DeepCopyObject() Object.

the object in this case is the Service or just pass the Namespace, doing this to the healtcheck object seems very brittle

I think you can get it from defaultBackendSvc types.NamespacedName

This again requires implementing the above methods either for string (in the case of namespace) or for types.NamespacedName (in the case of service), and when trying to do that, I'm getting the following error cannot define new methods on non-local type "k8s.io/apimachinery/pkg/types".NamespacedName. I guess I can write my own alias type for one of these and then implement the methods for the alias. @aojea, do you think it is a good idea?

no sorry, my point is that we should not modify this type, and use one of the Kubernetes types associated to the healtcheck, it can be the Service or the Namespace, ideally the Service

Thanks for the clarification! Done, please have a look.

you forgot to remove this code

aojea · 2023-04-12T08:47:12Z

pkg/healthchecks/healthchecks.go

@@ -142,6 +144,11 @@ func (h *HealthChecks) sync(hc *translator.HealthCheck, bchcc *backendconfigv1.H
 	changes := calculateDiff(filter(existingHC), filter(hc), bchcc)
 	if changes.hasDiff() {
 		klog.V(2).Infof("Health check %q needs update (%s)", existingHC.Name, changes)
+		if flags.F.EnableUpdateCustomHealthCheckDescription && changes.size() == 1 && changes.has("Description") {
+			message := fmt.Sprintf("Healthcheck will be updated and the only field updated is Description.\nOld: %+v\nNew: %+v\nDiff: %+v", existingHC, hc, changes)


this seems a very large message from an event, can we just send the diff or do we miss some important information?

The if guarantees that the diff contains only the Description update. Recall from our discussions that this event emission was intended as a precaution during Description update, so the event would not serve this purpose well if it contained only the "positive scenario" information.

the message contains the New and Old too, can we send only the changes? do we need the others?

%+v\nNew: %+v\nDiff: %+v", existingHC, hc, changes)

Yes, I'm trying to say that we need the others. The changes aka "diff" inside the if consists only of the modified Description (changes.size() == 1 && changes.has("Description")). However, we want to include more information in the message as a precaution.

A background fact that I should probably mention is that calculateDiff() does not return the full diff. It checks selected fields only (recently Description was added as one of them), sometimes just 2 of them.

aojea · 2023-04-12T08:47:41Z

/ok-to-test

looks good overall, some comments that needs to be addressed

pkg/healthchecks/healthchecks_test.go

aojea · 2023-04-13T20:37:16Z

pkg/healthchecks/healthchecks.go

+		if flags.F.EnableUpdateCustomHealthCheckDescription && changes.size() == 1 && changes.has("Description") {
+			message := fmt.Sprintf("Healthcheck will be updated and the only field updated is Description.\nOld: %+v\nNew: %+v\nDiff: %+v", existingHC, hc, changes)
+			h.eventRecorder.Event(
+				hc.Service, "Warning", "HealthcheckDescriptionUpdate", message)


hmm, I have to apologize, but now that I look it carefully , it seems Service is not the right object to pass, in this case this method is trying to sync bchcc *backendconfigv1.HealthCheckConfig , I think this object is implementing runtime.Object, isnt't it?, that will be the best thing since we can do this only with 4-5 lines of code

Sorry again, I've should have checked this better before doing any suggestion

@aojea we want to emit the even on the Service because UHC is created per Service and one BackendConfig can be assign to multiple Services but all Customer cares is a configuration of HealthCheck for Service.

yeah, Damien commented offline, sounds good

kl52752 · 2023-04-14T08:16:03Z

pkg/backends/backends.go

@@ -41,7 +42,7 @@ type Backends struct {
 }

 // Backends is a Pool.
-var _ Pool = (*Backends)(nil)
+var _ interfaces.Pool = (*Backends)(nil)


why this change was needed?

I think you need to rebase

Discussed offline, done.

kl52752 · 2023-04-14T08:19:02Z

pkg/backends/interfaces/interfaces.go

+	GetService(namespace, name string) (*api_v1.Service, error)
+}
+
+func NewEmptyServiceGetter() ServiceGetter {


kl52752 · 2023-04-14T08:20:51Z

pkg/backends/interfaces/interfaces.go

+
+type fakeServiceGetter struct{}
+
+func (fsg *fakeServiceGetter) GetService(namespace, name string) (*api_v1.Service, error) {


I think it will be better to return some default Service here. I don't think is good to return the empty object. Normally when you get nil error you expect that the Object is properly constructed. This may lead to some crashes I think.

According to the comments ,api_v1.Service has only one non-optional field of type metav1.TypeMeta, which in turn has only optional fields. So it seems that the "empty object" is a valid object.

you can at least fill name and namespace to match function params.
Where is it used?

this should be enough

svc := &v1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: namespace, }, }

It's just used as one of the Event parameters.

@aojea Yeah, I missed your comment earlier, but this is exactly what we ended up with.

kl52752 · 2023-04-14T08:36:41Z

pkg/controller/controller.go

@@ -114,7 +115,8 @@ func NewLoadBalancerController(
 		Interface: ctx.KubeClient.CoreV1().Events(""),
 	})

-	healthChecker := healthchecks.NewHealthChecker(ctx.Cloud, ctx.HealthCheckPath, ctx.DefaultBackendSvcPort.ID.Service)
+	eventRecorder := ctx.Recorder(ctx.Namespace)


the namespace for recorder needs to be the namespace of the Service you wants to emit the event for. And health checker is created one per controller you need to postpone this function to the place where you have your Service.
You probably need context there.

aojea · 2023-04-14T10:57:35Z

just please don't use calls to the apiserver on the getter, remember that you have informers that already have Listers and Getter to a local cache with all the objects information. The other alternative is to plumb down the information on initialization, I'm not familiar with the code, so I can't suggest which one is the best option

DamianSawicki · 2023-04-18T08:29:51Z

pkg/healthchecks/healthchecks.go

@@ -67,9 +69,23 @@ func (h *HealthChecks) new(sp utils.ServicePort) *translator.HealthCheck {
 	hc.Name = sp.BackendName()
 	hc.Port = sp.NodePort
 	hc.RequestPath = h.pathFromSvcPort(sp)
+	service := h.mainService(sp)


I'll flag-gate the following lines.

DamianSawicki · 2023-04-18T11:28:49Z

/uncc kl52752

DamianSawicki · 2023-04-19T14:54:29Z

/assign cezarygerard

cezarygerard · 2023-04-20T15:24:31Z

pkg/healthchecks/healthchecks.go

@@ -142,6 +160,11 @@ func (h *HealthChecks) sync(hc *translator.HealthCheck, bchcc *backendconfigv1.H
 	changes := calculateDiff(filter(existingHC), filter(hc), bchcc)
 	if changes.hasDiff() {
 		klog.V(2).Infof("Health check %q needs update (%s)", existingHC.Name, changes)
+		if flags.F.EnableUpdateCustomHealthCheckDescription && changes.size() == 1 && changes.has("Description") {
+			message := fmt.Sprintf("Healthcheck will be updated and the only field updated is Description.\nOld: %+v\nNew: %+v\nDiff: %+v", existingHC, hc, changes)
+			h.recorderGetter.Recorder(hc.Service.Namespace).Event(


hc.Service Can be null here,

if it error occurred while assigning the hc.Service in pkg/healthchecks/healthchecks.go

if flags.F.EnableUpdateCustomHealthCheckDescription { service := h.mainService(sp) var err error hc.Service, err = h.serviceGetter.GetService(service.Namespace, service.Name) if err != nil { klog.Errorf("Service %s/%s not found: %v.", service.Namespace, service.Name, err) } }

Great catch!

cezarygerard · 2023-04-20T15:27:22Z

pkg/healthchecks/healthchecks.go

@@ -67,9 +69,25 @@ func (h *HealthChecks) new(sp utils.ServicePort) *translator.HealthCheck {
 	hc.Name = sp.BackendName()
 	hc.Port = sp.NodePort
 	hc.RequestPath = h.pathFromSvcPort(sp)
+	if flags.F.EnableUpdateCustomHealthCheckDescription {


you should not be using flags this deep down the logic

convert them into fields somewhere in context or close to main

it makes it much more testable

We had this discussion already with kl52752, panslava, and bowei: #2018 (comment). This is the same flag as there and we're not adding any new non-parallelizable tests in this PR, so I'll stick to the approach bowei suggested in #2018 and keep the flag deep down.

cezarygerard · 2023-04-20T15:34:41Z

pkg/healthchecks/healthchecks.go

+		var err error
+		hc.Service, err = h.serviceGetter.GetService(service.Namespace, service.Name)
+		if err != nil {
+			klog.Errorf("Service %s/%s not found: %v.", service.Namespace, service.Name, err)


why erorr?
we keep going so it should be a Warning

Add one more sentence to this log, what are the consequences (not big, we keep going)

cezarygerard · 2023-04-20T15:36:48Z

pkg/healthchecks/healthchecks.go

@@ -67,9 +69,25 @@ func (h *HealthChecks) new(sp utils.ServicePort) *translator.HealthCheck {
 	hc.Name = sp.BackendName()
 	hc.Port = sp.NodePort
 	hc.RequestPath = h.pathFromSvcPort(sp)
+	if flags.F.EnableUpdateCustomHealthCheckDescription {
+		service := h.mainService(sp)


service is a confusing name here
It suggests it's v1.Service
but it is just name and namespace

see my next comment

cezarygerard · 2023-04-20T15:38:33Z

pkg/healthchecks/healthchecks.go

+	if flags.F.EnableUpdateCustomHealthCheckDescription {
+		service := h.mainService(sp)
+		var err error
+		hc.Service, err = h.serviceGetter.GetService(service.Namespace, service.Name)
+		if err != nil {
+			klog.Errorf("Service %s/%s not found: %v.", service.Namespace, service.Name, err)
+		}
+	}


this whole part can be well hidden:
see this:

// new returns a *HealthCheck with default settings and specified port/protocol func (h *HealthChecks) new(sp utils.ServicePort) *translator.HealthCheck { var hc *translator.HealthCheck if sp.NEGEnabled && !sp.L7ILBEnabled { hc = translator.DefaultNEGHealthCheck(sp.Protocol) } else if sp.L7ILBEnabled { hc = translator.DefaultILBHealthCheck(sp.Protocol) } else { hc = translator.DefaultHealthCheck(sp.NodePort, sp.Protocol) } // port is the key for retrieving existing health-check // TODO: rename backend-service and health-check to not use port as key hc.Name = sp.BackendName() hc.Port = sp.NodePort hc.RequestPath = h.pathFromSvcPort(sp) hc.Service = h.getService(sp) return hc } func (h *HealthChecks) getService(sp utils.ServicePort) *v1.Service { if flags.F.EnableUpdateCustomHealthCheckDescription { namespacedName := h.mainService(sp) var err error service, err := h.serviceGetter.GetService(namespacedName.Namespace, namespacedName.Name) if err != nil { klog.Errorf("Service %s/%s not found: %v.", namespacedName.Namespace, namespacedName.Name, err) } return service } return nil } func (h *HealthChecks) mainService(sp utils.ServicePort) types.NamespacedName { service := h.defaultBackendSvc if sp.ID.Service.Name != "" { service = sp.ID.Service } return service }

Much better, I somehow missed it!

cezarygerard · 2023-04-20T15:40:18Z

pkg/healthchecks/healthchecks.go

+		if flags.F.EnableUpdateCustomHealthCheckDescription && changes.size() == 1 && changes.has("Description") {
+			message := fmt.Sprintf("Healthcheck will be updated and the only field updated is Description.\nOld: %+v\nNew: %+v\nDiff: %+v", existingHC, hc, changes)
+			h.recorderGetter.Recorder(hc.Service.Namespace).Event(
+				hc.Service, "Warning", "HealthcheckDescriptionUpdate", message)


this is Info type of message, not Warning

be cautious what we log to customer projects ;-)

also use const from this package
vendor/k8s.io/api/core/v1/types.go

as we do in other places

// Valid values for event types (new types could be added in future) const ( // Information only and will not cause any problems EventTypeNormal string = "Normal" // These events are to warn that something might go wrong EventTypeWarning string = "Warning" )

cezarygerard · 2023-04-20T15:45:30Z

pkg/healthchecks/interfaces.go

+
+// ServiceGetter is an interface to retrieve Kubernetes Services.
+type ServiceGetter interface {
+	GetService(namespace, name string) (*v1.Service, error)
+}
+
+func NewFakeServiceGetter() ServiceGetter {
+	return &fakeServiceGetter{}
+}
+
+type fakeServiceGetter struct{}
+
+func (fsg *fakeServiceGetter) GetService(namespace, name string) (*v1.Service, error) {
+	return &v1.Service{
+		ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name},
+	}, nil
+}
+
+type RecorderGetter interface {
+	Recorder(ns string) record.EventRecorder
+}
+
+func NewFakeRecorderGetter(bufferSize int) RecorderGetter {
+	return &fakeRecorderGetter{bufferSize}
+}
+
+type fakeRecorderGetter struct {
+	bufferSize int
+}
+
+// Returns a different record.EventRecorder for every call.
+func (frg *fakeRecorderGetter) Recorder(namespace string) record.EventRecorder {
+	return record.NewFakeRecorder(frg.bufferSize)
+}
+
+type singletonFakeRecorderGetter struct {
+	recorder *record.FakeRecorder
+}
+
+// Returns the same record.EventRecorder irrespective of the namespace.
+func (sfrg *singletonFakeRecorderGetter) Recorder(namespace string) record.EventRecorder {
+	return sfrg.FakeRecorder()
+}
+
+func (sfrg *singletonFakeRecorderGetter) FakeRecorder() *record.FakeRecorder {
+	if sfrg.recorder == nil {
+		panic("singletonFakeRecorderGetter not initialised: recorder is nil.")
+	}
+	return sfrg.recorder
+}
+
+func NewFakeSingletonRecorderGetter(bufferSize int) *singletonFakeRecorderGetter {
+	return &singletonFakeRecorderGetter{recorder: record.NewFakeRecorder(bufferSize)}
+}


all fakes should be moved to _test.go file

Moved to a new flakes.go file because somehow when I moved it to healthchecks_test.go it was not visible in other packages.

yeah... so it has to be compiled into final binary

I have tried to use build constraint

//go:build test

or

//go:build testing

but it does not work

golang keeps amazing me :-D

https://stackoverflow.com/a/22277807

cezarygerard · 2023-04-21T14:43:22Z

/lgtm

k8s-ci-robot · 2023-04-21T14:43:32Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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 Apr 7, 2023

k8s-ci-robot requested review from aojea and code-elinka April 7, 2023 14:02

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 Apr 7, 2023

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

k8s-ci-robot assigned aojea Apr 7, 2023

aojea reviewed Apr 12, 2023

View reviewed changes

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

aojea reviewed Apr 12, 2023

View reviewed changes

vendor/k8s.io/cloud-provider-gcp/providers/gce/gce.go Outdated Show resolved Hide resolved

aojea reviewed Apr 12, 2023

View reviewed changes

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 Apr 12, 2023

DamianSawicki force-pushed the emit-event-on-desc-change branch 2 times, most recently from d396a05 to 9d5fea2 Compare April 12, 2023 09:18

aojea reviewed Apr 12, 2023

View reviewed changes

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

DamianSawicki force-pushed the emit-event-on-desc-change branch 2 times, most recently from 66f7f13 to ad6a07b Compare April 13, 2023 17:00

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 Apr 13, 2023

aojea reviewed Apr 13, 2023

View reviewed changes

kl52752 reviewed Apr 14, 2023

View reviewed changes

DamianSawicki force-pushed the emit-event-on-desc-change branch 3 times, most recently from cb61a41 to 60aa3d0 Compare April 14, 2023 12:45

DamianSawicki force-pushed the emit-event-on-desc-change branch 2 times, most recently from d851dac to d726038 Compare April 14, 2023 15:16

DamianSawicki marked this pull request as ready for review April 17, 2023 21:50

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 Apr 17, 2023

k8s-ci-robot requested a review from aojea April 17, 2023 21:51

DamianSawicki commented Apr 18, 2023

View reviewed changes

DamianSawicki force-pushed the emit-event-on-desc-change branch from d726038 to daa03ce Compare April 18, 2023 08:30

k8s-ci-robot assigned cezarygerard Apr 19, 2023

DamianSawicki force-pushed the emit-event-on-desc-change branch from daa03ce to 497877a Compare April 20, 2023 09:24

cezarygerard reviewed Apr 20, 2023

View reviewed changes

Emit event on Description-only healthcheck update

fe47055

DamianSawicki force-pushed the emit-event-on-desc-change branch from 497877a to fe47055 Compare April 21, 2023 08:23

DamianSawicki requested a review from cezarygerard April 21, 2023 09:00

cezarygerard approved these changes Apr 21, 2023

View reviewed changes

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

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

k8s-ci-robot merged commit f965f77 into kubernetes:master Apr 21, 2023


		type fakeServiceGetter struct{}

		func (fsg fakeServiceGetter) GetService(namespace, name string) (api_v1.Service, error) {

Emit event on Description-only healthcheck update #2072

Emit event on Description-only healthcheck update #2072

Conversation

DamianSawicki commented Apr 7, 2023 • edited Loading

k8s-ci-robot commented Apr 7, 2023

DamianSawicki commented Apr 7, 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

aojea commented Apr 12, 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

DamianSawicki Apr 14, 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

DamianSawicki Apr 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Apr 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DamianSawicki commented Apr 18, 2023

DamianSawicki commented Apr 19, 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

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

cezarygerard commented Apr 21, 2023

k8s-ci-robot commented Apr 21, 2023

DamianSawicki commented Apr 7, 2023 •

edited

Loading

DamianSawicki Apr 14, 2023 •

edited

Loading

DamianSawicki Apr 14, 2023 •

edited

Loading