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 ability to expose resource reconciliation progress #633

Merged
merged 2 commits into from
Apr 19, 2024
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
21 changes: 20 additions & 1 deletion apis/common/v1/condition.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ const (
ReasonReconcilePaused ConditionReason = "ReconcilePaused"
)

// See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties

// A Condition that may apply to a resource.
type Condition struct {
// Type of this condition. At most one of each condition type may apply to
Expand All @@ -74,10 +76,16 @@ type Condition struct {
// one status to another, if any.
// +optional
Message string `json:"message,omitempty"`

// 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.
// +optional
ObservedGeneration int64 `json:"observedGeneration,omitempty"`
}

// Equal returns true if the condition is identical to the supplied condition,
// ignoring the LastTransitionTime.
// ignoring the LastTransitionTime and ObservedGeneration.
func (c Condition) Equal(other Condition) bool {
return c.Type == other.Type &&
c.Status == other.Status &&
Expand All @@ -92,6 +100,13 @@ func (c Condition) WithMessage(msg string) Condition {
return c
}

// WithObservedGeneration returns a condition by adding the provided observed generation
// to existing condition.
func (c Condition) WithObservedGeneration(gen int64) Condition {
c.ObservedGeneration = gen
return c
}

// NOTE(negz): Conditions are implemented as a slice rather than a map to comply
// with Kubernetes API conventions. Ideally we'd comply by using a map that
// marshalled to a JSON array, but doing so confuses the CRD schema generator.
Expand Down Expand Up @@ -131,6 +146,7 @@ func (s *ConditionedStatus) GetCondition(ct ConditionType) Condition {
// SetConditions sets the supplied conditions, replacing any existing conditions
// of the same type. This is a no-op if all supplied conditions are identical,
// ignoring the last transition time, to those already set.
// Observed generation is updated if higher than the existing one.
func (s *ConditionedStatus) SetConditions(c ...Condition) {
for _, new := range c {
exists := false
Expand All @@ -141,6 +157,9 @@ func (s *ConditionedStatus) SetConditions(c ...Condition) {

if existing.Equal(new) {
Copy link
Member

Choose a reason for hiding this comment

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

GitHub won't let me add a comment to the existing.Equal docstring, but it should explicitly call out that it doesn't consider ObservedGeneration when assessing equality. It already calls out that it ignores LastTransitionTime.

exists = true
if existing.ObservedGeneration < new.ObservedGeneration {
existing.ObservedGeneration = new.ObservedGeneration
}
continue
}

Expand Down
29 changes: 29 additions & 0 deletions apis/common/v1/condition_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,3 +228,32 @@ func TestConditionWithMessage(t *testing.T) {
})
}
}

func TestConditionWithObservedGeneration(t *testing.T) {
cases := map[string]struct {
c Condition
observedGeneration int64
want Condition
}{
"Added": {
c: Condition{Type: TypeReady, Reason: ReasonUnavailable},
observedGeneration: 10,
want: Condition{Type: TypeReady, Reason: ReasonUnavailable, ObservedGeneration: 10},
},
"Changed": {
c: Condition{Type: TypeReady, Reason: ReasonUnavailable, ObservedGeneration: 3},
observedGeneration: 10,
want: Condition{Type: TypeReady, Reason: ReasonUnavailable, ObservedGeneration: 10},
},
}

for name, tc := range cases {
t.Run(name, func(t *testing.T) {
got := tc.c.WithObservedGeneration(tc.observedGeneration)

if diff := cmp.Diff(tc.want, got); diff != "" {
t.Errorf("a.Equal(b): -want, +got:\n%s", diff)
}
})
}
}
37 changes: 37 additions & 0 deletions apis/common/v1/observation.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
Copyright 2024 The Crossplane Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package v1

// ObservedStatus contains the recent reconciliation stats.
type ObservedStatus struct {
// ObservedGeneration is the latest metadata.generation
// which resulted in either a ready state, or stalled due to error
// it can not recover from without human intervention.
// +optional
ObservedGeneration int64 `json:"observedGeneration,omitempty"`
}

// SetObservedGeneration sets the generation of the main resource
// during the last reconciliation.
func (s *ObservedStatus) SetObservedGeneration(generation int64) {
s.ObservedGeneration = generation
}

// GetObservedGeneration returns the last observed generation of the main resource.
func (s *ObservedStatus) GetObservedGeneration() int64 {
return s.ObservedGeneration
}
1 change: 1 addition & 0 deletions apis/common/v1/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ type ResourceSpec struct {
// ResourceStatus represents the observed state of a managed resource.
type ResourceStatus struct {
ConditionedStatus `json:",inline"`
ObservedStatus `json:",inline"`

This comment was marked as duplicate.

This comment was marked as duplicate.

}

// A CredentialsSource is a source from which provider credentials may be
Expand Down
16 changes: 16 additions & 0 deletions apis/common/v1/zz_generated.deepcopy.go

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

6 changes: 3 additions & 3 deletions pkg/resource/fake/mocks.go
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ type Composite struct {
ConnectionSecretWriterTo
ConnectionDetailsPublisherTo

xpv1.ConditionedStatus
xpv1.ResourceStatus
ConnectionDetailsLastPublishedTimer
}

Expand All @@ -389,7 +389,7 @@ type Composed struct {
metav1.ObjectMeta
ConnectionSecretWriterTo
ConnectionDetailsPublisherTo
xpv1.ConditionedStatus
xpv1.ResourceStatus
}

// GetObjectKind returns schema.ObjectKind.
Expand Down Expand Up @@ -421,7 +421,7 @@ type CompositeClaim struct {
LocalConnectionSecretWriterTo
ConnectionDetailsPublisherTo

xpv1.ConditionedStatus
xpv1.ResourceStatus
ConnectionDetailsLastPublishedTimer
}

Expand Down
9 changes: 9 additions & 0 deletions pkg/resource/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,12 @@ type ConnectionDetailsPublishedTimer interface {
GetConnectionDetailsLastPublishedTime() *metav1.Time
}

// ReconciliationObserver can track data observed by resource reconciler.
type ReconciliationObserver interface {
SetObservedGeneration(generation int64)
GetObservedGeneration() int64
}
Copy link
Member

Choose a reason for hiding this comment

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

Do you expect that managed resources would also satisfy this interface, eventually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you expect that managed resources would also satisfy this interface, eventually?

Yes, I was just thinking that adding ReconciliationObserver to Managed would be too much for this PR, because then we would need to add the necessary logic to pkg/reconciler/managed/reconciler.go.


// An Object is a Kubernetes object.
type Object interface {
metav1.Object
Expand Down Expand Up @@ -245,6 +251,7 @@ type Composite interface { //nolint:interfacebloat // This interface has to be b

Conditioned
ConnectionDetailsPublishedTimer
ReconciliationObserver
}

// Composed resources can be a composed into a Composite resource.
Expand All @@ -254,6 +261,7 @@ type Composed interface {
Conditioned
ConnectionSecretWriterTo
ConnectionDetailsPublisherTo
ReconciliationObserver
}

// A CompositeClaim for a Composite resource.
Expand All @@ -272,4 +280,5 @@ type CompositeClaim interface { //nolint:interfacebloat // This interface has to

Conditioned
ConnectionDetailsPublishedTimer
ReconciliationObserver
}
11 changes: 10 additions & 1 deletion pkg/resource/interfaces_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@ limitations under the License.

package resource

import "github.com/crossplane/crossplane-runtime/pkg/resource/fake"
import (
"github.com/crossplane/crossplane-runtime/pkg/resource/fake"
"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/claim"
"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/composed"
"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/composite"
)

// We test that our fakes satisfy our interfaces here rather than in the fake
// package to avoid a cyclic dependency.
Expand All @@ -29,4 +34,8 @@ var (
_ CompositeClaim = &fake.CompositeClaim{}
_ Composite = &fake.Composite{}
_ Composed = &fake.Composed{}

_ CompositeClaim = &claim.Unstructured{}
_ Composite = &composite.Unstructured{}
_ Composed = &composed.Unstructured{}
)
15 changes: 15 additions & 0 deletions pkg/resource/unstructured/claim/claim.go
Original file line number Diff line number Diff line change
Expand Up @@ -253,3 +253,18 @@ func (c *Unstructured) GetConnectionDetailsLastPublishedTime() *metav1.Time {
func (c *Unstructured) SetConnectionDetailsLastPublishedTime(t *metav1.Time) {
_ = fieldpath.Pave(c.Object).SetValue("status.connectionDetails.lastPublishedTime", t)
}

// SetObservedGeneration of this composite resource claim.
func (c *Unstructured) SetObservedGeneration(generation int64) {
status := &xpv1.ObservedStatus{}
_ = fieldpath.Pave(c.Object).GetValueInto("status", status)
status.SetObservedGeneration(generation)
_ = fieldpath.Pave(c.Object).SetValue("status.observedGeneration", status.ObservedGeneration)
}

// GetObservedGeneration of this composite resource claim.
func (c *Unstructured) GetObservedGeneration() int64 {
status := &xpv1.ObservedStatus{}
_ = fieldpath.Pave(c.Object).GetValueInto("status", status)
return status.GetObservedGeneration()
}
25 changes: 25 additions & 0 deletions pkg/resource/unstructured/claim/claim_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -368,3 +368,28 @@ func TestConnectionDetailsLastPublishedTime(t *testing.T) {
})
}
}

func TestObservedGeneration(t *testing.T) {
cases := map[string]struct {
u *Unstructured
want int64
}{
"Set": {
u: New(func(u *Unstructured) {
u.SetObservedGeneration(123)
}),
want: 123,
},
"NotFound": {
u: New(),
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
got := tc.u.GetObservedGeneration()
if diff := cmp.Diff(tc.want, got); diff != "" {
t.Errorf("\nu.GetObservedGeneration(): -want, +got:\n%s", diff)
}
})
}
}
15 changes: 15 additions & 0 deletions pkg/resource/unstructured/composed/composed.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,3 +168,18 @@ type UnstructuredList struct {
func (cr *UnstructuredList) GetUnstructuredList() *unstructured.UnstructuredList {
return &cr.UnstructuredList
}

// SetObservedGeneration of this composite resource claim.
func (cr *Unstructured) SetObservedGeneration(generation int64) {
status := &xpv1.ObservedStatus{}
_ = fieldpath.Pave(cr.Object).GetValueInto("status", status)
status.SetObservedGeneration(generation)
_ = fieldpath.Pave(cr.Object).SetValue("status.observedGeneration", status.ObservedGeneration)
}

// GetObservedGeneration of this composite resource claim.
func (cr *Unstructured) GetObservedGeneration() int64 {
status := &xpv1.ObservedStatus{}
_ = fieldpath.Pave(cr.Object).GetValueInto("status", status)
return status.GetObservedGeneration()
}
25 changes: 25 additions & 0 deletions pkg/resource/unstructured/composed/composed_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,3 +146,28 @@ func TestWriteConnectionSecretToReference(t *testing.T) {
})
}
}

func TestObservedGeneration(t *testing.T) {
cases := map[string]struct {
u *Unstructured
want int64
}{
"Set": {
u: New(func(u *Unstructured) {
u.SetObservedGeneration(123)
}),
want: 123,
},
"NotFound": {
u: New(),
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
got := tc.u.GetObservedGeneration()
if diff := cmp.Diff(tc.want, got); diff != "" {
t.Errorf("\nu.GetObservedGeneration(): -want, +got:\n%s", diff)
}
})
}
}
15 changes: 15 additions & 0 deletions pkg/resource/unstructured/composite/composite.go
Original file line number Diff line number Diff line change
Expand Up @@ -258,3 +258,18 @@ func (c *Unstructured) SetEnvironmentConfigReferences(refs []corev1.ObjectRefere
}
_ = fieldpath.Pave(c.Object).SetValue("spec.environmentConfigRefs", filtered)
}

// SetObservedGeneration of this composite resource claim.
func (c *Unstructured) SetObservedGeneration(generation int64) {
status := &xpv1.ObservedStatus{}
_ = fieldpath.Pave(c.Object).GetValueInto("status", status)
status.SetObservedGeneration(generation)
_ = fieldpath.Pave(c.Object).SetValue("status.observedGeneration", status.ObservedGeneration)
}

// GetObservedGeneration of this composite resource claim.
func (c *Unstructured) GetObservedGeneration() int64 {
status := &xpv1.ObservedStatus{}
_ = fieldpath.Pave(c.Object).GetValueInto("status", status)
return status.GetObservedGeneration()
}
25 changes: 25 additions & 0 deletions pkg/resource/unstructured/composite/composite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -356,3 +356,28 @@ func TestConnectionDetailsLastPublishedTime(t *testing.T) {
})
}
}

func TestObservedGeneration(t *testing.T) {
cases := map[string]struct {
u *Unstructured
want int64
}{
"Set": {
u: New(func(u *Unstructured) {
u.SetObservedGeneration(123)
}),
want: 123,
},
"NotFound": {
u: New(),
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
got := tc.u.GetObservedGeneration()
if diff := cmp.Diff(tc.want, got); diff != "" {
t.Errorf("\nu.GetObservedGeneration(): -want, +got:\n%s", diff)
}
})
}
}
Loading