Skip to content
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

Merged
merged 7 commits into from
Jan 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .github/workflows/unit-test.yaml
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
16 changes: 15 additions & 1 deletion api/v1alpha1/operator_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,22 @@ type OperatorSpec struct {
PackageName string `json:"packageName"`
}

const (
// TODO(user): add more Types
TypeReady = "Ready"

// TODO(user): add more Reasons
ReasonNotImplemented = "NotImplemented"
)

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

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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 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
}

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Reconcile is idempotent. Same input always results in the same output.
  2. 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"

}

//+kubebuilder:object:root=true
//+kubebuilder:resource:scope=Cluster
Expand Down
10 changes: 9 additions & 1 deletion api/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

72 changes: 72 additions & 0 deletions config/crd/bases/operators.operatorframework.io_operators.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,78 @@ spec:
type: object
status:
description: OperatorStatus defines the observed state of Operator
properties:
conditions:
items:
description: "Condition contains details for one aspect of the current
state of this API Resource. --- This struct is intended for direct
use as an array at the field path .status.conditions. For example,
\n 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\"` \n // other fields }"
properties:
lastTransitionTime:
description: lastTransitionTime is the last time the condition
transitioned from one status to another. This should be when
the underlying condition changed. If that is not known, then
using the time when the API field changed is acceptable.
format: date-time
type: string
message:
description: message is a human readable message indicating
details about the transition. This may be an empty string.
maxLength: 32768
type: string
observedGeneration:
description: observedGeneration represents the .metadata.generation
that the condition was set based upon. For instance, if .metadata.generation
is currently 12, but the .status.conditions[x].observedGeneration
is 9, the condition is out of date with respect to the current
state of the instance.
format: int64
minimum: 0
type: integer
reason:
description: reason contains a programmatic identifier indicating
the reason for the condition's last transition. Producers
of specific condition types may define expected values and
meanings for this field, and whether the values are considered
a guaranteed API. The value should be a CamelCase string.
This field may not be empty.
maxLength: 1024
minLength: 1
pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$
type: string
status:
description: status of the condition, one of True, False, Unknown.
enum:
- "True"
- "False"
- Unknown
type: string
type:
description: type of condition in CamelCase or in foo.example.com/CamelCase.
--- Many .condition.type values are consistent across resources
like Available, but because arbitrary conditions can be useful
(see .node.status.conditions), the ability to deconflict is
important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt)
maxLength: 316
pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$
type: string
required:
- lastTransitionTime
- message
- reason
- status
- type
type: object
type: array
x-kubernetes-list-map-keys:
- type
x-kubernetes-list-type: map
type: object
type: object
served: true
Expand Down
58 changes: 57 additions & 1 deletion controllers/operator_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 panic()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specDiffers could be better named.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about unexpectedFieldsChanged?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

@tmshort tmshort Jan 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if updateFinalizers {
if updateErr := r.Update(ctx, reconciledOp); updateErr != nil {
return res, utilerrors.NewAggregate([]error{reconcileErr, updateErr})
}
}
Comment on lines +86 to +90
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 panic().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks.

Copy link
Member

Choose a reason for hiding this comment

The 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(),
Copy link
Member

Choose a reason for hiding this comment

The 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 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.

}
apimeta.SetStatusCondition(&op.Status.Conditions, readyCondition)

// TODO(user): your logic here

Expand Down
66 changes: 65 additions & 1 deletion controllers/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
)

Expand Down Expand Up @@ -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)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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 opName operator?

})
})