annotate a service with the pool used to provide its IP

[cyclinder]

annotate a service with the pool used to provide its IP

Signed-off-by: cyclinder qifeng.guo@daocloud.io

Fixed #1448

[fedepaol]

Neat! Can you modify one of the e2e tests to validate that the annotation is set properly? No need to create a new one I think.

[fedepaol]

Also, can you change the commit message to have the subject starting with a capital letter and provide a body on why the feature is useful? Thanks!

[fedepaol]

Also also, not sure you saw it but the test is failing

[cyclinder]

Yes, I saw it. I will test it in my local, and then update this PR.

[fedepaol]

I think we should reflect.DeepCopy annotations here, as what you are doing is copying the reference of the map (which is owned by svcRo)

[fedepaol]

Also, given this is becoming a bit beefy, consider moving it in a function

[cyclinder]

Thanks, Done.

[cyclinder]

Please review the latest changes, I appreciate it if you give me some advices

[cyclinder]

Hi @fedepaol Can you take a look?

[fedepaol]

yep sorry, I've been stuck with other stuff. I will do a round today / tomrorrow.
Sorry about that :/

[cyclinder]

@fedepaol oh No sorry and No rush. Take your time!

[fedepaol]

You need to call this only if one of the deepequals above changed. I suggest using a flag.

[cyclinder]

sorry I don't notice this

[fedepaol]

I realize only now that this might not work because metallb has writing permissions only on the status subresource https://github.com/metallb/metallb/blob/main/config/rbac/role.yaml#L19
So it might be that this requires additional permissions, and if that is the case I am not sure it's worth it. If it's needed, we can pivot to expose this information via one of the status crds we are working to expose as part of #1685

[cyclinder]

Nope, I made an image with the current branch and did a test for it, and the annotaitons of the service were successfully updated.

[fedepaol]

Why do we need this? svcRo is supposed to be read only

[cyclinder]

We should update status and annotations an same time , and then call UpdateService, Update svcRo's status here, I think it's OK.

[cyclinder]

Otherwise, when updateSvcAnnotations is called, the parameter svc has been reassigned and annotations are lost

[fedepaol]

uhm, never easy... let me think about it, not sure I like this versin because we'll need to remember if we ever add another function that changes something else

[fedepaol]

So (and sorry if I stress too much on this), I think we can keep the "defensive" spirit of the original code by doing this:

	if reflect.DeepEqual(svcRo, svc) {
		level.Debug(l).Log("event", "noChange", "msg", "service converged, no change")
		return successRes
	}

	toWrite := svcRo.DeepCopy()
	if !reflect.DeepEqual(toWrite.Status, svc.Status) {
		toWrite.Status = svc.Status
	}

	if !reflect.DeepEqual(toWrite.Annotations, svc.Annotations) {
		toWrite.Annotatins = svc.Annotations
	}

	if err := c.client.UpdateStatus(svc); err != nil {
		level.Error(l).Log("op", "updateServiceStatus", "error", err, "msg", "failed to update service")
		return controllers.SyncStateError
	}

In theory, the only two things convergeBalancer is supposed to change are the annotations and the status.
We can be even more cautious and call updateStatus when at least one of the two changes

[cyclinder]

No sorry, Your suggestions make sense.

Agree with you, and do we also need a flag to decide UpdateStatus? When annotations or status changed.

[fedepaol]

This looks great, super clean!
LGTM!