-
Notifications
You must be signed in to change notification settings - Fork 12
Conversation
Adds stackable reconcile actions to express reconciliation dependencies. Signed-off-by: John Strunk <jstrunk@redhat.com>
@sabose We should use this to as a part of deploying the etcd cluster |
for _, prereq := range ra.prereqs { | ||
cond, err := prereq.Execute(request) | ||
if err != nil || cond != corev1.ConditionTrue { | ||
unknown := corev1.ConditionUnknown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not return the condition from the failed prereq?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The state of the action is unknown... We only know the result of the prereq.
Consider:
- action = ensure devices are registered w/ gd2
- prereq = gd2 node is up
If the node is down or indeterminate, we don't actually know if the devices are registered or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in this case there's no distinction between false and unknown, the only part that matters is "not true". Is there need for any conditions other than true and not true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the end of the day, it's either reconciled or not, so characterizing that as true/not true is accurate.
The goal is to post all these statuses in the CR.status so an admin can understand where things are from the operator POV. In that sense, it's useful to have a distinction between "don't actually know" and "it's not".
Following the example above, we'd see in the CR:
- status.reconcileActions:
- devicesRegistered: Unknown
- nodeIsUp: False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, legit.
Adds an override to hold the x/net package to the 1.10 branch so the code can be built w/ go 1.10.x versions. Also locks Travis to 1.10 to ensure we don't break again. Justification for 1.10: - Downstream builds are performed w/ 1.10. - Kubernetes requires 1.10.2 $ docker run -it --rm docker.io/openshift/origin-release:golang-1.10 go version go version go1.10.3 linux/amd64 Signed-off-by: John Strunk <jstrunk@redhat.com>
The action() functions need access to the client and scheme in order to make changes to system state. Signed-off-by: John Strunk <jstrunk@redhat.com>
This refactors the reconcileaction package to also return a string message as part of the result. The intent is to use this to explain the True/False/Unknown result of the action. It will eventually be placed in the CR status field along with the list of ConditionStatus. As a part of this change, ReconcileAction was renamed to Action since it's in the reconcileaction package, and a Result struct was added to carry both the condition and the message. Signed-off-by: John Strunk <jstrunk@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially with the latest commits, LGTM.
Describe what this PR does
Adds stackable reconcile actions to express reconciliation dependencies.
This is a start at the infrastructure for expressing reconcile dependencies as described in #39 .
Is there anything that requires special attention?
Apologies to those that actually know idiomatic golang.
Related issues:
Related to #39
@phlogistonjohn had expressed interest in this a couple weeks ago.