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.

Sign up for GitHub

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

Jump to bottom

Add Conditions to status #91

Merged
merged 7 commits into from
Jan 17, 2023
Merged

Add Conditions to status #91

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
39b3c2e
Add Conditions to status
tmshort Jan 11, 2023
29631dc
fixup! Add Conditions to status
tmshort Jan 11, 2023
f3d0987
fixup! Add Conditions to status
tmshort Jan 11, 2023
1a3d231
fixup! Add Conditions to status
tmshort Jan 11, 2023
407cd2f
fixup! Add Conditions to status
tmshort Jan 11, 2023
4e05d47
fixup! Add Conditions to status
tmshort Jan 13, 2023
9f797cc
fixup! Add Conditions to status
tmshort Jan 17, 2023
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
View file
Open in desktop
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
14 changes: 13 additions & 1 deletion api/v1alpha1/operator_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,20 @@ type OperatorSpec struct {
PackageName string `json:"packageName"`
}

const (
TypeReady = "Ready"

StatusNotImplemented = "NotImplemented"
tmshort marked this conversation as resolved.
Show resolved Hide resolved
)

// 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
View file
Open in desktop

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
View file
Open in desktop
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
47 changes: 46 additions & 1 deletion controllers/operator_controller.go
View file
Open in desktop
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,48 @@ 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)

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

// 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})
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@tmshort tmshort Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(And possibly metadata)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should rukpak be updated as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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")
	}

Copy link
Member

@awgreene awgreene Jan 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@awgreene awgreene Jan 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.


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.StatusNotImplemented,
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
View file
Open in desktop
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("perator-test-%s", rand.String(8))
awgreene marked this conversation as resolved.
Show resolved Hide resolved
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?

})
})