Add Failed status field for all ArgoCD Controllers #1164
Conversation
Rizwana777
force-pushed
the
issue-gitops-2472
branch
from
e752c85
to
630c5b8
Compare
controllers/argocd/status.go
Outdated
| @@ -168,6 +168,8 @@ func (r *ReconcileArgoCD) reconcileStatusApplicationSetController(cr *argoproj.A | |||
| if deploy.Spec.Replicas != nil { | |||
| if deploy.Status.ReadyReplicas == *deploy.Spec.Replicas { | |||
| status = "Running" | |||
| } else if deploy.Status.ReadyReplicas < *deploy.Spec.Replicas { | |||
There was a problem hiding this comment.
The Failed state should only be for the case where the Deployment will likely never succeed, for example in the case of ImagePull error (as mentioned by Jaideep on the bug). Whereas here, it appears, the logic will fail if the number of replicas is less than 0 for any reason (for example, the container is starting).
The Failed state is a 'permanent fail', and should only be set when it is likely that it will never succeed. (Jaideep may have other suggestions for cases to chaeck besides image pull fail)
There was a problem hiding this comment.
@jgwest @Rizwana777
Personally, I am of the opinion we should overhaul the entire Argo CD status reporting mechanism we currently have. It seems rather unhelpful to me to just have it say "running" or "pending" without offering any more insight into why that is the case. I would like us to be a bit more comprehensive in how we determine the state of a component and also include a reason for why a component is not running successfully
(something I plan to look into as part of the redesign)
I think we should ideally be looking at the DeploymentConditions in the deployment status to determine what the latest condition is. It also includes the reason if the deployment is failed/unavailable
There was a problem hiding this comment.
I think image pull errors are 1 of many cases where the deployment could fail
another one could be that the pods fail to come up because they don't meet the security constraints set in place by a cluster (like on openshift)
In such cases I would hope that we can rely on the deploymentconditions to tell us what is wrong and we can relay that directly to the user, as that would be better than us trying to hard code specific checks to determine if the deployment has failed for xyz reason or not
There was a problem hiding this comment.
@jaideepr97 you mean instead of checking the state for each pod, we should check for the DeploymentConditions right?
There was a problem hiding this comment.
Got it @jaideepr97 thank you
Rizwana777
force-pushed
the
issue-gitops-2472
branch
2 times, most recently
from
d1361f1
to
8c19725
Compare
|
@jgwest Updated the changes , PTAL |
Rizwana777
force-pushed
the
issue-gitops-2472
branch
2 times, most recently
from
dc3631c
to
773fc99
Compare
|
@jgwest Updated the changes as suggested by Jaideep I am looking into deployment condition to mark the status as |
Signed-off-by: Rizwana777 <rizwananaaz177@gmail.com>
Rizwana777
force-pushed
the
issue-gitops-2472
branch
3 times, most recently
from
d4f3d40
to
1784885
Compare
Signed-off-by: Rizwana777 <rizwananaaz177@gmail.com>
Rizwana777
force-pushed
the
issue-gitops-2472
branch
from
1784885
to
e13a9f7
Compare
Rizwana777
changed the title
svghadi
added
the
backport-to-redesign
What type of PR is this?
What does this PR do / why we need it:
JIRA LINK - https://issues.redhat.com/browse/GITOPS-2472