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"
adamtrizuljak-sap
force-pushed
the
feat/configured-condition
branch
2 times, most recently
from
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.
https://github.wdf.sap.corp/sap-cloud-infrastructure/neutron-issues/issues/361 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 - Update the associated tests to check that the Configured condition has been set - Add Kubebuilder printcolumn for the Configured condition Set Configured condition in ACL controller Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com> Add Configured condition to controllers Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com> Add Kubebuilder printcolumn for the Configured condition Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com> Fix configured condition in BGP, PIM controller tests Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com> Code gen Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com> Update DHCPRelay test Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com> Set Configured condition in AAA controller Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com> Set Configured condition in Cisco NX BorderGateway, System controllers Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
adamtrizuljak-sap
force-pushed
the
feat/configured-condition
branch
from
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 |
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
https://github.wdf.sap.corp/sap-cloud-infrastructure/neutron-issues/issues/361
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