Add Conditions to status #91

tmshort · 2023-01-11T14:41:34Z

Adds an initial Condition: "Ready" with a "False" status.
Fixed Update operator api resources with a status to indicate solver success/failure #73
Include unit-tests

Signed-off-by: Todd Short tshort@redhat.com

For: https://issues.redhat.com/browse/OLM-2854

* Adds an initial Condition: "Ready" with a "False" status. * Fixed operator-framework#73 * Include unit-tests Signed-off-by: Todd Short <tshort@redhat.com>

joelanford · 2023-01-11T15:20:42Z

api/v1alpha1/operator_types.go

 // OperatorStatus defines the observed state of Operator
-type OperatorStatus struct{}
+type OperatorStatus struct {
+	Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`


I haven't seen (and I haven't looked very hard) patchStrategy and patchMergeKey on conditions before. Is this something suggested in official k8s docs somewhere?

I'm wondering what the implications are? It seems like this would permit two writers to server-side apply two different types concurrently? Do we want that?

Yes, these are the recommended attributes:
https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Condition
What it should mean is that you can only have a single instance of each type.

Do we also need the other tags for Conditions that are listed in the example FooStatus on that page? I'm not actually sure, but it's definitely worth adding them and running make generate manifests to see what they do to the generated code and/or CRD file.

type FooStatus struct{ // Represents the observations of a foo's current state. // Known .status.conditions.type are: "Available", "Progressing", and "Degraded" // +patchMergeKey=type // +patchStrategy=merge // +listType=map // +listMapKey=type Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"` // other fields }

It does add something (x-kubernetes-list) values. So I might as well.

you can only have a single instance of each type.

Another follow-up/TODO/ground rule/invariant:

For every condition.Type that we define, that type has a condition set no matter what code path is taken through the reconciler. Every single time reconcile is called, we make sure to evaluate what the value of each condition.Type should be and we make sure it is set that way.

TL;DR:

Reconcile is idempotent. Same input always results in the same output.

Output always includes exactly one of each condition type.

Where input is defined as: "Operator CR (minus its status) + existing cluster state"
And output is defined as: "Operator CR status + new desired cluster state"

controllers/operator_controller.go

controllers/suite_test.go

tmshort · 2023-01-11T16:47:44Z

And now the basic unit tests are actually run...

tmshort · 2023-01-11T18:26:15Z

@joelanford I believe I've addressed all comments

tmshort · 2023-01-12T19:06:29Z

This is for: https://issues.redhat.com/browse/OLM-2854

api/v1alpha1/operator_types.go

awgreene · 2023-01-12T22:11:16Z

controllers/operator_controller.go

+	// Compare resources - ignoring status - and update if required
+	existingOp.Status, reconciledOp.Status = operatorsv1alpha1.OperatorStatus{}, operatorsv1alpha1.OperatorStatus{}
+	if !equality.Semantic.DeepEqual(existingOp, reconciledOp) {
+		if updateErr := r.Update(ctx, reconciledOp); updateErr != nil {
+			return res, utilerrors.NewAggregate([]error{reconcileErr, updateErr})
+		}
+	}


Could you explain when the controller would modify something other than the Operator CR's status? I can't imagine a scenario where we'd want to modify the resource's spec, is this to update something in the metadata (annotations, labels, etc..)? If the point of this update call is to modify something outside the spec, does it make sense to exclude it?

This was suggested by @joelanford; it's copied from the rukpak reconciler. My assumption is to make this a complete wrapper around the reconcile helper function My presumption would be to fill in default values if they were not included in the original resource (if we wanted to do that).

(And possibly metadata)

Finalizers are the only thing that comes to mind. Defaulting should be handled for us by the apiserver during admission. Not reconciliation.

This is a good question though. Perhaps we should have the code panic if it detects a change we made to non-status, non-finalizers, as that would indicate a programming mistake.

Should rukpak be updated as well?

@joelanford: something like?

existingOp.Status, reconciledOp.Status = operatorsv1alpha1.OperatorStatus{}, operatorsv1alpha1.OperatorStatus{} existingOp.Finalizers, reconciledOp.Finalizers = []string{}, []string{} if !equality.Semantic.DeepEqual(existingOp, reconciledOp) { panic("spec or metadata changed by reconciler") }

This would strip finalizers from the object, which we may not want to. Something like this may work.

existingOp.Status, reconciledOp.Status = operatorsv1alpha1.OperatorStatus{}, operatorsv1alpha1.OperatorStatus{} if !equality.Semantic.DeepEqual(existingOp.Spec, reconciledOp.Spec) { panic("spec or metadata changed by reconciler") }

Are you sure? We aren't doing an r.Update() in this case, although that might still be necessary due to updated finalizers. We can't just compare the Specs, as that ignores other metadata.

OK, weirdness! r.Status().Update() actually makes additional modifications to the structure. So, do all the comparisons first, then update the status, panic() if required, then update the spec (should just be the finalizers).
The reconciledOp existingOp and compareOp are all local variables, and k8s is unmodified until an Update() call is made.

Are you sure? We aren't doing an r.Update() in this case, although that might still be necessary due to updated finalizers.

Ah sorry, I didn't notice that you changed the code between the time that I initially reviewed the PR and made that comment.

We can't just compare the Specs, as that ignores other metadata.

Controllers are allowed to edit metadata and remove finalizers, if we're saying that this controller specifically isn't allowed to make meatadata changes, this is fine.

controllers/suite_test.go

controllers/operator_controller.go

awgreene · 2023-01-17T19:12:19Z

controllers/operator_controller.go

+	compareOp := reconciledOp.DeepCopy()
+	existingOp.Status, compareOp.Status = operatorsv1alpha1.OperatorStatus{}, operatorsv1alpha1.OperatorStatus{}
+	existingOp.Finalizers, compareOp.Finalizers = []string{}, []string{}
+	specDiffers := !equality.Semantic.DeepEqual(existingOp, compareOp)


Is compareOp needed? Could you not compare the specs directly?

Suggested change

specDiffers := !equality.Semantic.DeepEqual(existingOp, compareOp)

specDiffers := !equality.Semantic.DeepEqual(&existingOp.Spec, &reconciled.Spec)

We're not comparing just specs, but specs and the metadata as well. Want to make sure no annotations or labels or etc were added.

If we didn't care about the metadata, then that would be correct.

This assumes that we want to allow changes to the metadata field.

In this case, it's only the finalizers that can change. Any other metadata change will cause a panic()

specDiffers could be better named.

How about unexpectedFieldsChanged?

Unresolved only because I'm not sure you'd see my comment. Not merge blocking though.

awgreene · 2023-01-17T19:14:20Z

controllers/operator_controller.go

+	if updateFinalizers {
+		if updateErr := r.Update(ctx, reconciledOp); updateErr != nil {
+			return res, utilerrors.NewAggregate([]error{reconcileErr, updateErr})
+		}
+	}


Rather than "updateFinalizers", should this be "updateMetadata"? CC @joelanford

I assume that there may be instances where we want to change metadata on the object we're reconciling. If we're suggesting that there is not, then we can leave it as is.

A prior comment from @joelanford implies the only thing that the reconciler could change are the Finalizers and the Status. So those are checked explicitly to see if they changed. If anything else was updated, then there's a panic().

Admittedly, updateFinalizers causes all of the resource (sans Status) to be updated, but if anything else were to change, it'd panic() first.

Got it. Thanks.

Yeah, I think it's a good idea to only allow finalizers to be changed. As a rule, reconcilers should only change non-status, non-finalizer fields on objects they manage/own, not their primary objects.

awgreene

Great work @tmshort, it may be worth adding a note that we're choosing not to allow changes to the object's metadata other than its finalizers, but this is fine.

/approve

tmshort · 2023-01-17T19:34:26Z

@joelanford @dtfranz ?

dtfranz

Looks good to me!

joelanford · 2023-01-17T20:53:14Z

controllers/operator_controller.go

+	if specDiffers {
+		panic("spec or metadata changed by reconciler")
+	}


There are probably good arguments on both sides, but did we consider moving this before the status update?

I gave it some thought. But I wasn't sure. Having the Status update at least gives an idea of what error may have led to this condition. But this is "something that shouldn't happen"™, and should never be seen after release.

joelanford · 2023-01-17T21:01:59Z

controllers/operator_controller.go

+		Status:             metav1.ConditionFalse,
+		Reason:             operatorsv1alpha1.ReasonNotImplemented,
+		Message:            "The Reconcile operation is not implemented",
+		ObservedGeneration: op.GetGeneration(),


Oh this reminds me of another way to test for making sure we update all conditions. Unit tests of the Reconcile method should make sure that all condition types have condition objects AND all of those objects have observedGeneration that matches the metadata.generation of the Operator that was reconciled.

joelanford · 2023-01-17T21:05:15Z

controllers/suite_test.go

+		It("has a Condition created", func() {
+			getOperator := &operatorsv1alpha1.Operator{}
+
+			err = k8sClient.Get(ctx, client.ObjectKey{
+				Name: opName,
+			}, getOperator)
+			Expect(err).To(Not(HaveOccurred()))
+
+			// There should always be a "Ready" condition, regardless of Status.
+			conds := getOperator.Status.Conditions
+			Expect(conds).To(Not(BeEmpty()))
+			Expect(conds).To(ContainElement(HaveField("Type", operatorsv1alpha1.TypeReady)))
+		})


Nit, but perhaps some good future proofing to do while things aren't already baked. Could we declare a slice of all of our condition types in the same place the we define them, and then in this test iterate that slice to make sure that all of them exist and have the generation of the opName operator?

joelanford · 2023-01-17T21:05:55Z

controllers/suite_test.go

+			// There should always be a "Ready" condition, regardless of Status.
+			conds := getOperator.Status.Conditions
+			Expect(conds).To(Not(BeEmpty()))
+			Expect(conds).To(ContainElement(HaveField("Type", operatorsv1alpha1.TypeReady)))


Should we also test for the specific Status and Reason we expect?

I decided to not bother with that. As I wanted to minimize changes (vs. additions) to the tests as code is added.

tmshort · 2023-01-17T21:13:11Z

Thank you all. I'll do a follow up to address some of minor issues/nits.

Make sure each condition is defined (as best we can) Enhancements to operator-framework#91 Signed-off-by: Todd Short <tshort@redhat.com>

* Add additional unit-tests for Conditions Make sure each condition is defined (as best we can) Enhancements to #91 Signed-off-by: Todd Short <tshort@redhat.com>

Add Conditions to status

39b3c2e

* Adds an initial Condition: "Ready" with a "False" status. * Fixed operator-framework#73 * Include unit-tests Signed-off-by: Todd Short <tshort@redhat.com>

joelanford reviewed Jan 11, 2023

View reviewed changes

controllers/operator_controller.go Outdated Show resolved Hide resolved

joelanford reviewed Jan 11, 2023

View reviewed changes

controllers/operator_controller.go Outdated Show resolved Hide resolved

joelanford reviewed Jan 11, 2023

View reviewed changes

controllers/operator_controller.go Outdated Show resolved Hide resolved

joelanford reviewed Jan 11, 2023

View reviewed changes

controllers/suite_test.go Outdated Show resolved Hide resolved

tmshort added 3 commits January 11, 2023 11:26

fixup! Add Conditions to status

29631dc

fixup! Add Conditions to status

f3d0987

fixup! Add Conditions to status

1a3d231

fixup! Add Conditions to status

407cd2f

awgreene reviewed Jan 12, 2023

View reviewed changes

api/v1alpha1/operator_types.go Outdated Show resolved Hide resolved

awgreene reviewed Jan 12, 2023

View reviewed changes

controllers/suite_test.go Outdated Show resolved Hide resolved

fixup! Add Conditions to status

4e05d47

tmshort requested review from dtfranz and awgreene January 16, 2023 15:09

fixup! Add Conditions to status

9f797cc

tmshort requested a review from joelanford January 17, 2023 16:14

awgreene reviewed Jan 17, 2023

View reviewed changes

controllers/operator_controller.go Show resolved Hide resolved

awgreene reviewed Jan 17, 2023

View reviewed changes

awgreene approved these changes Jan 17, 2023

View reviewed changes

dtfranz approved these changes Jan 17, 2023

View reviewed changes

joelanford reviewed Jan 17, 2023

View reviewed changes

tmshort merged commit 081c4b0 into operator-framework:main Jan 17, 2023

tmshort added a commit to tmshort/operator-controller that referenced this pull request Jan 17, 2023

Add additional unit-tests for Conditions

b0b532a

Make sure each condition is defined (as best we can) Enhancements to operator-framework#91 Signed-off-by: Todd Short <tshort@redhat.com>

tmshort mentioned this pull request Jan 17, 2023

Add additional unit-tests for Conditions #92

Merged

tmshort added a commit that referenced this pull request Jan 19, 2023

Add additional unit-tests for Conditions (#92)

d1b2189

* Add additional unit-tests for Conditions Make sure each condition is defined (as best we can) Enhancements to #91 Signed-off-by: Todd Short <tshort@redhat.com>

	specDiffers := !equality.Semantic.DeepEqual(existingOp, compareOp)
	specDiffers := !equality.Semantic.DeepEqual(&existingOp.Spec, &reconciled.Spec)

Add Conditions to status #91

Add Conditions to status #91

Conversation

tmshort commented Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort commented Jan 11, 2023

tmshort commented Jan 11, 2023

tmshort commented Jan 12, 2023

Choose a reason for hiding this comment

tmshort Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awgreene Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awgreene Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awgreene left a comment

Choose a reason for hiding this comment

tmshort commented Jan 17, 2023

dtfranz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort commented Jan 17, 2023

tmshort commented Jan 11, 2023 •

edited

Loading

tmshort Jan 13, 2023 •

edited

Loading

awgreene Jan 16, 2023 •

edited

Loading

awgreene Jan 17, 2023 •

edited

Loading

tmshort Jan 17, 2023 •

edited

Loading