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
e6399fd to
a151482
Compare
| 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.
There was a problem hiding this comment.
This is a very valid concern, if we always set the condition at the start of the reconcilation we alter the status even if we go from Success → Success, which cause an infinite loop of reconcilation (as also status updates trigger another reconcilation) and it also breaks the LastTransitionedAt value.
There was a problem hiding this comment.
I removed this as we discussed. Some failing tests popped up again, which I tried to fix.
|
@IvoGoman correctly noted that overwriting the conditions in every loop updates the The proper way will be to explicitly handle all |
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>
This would always update the LastTransitionedAt timestamp and force another reconciliation Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Wait for Device to be actually created Create a different device, let it reconcile, clean it up and its referenced interfaces Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
…t found Changed from TerminalError to regular error -> controller will retry reconciliation Watche enqueues BGPPeers for reconciliation when a referenced Interface is created or deleted Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
…arvation Tests 1, 5, and 6 created Aggregate Interfaces with MemberInterfaceRefs pointing to "eth1", which didn't exist as a Kubernetes resource. This caused the Interface controller to return a terminal error on every reconcile pass, cycling through 4 passes while holding the device Lease at priority 10. The EthernetSegment controller (priority 1) could never acquire the same Lease within the 60s timeout, so its finalizer was never added. Fix by creating a real Physical Interface as the member reference before each Aggregate Interface. The Interface controller can now resolve all member refs, reconciles successfully, and stops holding the lock. A "Wait for Interface configured" step is added before EthernetSegment creation to ensure the Interface controller has fully finished before the EthernetSegment controller competes for the lock. Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
a151482 to
124a34b
Compare
hardikdr
added
the
area/switch-automation
| Message: fmt.Sprintf("source interface %q not found", addr.InterfaceRef.Name), | ||
| }) | ||
| return reconcile.TerminalError(fmt.Errorf("source interface %q not found", addr.InterfaceRef.Name)) | ||
| return fmt.Errorf("source interface %q not found", addr.InterfaceRef.Name) |
There was a problem hiding this comment.
This should stay a reconcile.TerminalError. The watcher will make sure to retrigger reconcilation when the referenced resource is created. Returning a non-terminal error will otherwise retry over and over again in which there is no point.
There was a problem hiding this comment.
I found that after TerminalError the controller would never be re-enqueued for reconciliation. Maybe this is a deeper issue with envtest, as similar problems are happening all over the test suite.
| Expect(k8sClient.Create(ctx, differentDevice)).To(Succeed()) | ||
| DeferCleanup(func() { | ||
| intfList := &v1alpha1.InterfaceList{} | ||
| Expect(k8sClient.List(ctx, intfList, client.InNamespace(metav1.NamespaceDefault), client.MatchingLabels{v1alpha1.DeviceLabel: differentDevice.Name})).To(Succeed()) | ||
| for i := range intfList.Items { | ||
| Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, &intfList.Items[i]))).To(Succeed()) | ||
| } | ||
| Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, differentDevice))).To(Succeed()) | ||
| }) |
There was a problem hiding this comment.
Why are we deleting interfaces here for a freshly created device? (This shouldn't be neeed, the device uses a GenerateName, so it's most likely unique anyways and doesn't have interface resources linked against it)
There was a problem hiding this comment.
You are right. And envtest should even create a unique namespace for every test, so cleanup shouldn't really be required here. It was added by Claude while trying to fix the tests. I've removed it and it doesn't seem to have an impact on test failures.
|
In the case that the device gets paused, the |
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
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
|
Reconciliation doesn't happen when the device is paused. So I think that it's semantically correct that the current condition might not be reflecting the current state, it will get updated once the device is resumed. Are you suggesting to always set Configured to False (or Unknown) when the device is paused? https://github.com/ironcore-dev/network-operator/blob/main/internal/paused/paused.go#L48 |
5 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.