cluster: don't report failed on controller restore #629
Conversation
| @@ -211,11 +211,10 @@ func (c *Cluster) run(stopC <-chan struct{}, wg *sync.WaitGroup) { | |||
| if needDeleteCluster { | |||
| c.logger.Infof("deleting cluster") | |||
| c.delete() | |||
| go c.reportFailedStatus() | |||
There was a problem hiding this comment.
this does not seem right either. we should still report failure if reconcile fails, not just when we delete the cluster.
also for cluster delete case, the tpr might already be deleted. there is no point to update it.
There was a problem hiding this comment.
if reconcile fails => needDeleteCluster = true
the tpr might already be deleted => See reportFailedStatus() impl. It will return if TPR deleted
There was a problem hiding this comment.
we should not delete the cluster when reconcile fails. we should keep the trp so that user can see why it fails. user should delete the tpr themselves.
There was a problem hiding this comment.
we should not delete the cluster when reconcile fails.
Reconcile only fail when it should. Create an new issue and let's discuss elsewhere.
This PR is trying to fix controller restore case. We shouldn't report failed.
we should keep the trp so that user can see why it fails.
Operator didn't delete it.
user should delete the tpr themselves.
Same as above
There was a problem hiding this comment.
Reconcile only fail when it should. Create an new issue and let's discuss elsewhere.
This pr is incorrect. Now the code logic is that we report TPR status as failure when the cluster TPR is deleted. This makes no sense to me.
There was a problem hiding this comment.
This pr doesn't change previous logic. If reconcile fail, we already set status failed.
Actually this PR is fixing it. If reconcile exits and needDeleteCluster = true, it must be error case and must be reported failed. But if needDeleteCluster != true, we shouldn't report failed
There was a problem hiding this comment.
The right logic is to set cluster TPR status to fail when cluster fails. It should be straightforward and obvious.
That's what's happening now and what this PR is going to fix.
There was a problem hiding this comment.
I am going to fix this in another pr. needDeleteCluster is incorrect and confusing at the first place. That is why we put c.reportFailedStatus() outside before without even noticing it is wrong.
There was a problem hiding this comment.
I totally understand why the current behavior is wrong. but the root problem is the logical error.
moving reportFailedStatus into a bad designed condition will not make things better from code logic perspective.
|
Closing in favor of #631 |
If needDeleteCluster is false, it's controller restore case.
On controller restore, we didn’t delete or fail the cluster, and shouldn't report failed.