Use statefulset.IsReady() to validate status #430
Conversation
|
@slawqo @karelyatin can you please review if this is appropriate for the ovndbcluster statefulset. I did not fully get the current envtest and the update I did is good. |
stuggi
requested review from
slawqo and
karelyatin
and removed request for
viroel
| Eventually(func(g Gomega) { | ||
| conditions := GetOVNDBCluster(dbs[0]).Status.Conditions | ||
| cond := conditions.Get(condition.ExposeServiceReadyCondition) | ||
| g.Expect(cond.Status).To(Equal(corev1.ConditionFalse)) | ||
| }).Should(Succeed()) |
There was a problem hiding this comment.
this error part is not what I got.
There was a problem hiding this comment.
that error was added on purpose some months back by @averdagu , let's check that first
There was a problem hiding this comment.
Hi, sorry for late response. This test was added due to OSPRH-6798. Where it could happen some panic due to checking Out of Range item on a list. What this test is checking is to not report ready if pods are not created.
Eventho this test scales up the replicas to 3, reconcile loop won't create the new pods (and len(pods) will always be 1), this is like this due to pods are created on function CreateOVNDBClusters more specifically on SimulateStatefulSetReplicaReadyWithPods.
I checked the code with master, tomorrow will check code with your code to see why you're not seeing this error :)
| @@ -662,7 +665,7 @@ func (r *OVNDBClusterReconciler) reconcileNormal(ctx context.Context, instance * | |||
| return ctrl.Result{}, err | |||
| } | |||
|
|
|||
| if instance.Status.ReadyCount > 0 && len(svcList.Items) > 0 { | |||
| if statefulset.IsReady(deploy) && len(svcList.Items) > 0 { | |||
There was a problem hiding this comment.
I may be wrong here but I don't really know why we need this additional condition there really. Maybe @karelyatin will know something more about it
There was a problem hiding this comment.
We need to ensure statefulset is ready before setting Ready Condition
There was a problem hiding this comment.
thats what statefulset.IsReady(deploy) is checking [1]. it is not just checking that we have >0 replica, it is checking:
// IsReady - validates when deployment is ready deployed to whats being requested
// - the requested replicas in the spec matches the ReadyReplicas of the status
// - both when the Generatation of the object matches the ObservedGeneration of the Status
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: slawqo, stuggi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -603,7 +603,10 @@ func (r *OVNDBClusterReconciler) reconcileNormal(ctx context.Context, instance * | |||
| return ctrlResult, nil | |||
| } | |||
|
|
|||
| instance.Status.ReadyCount = sfset.GetStatefulSet().Status.ReadyReplicas | |||
| deploy := sfset.GetStatefulSet() | |||
There was a problem hiding this comment.
hmm we shoudn't use "deploy" vars for statefulsets :) , we need to cleanup some conditions too for this.
There was a problem hiding this comment.
I can do that, I kept it as deploy since a statefulset is basically also a deployment. let me change it
| Eventually(func(g Gomega) { | ||
| conditions := GetOVNDBCluster(dbs[0]).Status.Conditions | ||
| cond := conditions.Get(condition.ExposeServiceReadyCondition) | ||
| g.Expect(cond.Status).To(Equal(corev1.ConditionFalse)) | ||
| }).Should(Succeed()) |
There was a problem hiding this comment.
that error was added on purpose some months back by @averdagu , let's check that first
Changes to use the common IsReady() func to validate a deployment is fully up as requested and e.g. no update rollout in progress. During a minor update this has seen to already report/mark the condition ready, even the deployment is still in progress, or the replacement pod failed. Jira: OSPRH-14472 Depends-On: openstack-k8s-operators/lib-common#616 Signed-off-by: Martin Schuppert <[email protected]>
|
/retest |
Changes to use the common IsReady() func to validate a deployment is fully up as requested and e.g. no update rollout in progress. During a minor update this has seen to already report/mark the condition ready, even the deployment is still in progress, or the replacement pod failed.
Jira: OSPRH-14472
Depends-On: openstack-k8s-operators/lib-common#616