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. |
dc2f019 to
e6399fd
Compare
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>
Initialize it to False with ReconcilePendingReason. If the controller exits early due to error before it reaches the point where the condition is updated to the actual value, the condition will be left in a consistent state Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
e6399fd to
a151482
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
|
| conditions.Set(s.BorderGateway, metav1.Condition{ | ||
| Type: v1alpha1.ConfiguredCondition, | ||
| Status: metav1.ConditionFalse, | ||
| Reason: v1alpha1.ReconcilePendingReason, | ||
| Message: "Reconciliation is in progress", | ||
| }) |
There was a problem hiding this comment.
General Question: Did we agree that we want to reset the status condition like this at the start of the reconcilation?
There was a problem hiding this comment.
I think we discussed this on our sync last week and we agreed on this, but correct me if I'm wrong. We can discuss it again today if needed.
There was a problem hiding this comment.
Have we validated this outside of the tests? I would expect the resetting on the beginning of the reconciliation to cause a reconciliation loop since the objects status is changed during each iteration. As soon as the status is flipped the condition will get a new LastTransitionedAt timestamp, so even for an otherwise noop each reconciliation will end up requeueing the object.
4 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.
ConfiguredConditiontoConditionFalsewithReconcilePendingReasonto ensure consistency if the controller exits early due to an errorconditions.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 setTest reliability
While working on this PR I discovered issues with tests that cause typically 1-2 failures out of 10 runs of the test suite. See my comment below. I've spent lots of time trying to chase complex race conditions and managed to fix some of the failure modes, including:
RequeueIntervaltoEthernetSegmentReconcilerbecause it was not getting re-queued for reconciliationI've separated the test fixes into a separate PR #472 which has already been merged.
However the effort has reached diminishing returns while significantly delaying the completion of this PR. Therefore we've decided to merge this PR and continue to work on improving the tests in follow-up work.