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 1 commit
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
8 changes: 7 additions & 1 deletion api/v1alpha1/operator_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,14 @@ type OperatorSpec struct {
PackageName string `json:"packageName"`
}

const (
TypeReady = "Ready"
)

// 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"`
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.

69 changes: 69 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,75 @@ 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
type: object
type: object
served: true
Expand Down
27 changes: 26 additions & 1 deletion controllers/operator_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (
"sigs.k8s.io/controller-runtime/pkg/log"

operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// OperatorReconciler reconciles a Operator object
Expand All @@ -47,10 +49,33 @@ 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)
log := log.FromContext(ctx).WithName("reconcile")

var operator operatorsv1alpha1.Operator
if err := r.Get(ctx, req.NamespacedName, &operator); err != nil {
log.Error(err, "unable to fetch Operator")
return ctrl.Result{}, client.IgnoreNotFound(err)
}

readyCondition := metav1.Condition{
Type: operatorsv1alpha1.TypeReady,
Status: metav1.ConditionFalse,
Reason: "startUp",
tmshort marked this conversation as resolved.
Show resolved Hide resolved
Message: "Operator startup",
ObservedGeneration: operator.GetGeneration(),
}
apimeta.SetStatusCondition(&operator.Status.Conditions, readyCondition)
log.Info("set not ready Condition")

// TODO(user): your logic here

// This is something that needs to happen if any Status is updated,
// even if an error occurs
if err := r.Status().Update(ctx, &operator); err != nil {
tmshort marked this conversation as resolved.
Show resolved Hide resolved
log.Error(err, "unable to update Operator status")
tmshort marked this conversation as resolved.
Show resolved Hide resolved
return ctrl.Result{}, err
}

return ctrl.Result{}, nil
}

Expand Down
64 changes: 63 additions & 1 deletion controllers/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package controllers
package controllers_test

import (
"context"
"path/filepath"
"testing"

Expand All @@ -31,6 +32,10 @@ import (
"sigs.k8s.io/controller-runtime/pkg/log/zap"

operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
"github.com/operator-framework/operator-controller/controllers"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
//+kubebuilder:scaffold:imports
)

Expand Down Expand Up @@ -78,3 +83,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 = "operator-test"
pkgName = "package-test"
tmshort marked this conversation as resolved.
Show resolved Hide resolved

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?

})
})