feature: ncp and nnp conditions#2628
Conversation
dahalperin
requested review from
almaslennikov,
e0ne,
heyvister1 and
rollandf
and removed request for
almaslennikov,
e0ne and
rollandf
Greptile SummaryThis PR adds
Confidence Score: 5/5This PR is safe to merge; it adds a new, additive status field and keeps all existing status fields intact. The conditions implementation follows the standard Kubernetes apimeta.SetStatusCondition pattern correctly, stale-condition pruning is exercised by dedicated tests, and the change is purely additive — no existing status fields or reconcile paths are modified. The ConditionHolder type assertion in updatePolicyCRStatus is guarded and backward-compatible. No files require special attention; the core logic in controllers/conditions.go is well-tested and straightforward. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[updatePolicyCRStatus] --> B[SetAppliedStates and SetPolicyState]
B --> C{cr implements ConditionHolder?}
C -->|No| G[Status Update]
C -->|Yes| D[computePolicyConditions]
D --> E[Iterate StatesStatus]
E --> F{Sync Status}
F -->|SyncStateReady| R[condition True / ComponentReady]
F -->|SyncStateIgnore| S[RemoveStatusCondition + mark seen]
F -->|SyncStateError| T[condition False / ComponentError, hasError=true]
F -->|default NotReady or Reset| U[condition False / ComponentNotReady, hasNotReady=true]
R --> V[SetStatusCondition]
T --> V
U --> V
S --> W[Prune unseen mapped conditions]
V --> W
W --> X{hasError?}
X -->|Yes| Y[Ready=False / ComponentError]
X -->|No| Z{hasNotReady?}
Z -->|Yes| AA[Ready=False / ComponentsNotReady]
Z -->|No| BB[Ready=True / AllComponentsReady]
Y --> CC[SetStatusCondition Ready]
AA --> CC
BB --> CC
CC --> DD[SetConditions on CR]
DD --> G
Reviews (2): Last reviewed commit: "feature: ncp and nnp conditions" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // StateNameToConditionType maps the Name() of a pkg/state.State to the | ||
| // corresponding NicClusterPolicy condition type constant. | ||
| // States absent from this map (e.g. states used only by NicNodePolicy) produce | ||
| // no condition entry. |
There was a problem hiding this comment.
pkg/state/factory.go, newNicNodePolicyStates returns only {ofedState, sharedDpState, sriovDpState} — all three of those state names ("state-OFED", "state-RDMA-device-plugin", "state-SRIOV-device-plugin") are already present in this map. There are no NicNodePolicy states that are absent from the map, so the parenthetical example misleads future readers into thinking NicNodePolicy has its own unmapped states.
| // StateNameToConditionType maps the Name() of a pkg/state.State to the | |
| // corresponding NicClusterPolicy condition type constant. | |
| // States absent from this map (e.g. states used only by NicNodePolicy) produce | |
| // no condition entry. | |
| // StateNameToConditionType maps the Name() of a pkg/state.State to the | |
| // corresponding condition type constant used by both NicClusterPolicy and NicNodePolicy. | |
| // States absent from this map produce no condition entry and do not affect the | |
| // aggregate Ready condition. |
| switch { | ||
| case hasError: | ||
| ready.Status = metav1.ConditionFalse | ||
| ready.Reason = mellanoxv1alpha1.ConditionReasonComponentError | ||
| ready.Message = "One or more components are in error state" | ||
| case hasNotReady: | ||
| ready.Status = metav1.ConditionFalse | ||
| ready.Reason = mellanoxv1alpha1.ConditionReasonComponentsNotReady | ||
| ready.Message = "One or more components are not yet ready" | ||
| default: | ||
| ready.Status = metav1.ConditionTrue | ||
| ready.Reason = mellanoxv1alpha1.ConditionReasonAllComponentsReady | ||
| ready.Message = "" | ||
| } |
There was a problem hiding this comment.
When a policy has no configured components (all states return SyncStateIgnore), neither hasError nor hasNotReady is set, so the Ready condition is emitted as True / AllComponentsReady. A freshly created NicNodePolicy or NicClusterPolicy with an empty spec would immediately surface Ready=True even though nothing has been deployed. The test TestComputePolicyConditionsIgnoredComponentsOmitted codifies this choice, but it is worth confirming it is intentional — an Unknown status for the initial/empty case is a common Kubernetes pattern and may be less surprising to consumers of this condition.
There was a problem hiding this comment.
Intentional. Ready=True means all configured components are ready
Signed-off-by: Dana Halperin <dahalperin@nvidia.com>
|
/retest-nic_operator_kind |
1 similar comment
|
/retest-nic_operator_kind |
3 participants
No description provided.