Wrong assumptions and anti-patterns in InstanceController

[gkleiman]

While trying to address a TODO left in the InstanceController (#380) I found a few issues:

1. The current implementation of the watch predicate function assumes that it will always receive both the old and the updated version of an instance. Having read the Kubernetes documentation, this assumption seems to be incorrect:

A given Kubernetes server will only preserve a historical list of changes for a limited time. Clusters using etcd3 preserve changes in the last 5 minutes by default. When the requested watch operations fail because the historical version of that resource is not available, clients must handle the case by recognizing the status code 410 Gone, clearing their local cache, performing a list operation, and starting the watch from the resourceVersion returned by that new list operation.

2. The predicate function reads the instance (controller's root object) status and may have side-effects (i.e., create a PlanExecution). Going through the Kubebuilder tutorial, I saw that they strongly discourage operators from doing that:

Remember, status should be able to be reconstituted from the state of the world, so it's generally not a good idea to read from the status of the root object. Instead, you should reconstruct it every run.

I think that we could address both issues by:

1. Storing parameters in PlanExecution.
2. Making the instances watcher in InstanceController use the default predicate.
3. Moving all the logic from the current predicate to Reconcile. The new implementation of Reconcile would then reconstitute state of the world from the latest PlanExecution, eliminating the need to read the status field or to compare the current version of an instance with an older one.

Note that these changes would most likely break backwards compatibility, so I think we should work on this ASAP.

[gerred]

Yep. Let's talk about this in community meeting. 1 we have definitely waned to address for a while for reasons unrelated to this, so that's great. I'm thinking this could start this next release cycle for v0.3.1 or v0.4.0.

Is there an e2e/intregration test that could make this case fail today just so we have a bar to work against?

[gkleiman]

A test following this scenario would probably lead to incorrect behaviour today:

1. Start k8s and KUDO
2. Add an Instance and wait for the PlanExecution to complete
3. Kill KUDO
4. Modify the instance updating a parameter with a non-default trigger
5. Manually trigger an etcd compaction, e.g., via etcdctl compact <resourceVersion>
6. Restart KUDO, who will probably erroneously act as if no parameter had been changed

[kensipe]

This is a critical issue... scheduling this fix for a 0.4.x bug fix release or for 0.5.0.

[alenkacz]

We started working on this with @kensipe

[alenkacz]

From the docs: OnUpdate is called when an object is modified. Note that oldObj is the last known state of the object-- it is possible that several changes were combined together, so you can't use this to see every single change.