Fix racy FrameworkVersion and Instance reconciliation #363
Conversation
zen-dog
changed the title
First draft: - `intance_controller` now watches for `FrameworkVersion` events and queues all corresponding instances for reconciliation
… for the race condition
… for the race condition
zen-dog
force-pushed
the
ad/fix-racy-fv-instance
branch
from
f40d5ce
to
d6295c3
Compare
| defer c.Delete(context.TODO(), frameworkVersion) | ||
|
|
||
| log.Printf("Then its instances are reconciled") | ||
| g.Eventually(requests, timeout).Should(gomega.Receive(gomega.Equal(expectedRequest))) |
There was a problem hiding this comment.
We should also make sure that a deploy PlanExecution gets created. Since the Deploy plan only gets triggered when the Instance objects gets updated, it might not be enough to just reconcile the object.
https://github.com/kudobuilder/kudo/blob/master/pkg/controller/instance/instance_controller.go#L69
There was a problem hiding this comment.
That's a very good catch. Because the existing instance hasn't changed, reconciliation won't trigger an execution plan.
There was a problem hiding this comment.
I find it quite strange for a predicate to have side-effects (i.e., to create a PlanExecution). Is this a common/right pattern?
Looking at the Kubebuilder examples, I got the impression that predicates should just inspect events and decide whether the reconciler should take a look at them. But that the reconciler is responsible for making any changes.
It also seems to be a bad practice to look at the status in order to decide whether to apply any changes:
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. That's what we'll do here.
There was a problem hiding this comment.
find it quite strange for a predicate to have side-effects (i.e., to create a PlanExecution).
I agree, however, current logic compares old instance spec with the new one (e.g. for new/updated parameters) and one has access to the old and new object in the predicate but not in the reconciliation function.
|
I wrote in a comment on #361 (comment), but thought I'd include it here too: This integration test can be written using the declarative test harness. Create a test case with three steps:
Documentation is here before it's merged: https://github.com/kudobuilder/kudo/blob/e6afeb0e16e257cc9a2a0b6ca542b55732a10c40/docs/testing.md |
| defer c.Delete(context.TODO(), frameworkVersion) | ||
|
|
||
| log.Printf("Then its instances are reconciled") | ||
| g.Eventually(requests, timeout).Should(gomega.Receive(gomega.Equal(expectedRequest))) |
There was a problem hiding this comment.
I find it quite strange for a predicate to have side-effects (i.e., to create a PlanExecution). Is this a common/right pattern?
Looking at the Kubebuilder examples, I got the impression that predicates should just inspect events and decide whether the reconciler should take a look at them. But that the reconciler is responsible for making any changes.
It also seems to be a bad practice to look at the status in order to decide whether to apply any changes:
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. That's what we'll do here.
Review suggestions Co-Authored-By: Gastón Kleiman <gaston@apache.org>
when a `FrameworkVersion` is **created** and there are instances without an active execution plan.
| return requests | ||
| }) | ||
|
|
||
| if err = c.Watch(&source.Kind{Type: &kudov1alpha1.FrameworkVersion{}}, &handler.EnqueueRequestsFromMapFunc{ToRequests: mapFn}); err != nil { | ||
| // This map function makes sure that we *ONLY* handle created frameworkVersion |
There was a problem hiding this comment.
This brings up a bigger question that's out of scope for here:
IF someone updates a FV, what do we do with the instances?
There was a problem hiding this comment.
Lots of implications here. The instance might drastically change on the next plan run, even if we didn't do anything. Maybe we need to serialize the current FV to the instance.Status or something, idk.
There was a problem hiding this comment.
Yep, the same race is certainly possible with FV+I update.
What type of PR is this?
/component operator
What this PR does / why we need it:
Currently, there is a race condition when applying
FrameworkVersionandInstanceCRDs. The second one relies on the first one to exist for the reconciliation. However, there is no "happened-before" guarantee. To fix this,instance_controllernow watches theFrameworkVersiontoo and reconciles accordingly.Which issue(s) this PR fixes:
Fixes #274
Special notes for your reviewer:
Does this PR introduce a user-facing change?: