Add Configured condition to all controllers - #426
Conversation
felix-kaestner
left a comment
felix-kaestner
left a comment
There was a problem hiding this comment.
As we are adding a new condition to these resources, should we also add a printcolumn like so?
diff --git a/api/core/v1alpha1/acl_types.go b/api/core/v1alpha1/acl_types.go
index 6501f362..a7a01b70 100644
--- a/api/core/v1alpha1/acl_types.go
+++ b/api/core/v1alpha1/acl_types.go
@@ -122,6 +122,7 @@ type AccessControlListStatus struct {
// +kubebuilder:printcolumn:name="Device",type=string,JSONPath=`.spec.deviceRef.name`
// +kubebuilder:printcolumn:name="Entries",type=string,JSONPath=`.status.entriesSummary`,priority=1
// +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].status`
+// +kubebuilder:printcolumn:name="Configured",type=string,JSONPath=`.status.conditions[?(@.type=="Configured")].status`,priority=1
// +kubebuilder:printcolumn:name="Paused",type=string,JSONPath=`.status.conditions[?(@.type=="Paused")].status`,priority=1
// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"
032b437 to
8d76d7d
Compare
|
@adamtrizuljak-sap We merged the Likewise, in this PR we only tackle Thanks a lot for your contribution! 🔥 |
Done. Are there any other resources that need this change? |
felix-kaestner
left a comment
felix-kaestner
left a comment
There was a problem hiding this comment.
We currently have some places in the codebase that use the check for IsReady on resources, that previously only had the Ready condition, e.g.
network-operator/internal/controller/core/bgp_controller.go
Lines 465 to 466 in 046350c
This is combined with a watch trigger to do reconcilation once this condition changes, e.g.
Now that resources like the VRF get a configured condition, I think those checks should also be adjusted to use the IsConfigured check instead of the IsReady check,
network-operator/internal/conditions/conditions.go
Lines 85 to 96 in 046350c
So I think one additional task would be to go through the codebase and find usages of IsReady which were used for resources that only had the Ready condition previously and change those to now check for IsConfigured instead.
1b654c2 to
4ead8e3
Compare
@felix-kaestner I think that this should not be an issue or a change in behavior? At the end of reconciliation, https://github.com/ironcore-dev/network-operator/blob/main/internal/conditions/conditions.go#L119 Therefore if the
Do you think that we should also explicitly evaluate the |
Yes, I think that would be more semantically correct here, as the intention is "I can't configure X before configuring Y". The reason this currently uses the "Ready" condition was simply that these resources didn't have a "Configured" condition, as is also commented in those places. Now that we introduce the "Configured" condition I think we should change that. |
4ead8e3 to
d5f5438
Compare
Got it. @nikatza helped me understand the problem - you were talking about only about the controllers that depend on other referenced resources. For example the |
d5f5438 to
691ea9a
Compare
691ea9a to
2434e13
Compare
|
Update after long time, as I've been (and still am) trying to fix a regression in test reliability... The My idea to fix this is to explicitly set the conditions.Set(s.BGPPeer, metav1.Condition{
Type: v1alpha1.ConfiguredCondition,
Status: metav1.ConditionFalse,
})
// Controller code which may return early due to error...
err = s.Provider.EnsureBGPPeer(ctx, &provider.EnsureBGPPeerRequest{...})
cond := conditions.FromError(err)
conditions.Set(s.BGPPeer, cond)This method has improved the test suite reliability to less than 1 failure out of 10 runs, but it's still not quite there. I will likely have to add this fix to all controllers that have the Configured condition. |
Previously, some controllers did not set the Configured condition after performing the configuration. They only set the Ready condition, which was ambiguous. This PR explicitly sets the Configured condition based on the provider success and ensures that the Ready condition is set correctly at the end of the reconcile loop. - Initialize the `ConfiguredCondition` - Add deferred call of `conditions.RecomputeReady()` to ensure the Ready condition is evaluated and set at the end of the reconcile loop - `cond := conditions.FromError(err)` already returns the Configured condition, so we just remove the next line that was overriding it with the Ready condition - Check `Configured` condition on referenced resources - Update the associated tests to check that the Configured condition has been set - Add Kubebuilder printcolumn for the Configured condition Some controllers depend on referenced resources. E.g. BGPPeer controller watches referenced BGP and VRF resources and triggers a self-reconciliation if their status changes. Since these resources now expose a proper Configured condition, we update the checking logic to use it instead of the Ready condition (which is now too broad) Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
78339ed to
17ed13a
Compare
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
3 participants
Previously, some controllers did not set the Configured condition after performing the configuration. They only set the Ready condition, which was ambiguous. This PR explicitly sets the Configured condition based on the provider success and ensures that the Ready condition is set correctly at the end of the reconcile loop.
ConfiguredConditionconditions.RecomputeReady()to ensure the Ready condition is evaluated and set at the end of the reconcile loopcond := conditions.FromError(err)already returns the Configured condition, so we just remove the next line that was overriding it with the Ready conditionTodo
ReadyConditiontoConfiguredCondition- I suspect both conditions should be set