Add ability to expose resource reconciliation progress #633

pedjak · 2023-12-27T16:09:37Z

Description of your changes

Optional status.observedGeneration fields has been added to claim/composite/composed status, showing 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 status.conditions[x].observedGeneration represents the .metadata.generation that the condition was set based upon.

Conditions are now aligned with SIG Architecture API convention.

Fixes crossplane/crossplane#4695

Part of crossplane/crossplane#4655

I have:

Read and followed Crossplane's contribution process.
Run make reviewable test to ensure this PR is ready for review.

negz · 2024-01-02T19:48:11Z

apis/common/v1/condition.go

+	// ObservedLabels are the most recent resource metadata.labels
+	// observed by the reconciler
+	// +optional
+	ObserverLabels map[string]string `json:"observedLabels,omitempty"`
+
+	// ObservedLabels are the most recent resource metadata.annotations
+	// observed by the reconciler
+	// +optional
+	ObserverAnnotations map[string]string `json:"observedAnnotations,omitempty"`


Suggested change

// ObservedLabels are the most recent resource metadata.labels

// observed by the reconciler

// +optional

ObserverLabels map[string]string `json:"observedLabels,omitempty"`

// ObservedLabels are the most recent resource metadata.annotations

// observed by the reconciler

// +optional

ObserverAnnotations map[string]string `json:"observedAnnotations,omitempty"`

// ObservedLabels are the most recent resource metadata.labels

// observed by the reconciler

// +optional

ObservedLabels map[string]string `json:"observedLabels,omitempty"`

// ObservedLabels are the most recent resource metadata.annotations

// observed by the reconciler

// +optional

ObservedAnnotations map[string]string `json:"observedAnnotations,omitempty"`

Can you please create a new struct (something like ObservedStatus) for these fields, instead of adding them to ConditionedStatus. The idea of ConditionedStatus is that you can embed it in a status struct to support status conditions. The new struct should be in a different file (not condition.go).

Is there precedent for other tools having observedLabels and observedAnnotations as well as observedGeneration? I'm supportive of observedGeneration but not convinced that it's worth two additional fields for labels and annotations.

@negz great that you spot the typo!

The new struct should be in a different file (not condition.go).

Done, the struct lives now in apis/common/v1/observation.go

Is there precedent for other tools having observedLabels and observedAnnotations as well as observedGeneration? I'm supportive of observedGeneration but not convinced that it's worth two additional fields for labels and annotations.

Value in metadata.generation get increased only when resource spec changes. Given that our controllers react also on label/annotation changes and often use/propagate them during the reconciliation, writing down just the observed generation might not be enough, i.e. an observer would not be able to detect if/when a label/annotation change got processed.

I'm sold on observedGeneration because:

It's succinct, just a generation number not a copy of the whole spec object.

It's an established Kubernetes pattern - it's common (and recommended iirc) to use observedGeneration.

Reacting to spec (i.e. desired state) changes is pretty much always critical.

However I'm not sold on observedLabels and observedAnnotations. Compared to observedGeneration:

It's more verbose - we have to duplicate all labels and annotations under status.

It's not a pattern I've ever seen before. (I didn't search though - let me know if I'm mistaken.)

Reacting to label/annotation changes seems less clearly critical. I can see that it could be in some cases, but it doesn't seem as clear cut at spec.

Do we know of concrete situations (ideally issues) that recording observed labels and annotations would have helped with? If not, I would suggest we omit them until there's evidence they're needed.

Do we know of concrete situations (ideally issues) that recording observed labels and annotations would have helped with? If not, I would suggest we omit them until there's evidence they're needed.

The PR adds an ability to record the observed labels/annotations and the both fields are optional. IMHO a controller should set them if they are used anyhow to influence the reconcilation logic.

In a Crossplane claim, its annotations and labels are copied down to its counterpart composite as a part of reconciliation.

If we do not record the observed claim annotations/lables, then an observer is not able to detect if a reconciliation has happened:

A claim got created and became ready

The claim annotation got added/removed/updated

Given that metadata.generation did not change, status.observedGeneration does not change either after the reconcile

I have found an old issue about asking to increase metadata.generation on annotation/label changes:
kubernetes/kubernetes#67428 and very interesting followup discussion.

@sttts what are your thought on this? Should we stop propagating annotations/labels from claim to composite? Should it be acceptable if claim reconciliation on pure annotation/label changes is not detectable by an observer?

If we do not record the observed claim annotations/lables, then an observer is not able to detect if a reconciliation has happened.

I get that, but I'm challenging whether this has ever been a real problem for someone. Should we wait to see if this is hurting people before we fix it?

Should we stop propagating annotations/labels from claim to composite?

I don't think we can - this would be a breaking behaviour change.

The PR adds an ability to record the observed labels/annotations and the both fields are optional. IMHO a controller should set them if they are used anyhow to influence the reconcilation logic.
In a Crossplane claim, its annotations and labels are copied down to its counterpart composite as a part of reconciliation.
If we do not record the observed claim annotations/lables, then an observer is not able to detect if a reconciliation has happened

I tend to agree with these statements.

That having said, IMO passing down labels/annotations from claim to xrd and similar is wrong, especially for annotations. But as they are passed down today, it sounds logical that the controller records them as part of the observed state.

negz · 2024-01-02T19:49:22Z

pkg/resource/interfaces.go

+
+	SetObservedAnnotations(annotations map[string]string)
+	GetObservedAnnotations() map[string]string
+}


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

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.

sttts · 2024-03-17T20:47:49Z

Which labels and annotations do we have in mind here? In the Kubernetes object model neither should really matter, the spec does.

How can we move this forward? If the observedGeneration is a no-brainer, can we move that forward first?

negz · 2024-03-25T20:49:38Z

If the observedGeneration is a no-brainer, can we move that forward first?

Yes, I think we should do that.

The labels/annotations seems trickier, so I want to hold on that for now:

There's a school of thought that we shouldn't propagate them at all.
observedGeneration is an established pattern in the Kobe world, but I don't think observedLabels etc is.
observedGeneration copies only the generation int, labels etc we'd need to copy the whole object.

apis/common/v1/resource.go

@@ -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"`


* `status.observedGeneration` fields has been added to claim/composite/composed status, showing the latest metadata.generation which resulted in either a ready state, or stalled due to error it can not recover from without human intervention. * `status.conditions[x].observedGeneration represents the .metadata.generation that the condition was set based upon Signed-off-by: Predrag Knezevic <[email protected]>

pedjak · 2024-04-09T09:22:53Z

Yes, I think we should do that.

@negz @sttts I have updated PR, aligned also with SIG Architecture API convention.

This opens up the possibility to achieve compatibility with kstatus specification at some point.

sttts · 2024-04-18T13:37:38Z

@negz can you take another look?

negz · 2024-04-18T17:42:24Z

pkg/resource/unstructured/claim/claim_test.go

+func TestObservedGeneration(t *testing.T) {
+	u := &Unstructured{unstructured.Unstructured{Object: map[string]any{}}}
+	want := int64(123)
+	u.SetObservedGeneration(want)
+
+	if got := u.GetObservedGeneration(); got != want {
+		t.Errorf("u.GetObservedGeneration() got: %v, want %v", got, want)
+	}
+	if g := u.GetUnstructured().Object["status"].(map[string]any)["observedGeneration"]; g != want {
+		t.Errorf("Generations do not match! got: %v (%T)", g, g)
+	}
+}
+
+func TestObservedGenerationNotFound(t *testing.T) {
+	u := &Unstructured{unstructured.Unstructured{Object: map[string]any{}}}
+
+	if g := u.GetObservedGeneration(); g != 0 {
+		t.Errorf("u.GetObservedGeneration(): expected nil, but got %v", g)
+	}
+}


These tests are written in a different style from every other test in the file. Despite the fact that they're mostly tables with a single case, I'd prefer to match the existing practices.

negz · 2024-04-18T17:45:14Z

apis/common/v1/condition.go

@@ -141,6 +157,9 @@ func (s *ConditionedStatus) SetConditions(c ...Condition) {

 			if existing.Equal(new) {


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.

negz · 2024-04-18T17:46:10Z

pkg/resource/unstructured/claim/claim.go

@@ -223,7 +223,7 @@ func (c *Unstructured) SetPublishConnectionDetailsTo(ref *xpv1.PublishConnection

 // GetCondition of this composite resource claim.
 func (c *Unstructured) GetCondition(ct xpv1.ConditionType) xpv1.Condition {
-	conditioned := xpv1.ConditionedStatus{}
+	conditioned := xpv1.ResourceStatus{}


Why change this to ResourceStatus?

negz · 2024-04-18T17:46:13Z

pkg/resource/unstructured/claim/claim.go

@@ -233,7 +233,7 @@ func (c *Unstructured) GetCondition(ct xpv1.ConditionType) xpv1.Condition {

 // SetConditions of this composite resource claim.
 func (c *Unstructured) SetConditions(conditions ...xpv1.Condition) {
-	conditioned := xpv1.ConditionedStatus{}
+	conditioned := xpv1.ResourceStatus{}


Why change this to ResourceStatus?

negz · 2024-04-18T17:46:29Z

pkg/resource/unstructured/composed/composed.go

@@ -72,7 +72,7 @@ func (cr *Unstructured) GetUnstructured() *unstructured.Unstructured {

 // GetCondition of this Composed resource.
 func (cr *Unstructured) GetCondition(ct xpv1.ConditionType) xpv1.Condition {
-	conditioned := xpv1.ConditionedStatus{}
+	conditioned := xpv1.ResourceStatus{}


Why change this to ResourceStatus?

negz · 2024-04-18T17:46:34Z

pkg/resource/unstructured/composed/composed.go

@@ -82,7 +82,7 @@ func (cr *Unstructured) GetCondition(ct xpv1.ConditionType) xpv1.Condition {

 // SetConditions of this Composed resource.
 func (cr *Unstructured) SetConditions(c ...xpv1.Condition) {
-	conditioned := xpv1.ConditionedStatus{}
+	conditioned := xpv1.ResourceStatus{}


Why change this to ResourceStatus?

negz · 2024-04-18T17:47:33Z

pkg/resource/unstructured/claim/claim.go

+
+// SetObservedGeneration of this composite resource claim.
+func (c *Unstructured) SetObservedGeneration(generation int64) {
+	status := &xpv1.ResourceStatus{}


Why not use ObservedStatus? (Here and elsewhere)

Signed-off-by: Predrag Knezevic <[email protected]>

pedjak · 2024-04-19T13:50:11Z

I have addressed your comments @negz ptal.

negz

Thank you!

pedjak requested review from a team as code owners December 27, 2023 16:09

pedjak requested review from negz and MisterMX December 27, 2023 16:09

negz reviewed Jan 2, 2024

View reviewed changes

pedjak force-pushed the add-reconcile-observation branch from 8a33531 to 8fd92c1 Compare January 8, 2024 14:16

negz mentioned this pull request Feb 21, 2024

Enable user to understand when external resource is reconciled crossplane/crossplane#4695

Closed

negz mentioned this pull request Mar 25, 2024

One Pager: Resource Status Improvements crossplane/crossplane#5453

Closed

1 task

negz reviewed Mar 25, 2024

View reviewed changes

pedjak mentioned this pull request Mar 26, 2024

Support observing child resources crossplane-contrib/provider-kubernetes#209

Closed

pedjak force-pushed the add-reconcile-observation branch from 8fd92c1 to b5462b5 Compare April 9, 2024 09:10

negz reviewed Apr 18, 2024

View reviewed changes

Address reviewer comments

d049fcc

Signed-off-by: Predrag Knezevic <[email protected]>

negz approved these changes Apr 19, 2024

View reviewed changes

negz merged commit b121916 into crossplane:master Apr 19, 2024
8 checks passed

pintonunes mentioned this pull request Nov 12, 2024

chore: added observedGeneration to the Ready and Sync condition #789

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to expose resource reconciliation progress #633

Add ability to expose resource reconciliation progress #633

pedjak commented Dec 27, 2023 •

edited

Loading

negz Jan 2, 2024

pedjak Jan 8, 2024

negz Jan 8, 2024 •

edited

Loading

pedjak Jan 9, 2024

negz Jan 9, 2024

negz Jan 9, 2024

sttts Mar 17, 2024

negz Jan 2, 2024

pedjak Jan 8, 2024

sttts commented Mar 17, 2024 •

edited

Loading

negz commented Mar 25, 2024

This comment was marked as duplicate.

This comment was marked as duplicate.

pedjak commented Apr 9, 2024 •

edited

Loading

sttts commented Apr 18, 2024

negz Apr 18, 2024

negz Apr 18, 2024

negz Apr 18, 2024

negz Apr 18, 2024

negz Apr 18, 2024

negz Apr 18, 2024

negz Apr 18, 2024

pedjak commented Apr 19, 2024

negz left a comment

		@@ -141,6 +157,9 @@ func (s *ConditionedStatus) SetConditions(c ...Condition) {

		if existing.Equal(new) {

Add ability to expose resource reconciliation progress #633

Add ability to expose resource reconciliation progress #633

Conversation

pedjak commented Dec 27, 2023 • edited Loading

Description of your changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

negz Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Mar 17, 2024 • edited Loading

negz commented Mar 25, 2024

This comment was marked as duplicate.

This comment was marked as duplicate.

pedjak commented Apr 9, 2024 • edited Loading

sttts commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedjak commented Apr 19, 2024

negz left a comment

Choose a reason for hiding this comment

pedjak commented Dec 27, 2023 •

edited

Loading

negz Jan 8, 2024 •

edited

Loading

sttts commented Mar 17, 2024 •

edited

Loading

pedjak commented Apr 9, 2024 •

edited

Loading