🌱 reconciler/cache/reconciler: simplify and generalize #2609
Conversation
| // the replication function handles the following cases: | ||
| // 1. creation of the object in the cache server when the cached object is not found by retrieveLocalObject | ||
| // 2. deletion of the object from the cache server when the original/local object was removed OR was not found by retrieveLocalObject | ||
| // 1. creation of the object in the cache server when the cached object is not found by getLocalCopy |
There was a problem hiding this comment.
yes, we create an obj when it wasn't found in the globalCache
| if err != nil { | ||
| return err | ||
| // local is gone or being deleted. Delete in cache. | ||
| if !localExists || !localCopy.GetDeletionTimestamp().IsZero() { |
There was a problem hiding this comment.
How does it play with finalizers? Doesn't it remove the object from the cache when finalizers may be holding the local copy? Do we want that?
There was a problem hiding this comment.
we patched the cache server to ignore Finalizers
There was a problem hiding this comment.
very good question. So we should keep updating it, but filter the deletion timestsamp?
Does the cache-server ignore or follow finalizers?
There was a problem hiding this comment.
removing an object is enough
| return err | ||
|
|
||
| // Object doesn't exist anymore, delete it from the global cache. | ||
| logger.V(1).WithValues("cluster", clusterName, "namespace", ns, "name", name).Info("Deleting object from global cache") |
There was a problem hiding this comment.
Nit: it seems that some other controllers log similar events at V(2)
| logger.V(1).WithValues("cluster", clusterName, "namespace", ns, "name", name).Info("Deleting object from global cache") | ||
| if err := r.deleteObject(ctx, clusterName, ns, name); err != nil && !errors.IsNotFound(err) { | ||
| runtime.HandleError(err) | ||
| return nil |
There was a problem hiding this comment.
oh, right, we should retry
| // local exists, global doesn't. Create in cache. | ||
| if !globalExists { | ||
| // TODO: in the future the original RV will have to be stored in an annotation (?) | ||
| // so that the clients that need to modify the original/local object can do i |
sttts
force-pushed
the
sttts-replication-controller-rewrite
branch
from
19e4bde to
af43265
Compare
|
/lgtm |
Co-authored-by: Lukasz Szaszkiewicz <lukasz.szaszkiewicz@gmail.com> Co-authored-by: Stefan Schimanski <sttts@redhat.com>
sttts
force-pushed
the
sttts-replication-controller-rewrite
branch
from
af43265 to
1a046b6
Compare
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sttts 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
Generalized to host more resources more easily and simplified whole flow.
Generalized to host more resources more easily and simplified whole flow.
Generalized to host more resources more easily and simplified whole flow.
Generalized to host more resources more easily and simplified whole flow.
Generalized to host more resources more easily and simplified whole flow.