-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Conditions to status #91
Changes from all commits
39b3c2e
29631dc
f3d0987
1a3d231
407cd2f
4e05d47
9f797cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
name: unit-test | ||
|
||
on: | ||
workflow_dispatch: | ||
pull_request: | ||
push: | ||
branches: | ||
- main | ||
|
||
jobs: | ||
unit-test-basic: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v1 | ||
- uses: actions/setup-go@v3 | ||
with: | ||
go-version-file: "go.mod" | ||
- name: Run basic unit tests | ||
run: | | ||
make test |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -19,7 +19,11 @@ package controllers | |||||
import ( | ||||||
"context" | ||||||
|
||||||
"k8s.io/apimachinery/pkg/api/equality" | ||||||
apimeta "k8s.io/apimachinery/pkg/api/meta" | ||||||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||||
"k8s.io/apimachinery/pkg/runtime" | ||||||
utilerrors "k8s.io/apimachinery/pkg/util/errors" | ||||||
ctrl "sigs.k8s.io/controller-runtime" | ||||||
"sigs.k8s.io/controller-runtime/pkg/client" | ||||||
"sigs.k8s.io/controller-runtime/pkg/log" | ||||||
|
@@ -47,7 +51,59 @@ type OperatorReconciler struct { | |||||
// For more details, check Reconcile and its Result here: | ||||||
// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.13.1/pkg/reconcile | ||||||
func (r *OperatorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | ||||||
_ = log.FromContext(ctx) | ||||||
l := log.FromContext(ctx).WithName("reconcile") | ||||||
l.V(1).Info("starting") | ||||||
defer l.V(1).Info("ending") | ||||||
|
||||||
var existingOp = &operatorsv1alpha1.Operator{} | ||||||
if err := r.Get(ctx, req.NamespacedName, existingOp); err != nil { | ||||||
return ctrl.Result{}, client.IgnoreNotFound(err) | ||||||
} | ||||||
|
||||||
reconciledOp := existingOp.DeepCopy() | ||||||
res, reconcileErr := r.reconcile(ctx, reconciledOp) | ||||||
|
||||||
// Do checks before any Update()s, as Update() may modify the resource structure! | ||||||
updateStatus := !equality.Semantic.DeepEqual(existingOp.Status, reconciledOp.Status) | ||||||
updateFinalizers := !equality.Semantic.DeepEqual(existingOp.Finalizers, reconciledOp.Finalizers) | ||||||
|
||||||
// Compare resources - ignoring status & metadata.finalizers | ||||||
compareOp := reconciledOp.DeepCopy() | ||||||
existingOp.Status, compareOp.Status = operatorsv1alpha1.OperatorStatus{}, operatorsv1alpha1.OperatorStatus{} | ||||||
awgreene marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
existingOp.Finalizers, compareOp.Finalizers = []string{}, []string{} | ||||||
specDiffers := !equality.Semantic.DeepEqual(existingOp, compareOp) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is compareOp needed? Could you not compare the specs directly?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we didn't care about the metadata, then that would be correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assumes that we want to allow changes to the metadata field. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case, it's only the finalizers that can change. Any other metadata change will cause a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unresolved only because I'm not sure you'd see my comment. Not merge blocking though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see it! |
||||||
|
||||||
if updateStatus { | ||||||
if updateErr := r.Status().Update(ctx, reconciledOp); updateErr != nil { | ||||||
return res, utilerrors.NewAggregate([]error{reconcileErr, updateErr}) | ||||||
} | ||||||
} | ||||||
|
||||||
if specDiffers { | ||||||
panic("spec or metadata changed by reconciler") | ||||||
} | ||||||
Comment on lines
+82
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are probably good arguments on both sides, but did we consider moving this before the status update? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I gave it some thought. But I wasn't sure. Having the |
||||||
|
||||||
if updateFinalizers { | ||||||
if updateErr := r.Update(ctx, reconciledOp); updateErr != nil { | ||||||
return res, utilerrors.NewAggregate([]error{reconcileErr, updateErr}) | ||||||
} | ||||||
} | ||||||
Comment on lines
+86
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than "updateFinalizers", should this be "updateMetadata"? CC @joelanford There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Admittedly, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. Thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
|
||||||
return res, reconcileErr | ||||||
} | ||||||
|
||||||
// Helper function to do the actual reconcile | ||||||
func (r *OperatorReconciler) reconcile(ctx context.Context, op *operatorsv1alpha1.Operator) (ctrl.Result, error) { | ||||||
|
||||||
// TODO(user): change ReasonNotImplemented when functionality added | ||||||
readyCondition := metav1.Condition{ | ||||||
Type: operatorsv1alpha1.TypeReady, | ||||||
Status: metav1.ConditionFalse, | ||||||
Reason: operatorsv1alpha1.ReasonNotImplemented, | ||||||
Message: "The Reconcile operation is not implemented", | ||||||
ObservedGeneration: op.GetGeneration(), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh this reminds me of another way to test for making sure we update all conditions. Unit tests of the |
||||||
} | ||||||
apimeta.SetStatusCondition(&op.Status.Conditions, readyCondition) | ||||||
|
||||||
// TODO(user): your logic here | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,23 +14,30 @@ See the License for the specific language governing permissions and | |
limitations under the License. | ||
*/ | ||
|
||
package controllers | ||
package controllers_test | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"path/filepath" | ||
"testing" | ||
|
||
. "github.com/onsi/ginkgo/v2" | ||
. "github.com/onsi/gomega" | ||
|
||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/types" | ||
"k8s.io/apimachinery/pkg/util/rand" | ||
"k8s.io/client-go/kubernetes/scheme" | ||
"k8s.io/client-go/rest" | ||
"sigs.k8s.io/controller-runtime/pkg/client" | ||
"sigs.k8s.io/controller-runtime/pkg/envtest" | ||
logf "sigs.k8s.io/controller-runtime/pkg/log" | ||
"sigs.k8s.io/controller-runtime/pkg/log/zap" | ||
"sigs.k8s.io/controller-runtime/pkg/reconcile" | ||
|
||
operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" | ||
"github.com/operator-framework/operator-controller/controllers" | ||
//+kubebuilder:scaffold:imports | ||
) | ||
|
||
|
@@ -78,3 +85,60 @@ var _ = AfterSuite(func() { | |
err := testEnv.Stop() | ||
Expect(err).NotTo(HaveOccurred()) | ||
}) | ||
|
||
var _ = Describe("Reconcile Test", func() { | ||
When("an Operator is created", func() { | ||
var ( | ||
operator *operatorsv1alpha1.Operator | ||
ctx context.Context | ||
opName string | ||
pkgName string | ||
err error | ||
) | ||
BeforeEach(func() { | ||
ctx = context.Background() | ||
|
||
opName = fmt.Sprintf("operator-test-%s", rand.String(8)) | ||
pkgName = fmt.Sprintf("package-test-%s", rand.String(8)) | ||
|
||
operator = &operatorsv1alpha1.Operator{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: opName, | ||
}, | ||
Spec: operatorsv1alpha1.OperatorSpec{ | ||
PackageName: pkgName, | ||
}, | ||
} | ||
err = k8sClient.Create(ctx, operator) | ||
Expect(err).To(Not(HaveOccurred())) | ||
|
||
or := controllers.OperatorReconciler{ | ||
k8sClient, | ||
scheme.Scheme, | ||
} | ||
_, err = or.Reconcile(ctx, reconcile.Request{ | ||
NamespacedName: types.NamespacedName{ | ||
Name: opName, | ||
}, | ||
}) | ||
Expect(err).To(Not(HaveOccurred())) | ||
}) | ||
AfterEach(func() { | ||
err = k8sClient.Delete(ctx, operator) | ||
Expect(err).To(Not(HaveOccurred())) | ||
}) | ||
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))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also test for the specific Status and Reason we expect? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided to not bother with that. As I wanted to minimize changes (vs. additions) to the tests as code is added. |
||
}) | ||
Comment on lines
+130
to
+142
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
}) | ||
}) |
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.
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?
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.
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.
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.
Do we also need the other tags for
Conditions
that are listed in the exampleFooStatus
on that page? I'm not actually sure, but it's definitely worth adding them and runningmake generate manifests
to see what they do to the generated code and/or CRD file.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.
It does add something (
x-kubernetes-list
) values. So I might as well.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.
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 eachcondition.Type
should be and we make sure it is set that way.TL;DR:
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"