operator: Handle conflicts in CES update.

[alan-kut]

If for some reason operator has outdated version of CES it will not be able to update such CES and it will never recover from such state.

This can happen not only due to some othe client updating CES but also when the update from operator succeeds but for some reason api-server does't return OK (it can fail after updating etcd).

Signed-off-by: Alan Kutniewski kutniewski@google.com

Fix operator entering broken state when it has outdated version of the CES in the cache.

[maintainer-s-little-helper]

Commit 9c7119712346a4d25c1ebebab955ef8b230ca140 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[alan-kut]

/assign dlapcevic

[alan-kut]

\assign dlapcevic

[sayboras]

Can you also help to correct the sign-off statement in the commit message?

Signed-off-by:  Alan Kutniewski <kutniewski@google.com>

[dlapcevic]

/assign @dlapcevic

[dlapcevic]

/lgtm

[chancez]

This looks a little questionable. Might want to see if you can update the obj.Spec/obj.Status instead, otherwise you might overwrite values added by another update.

[dlapcevic]

I think it's fine because the operator doesn't want to modify CES ObjectMeta.
The operator creates CESs and then only modifies the Endpoints field.

The only remaining concern is that updates will still fail if another source (other than operator) modifies the Endpoints field, but the aim of this PR is to fix the issue of operator getting stuck on its own, just as a part of the k8s system.

[chancez]

I'm not worried about the ObjectMeta data, I'm worried that it's overwriting the entire rest of the object without concern for it's previous values.

[alan-kut]

I don't understand it. I intended to overwrite ObjectMeta only and I thought this is what I did.
This is the same as updateCESInCache code called later:

func (c *cesMgr) updateCESInCache(srcCES *cilium_v2.CiliumEndpointSlice, isDeepCopy bool) {
    if ces, ok := c.desiredCESs.getCESTracker(srcCES.GetName()); ok {
        ces.backendMutex.Lock()
        defer ces.backendMutex.Unlock()
        if !isDeepCopy {
            ces.ces.ObjectMeta = srcCES.ObjectMeta
        } else {
            ces.ces = srcCES
                for _, cep := range ces.ces.Endpoints {
                    // Update the desiredCESs, to reflect all CEPs are packed in a CES
                    c.desiredCESs.insertCEP(GetCEPNameFromCCEP(&cep, ces.ces.Namespace), srcCES.GetName())
                }
            }
        } else {
            log.WithFields(logrus.Fields{
            logfields.CESName: srcCES.GetName(),
            }).Debug("Attempted to updateCESInCache non-existent, skipping.")
    }
}

[alan-kut]

Ah, you are worried that in the apisever we ignore all other possible changes.

What is the case in which we shouldn't do this?
Operator should be the only controller for CES.

If some client changes k8s.Endpoints etc the controllers owning them will revert such changes. The same would happen here (to some extent) with the CiliumEndpointSlice.

Long term I think the operator should always reconcile the full state of CiliumEndpointSlice and make sure they are exactly as operator expects them to be.

[alan-kut]

I don't think it would work better.

It would fix the problem with conflict eventually but it would introduce more problem.
It would always use metadata from the watcher cache so the metadata would be outdated after update before watch gets the event.

This is happening right now with creates. We determine create vs update based on the watch cache, we create if CES is not present in the watch cache. So we observed it happening that we create the CES and operator wants to update it before the watcher obeserves the event so it tries to create again and fails.

[alan-kut]

I could move my change to the handleErr - I think it makes more sense, will update the PR soon

[aojea]

It would always use metadata from the watcher cache so the metadata would be outdated after update before watch gets the event.

that is how kubernetes controllers work, there is no pass through cache and you have to operate with the state of the informer, it eventually be consistent

[aojea]

This is happening right now with creates. We determine create vs update based on the watch cache, we create if CES is not present in the watch cache

that is the part I don't understand, why are those two different operations?

[alan-kut]

PTAL

I changed the place for handling conflict errors to errorhandling in endpointslices.

[alan-kut]

/test

[brb]

Please rebase against the latest main branch to fix the ci-e2e failures.

[alan-kut]

All tests passed except flaky K8sUpstreamNetConformance / kubernetes-e2e-net-conformance (ipv4)

[aojea]

/lgtm

K8sUpstreamNetConformance / kubernetes-e2e-net-conformance (ipv4) (pull_request

this job is flaky and not required and does not seem related, is on my backlog and I hope I can get to it as it is causing a lot of troubles

[nbusseneau]

Considering the above, I have unmarked this PR for backport to 1.11 / 1.12 / 1.13. Please add them back should we revise our stance.