Ensure all objects are reconciled at least once

[adrianludwin]

While looking into a sporadic but reproducible (maybe 1/5 times) failure
of the rolebinding recreation test, I observed that the objects in the
parent namespace were occasionally not being reconciled after HNC
restarted. This appeared to be because the HC reconciler was not
modifying either the HC or the namespace, and so was skipping the update
of the object reconcilers. This resulted in source objects not being
inserted into the forest, and as a result, propagated objects were being
removed.

By adding a check that the object reconcilers are called when a
namespace is first added to the forest, we can be certain that all
objects are present in the forest.

Tested: ran the rolebinding e2e test 10/10 times successfully.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianludwin

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 [adrianludwin]

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

[adrianludwin]

/assign @yiqigao217
/assign @GinnyJI
/cc @rjbez17

[yiqigao217]

While looking into a sporadic but reproducible (maybe 1/5 times) failure
of the rolebinding recreation test, I observed that the objects in the
parent namespace were occasionally not being reconciled after HNC
restarted. This appeared to be because the HC reconciler was not
modifying either the HC or the namespace, and so was skipping the update
of the object reconcilers.

Why it's flaky but not consistent failure if HC reconciler was not modifying objects? When would it modify objects and cause them to be enqueued to update forest and passed the test?

[adrianludwin]

While looking into a sporadic but reproducible (maybe 1/5 times) failure
of the rolebinding recreation test, I observed that the objects in the
parent namespace were occasionally not being reconciled after HNC
restarted. This appeared to be because the HC reconciler was not
modifying either the HC or the namespace, and so was skipping the update
of the object reconcilers.

Why it's flaky but not consistent failure if HC reconciler was not modifying objects? When would it modify objects and cause them to be enqueued to update forest and passed the test?

I believe it's because there's a race condition between reconciling the objects and the namespace. If the namespace is reconciled first, then when we reconcile the source object, it gets inserted into the forest. But if we reconcile the source object first, then the namespace doesn't exist in the forest and we just ignore the object, under the assumption that when we do sync the namespace, we'll enqueue all the objects in that namespace.

[yiqigao217]

I believe it's because there's a race condition between reconciling the objects and the namespace. If the namespace is reconciled first, then when we reconcile the source object, it gets inserted into the forest. But if we reconcile the source object first, then the namespace doesn't exist in the forest and we just ignore the object, under the assumption that when we do sync the namespace, we'll enqueue all the objects in that namespace.

Sorry that I got a little confused. It seems to me that even before your change, if namespace is reconciled first, if there's no change, r.updateObjects(ctx, log, nm) won't get chance to be called?

[adrianludwin]

Sorry that I got a little confused. It seems to me that even before your change, if namespace is reconciled first, if there's no change, r.updateObjects(ctx, log, nm) won't get chance to be called?

That's correct - if the namespace is reconciled first, then r.updateObjects won't be called. But the problem is here:

https://github.com/kubernetes-sigs/multi-tenancy/blob/master/incubator/hnc/internal/reconcilers/object.go#L263

If the namespace is reconciled first, then even though r.updateObjects isn't called, when the object is reconciled second it will be put into the forest, and everything is fine. But if the object is reconciled first, then ns.Exists() returns false and the object is not placed into the forest. And then, because r.updateObjects isn't called, it's never reconciled again. This fix addresses that second case.

Does this make sense?

[yiqigao217]

Sorry that I got a little confused. It seems to me that even before your change, if namespace is reconciled first, if there's no change, r.updateObjects(ctx, log, nm) won't get chance to be called?

That's correct - if the namespace is reconciled first, then r.updateObjects won't be called. But the problem is here:

https://github.com/kubernetes-sigs/multi-tenancy/blob/master/incubator/hnc/internal/reconcilers/object.go#L263

If the namespace is reconciled first, then even though r.updateObjects isn't called, when the object is reconciled second it will be put into the forest, and everything is fine. But if the object is reconciled first, then ns.Exists() returns false and the object is not placed into the forest. And then, because r.updateObjects isn't called, it's never reconciled again. This fix addresses that second case.

Does this make sense?

Yes, makes sense now. So if namespace reconciles first, it would be fine since the object reconciler would not early exit with actionNop. And this PR is to make sure we don't early exit in the HC reconciler if the namespace is first-time synced and make sure objects are enqueued.

/lgtm

[adrianludwin]

Exactly!

…

On Thu, Oct 1, 2020 at 10:12 AM Yiqi Gao ***@***.***> wrote: Sorry that I got a little confused. It seems to me that even before your change, if namespace is reconciled first, if there's no change, r.updateObjects(ctx, log, nm) won't get chance to be called? That's correct - if the namespace is reconciled first, then r.updateObjects won't be called. But the problem is here: https://github.com/kubernetes-sigs/multi-tenancy/blob/master/incubator/hnc/internal/reconcilers/object.go#L263 If the namespace is reconciled first, then even though r.updateObjects isn't called, when the object is reconciled *second* it will be put into the forest, and everything is fine. But if the *object* is reconciled first, then ns.Exists() returns false and the object is *not* placed into the forest. And then, because r.updateObjects isn't called, it's never reconciled again. This fix addresses that second case. Does this make sense? Yes, makes sense now. So if namespace reconciles first, it would be fine since the object reconciler would not early exit with actionNop. And this PR is to make sure we don't early exit in the HC reconciler if the namespace is first-time synced and make sure objects are enqueued. /lgtm — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1163 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE43PZEE226AZI2RQSGUGBTSISE4JANCNFSM4R7JX6HQ> .