-
Notifications
You must be signed in to change notification settings - Fork 190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
K8SPXC-1152: restore stucks on operator restart #1610
base: main
Are you sure you want to change the base?
Conversation
if err != nil { | ||
return rr, errors.Wrap(err, "run pitr") | ||
switch err { | ||
case errWaitingPods, errWaitingPVC: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we check these errors on the if
line so we can avoid this inner switch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
return rr, nil | ||
} else { | ||
if cluster.Status.ObservedGeneration == cluster.Generation && cluster.Status.PXC.Status == api.AppStateReady { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about this condition. why do we say waiting for cluster to start
only if cluster.Status.PXC.Status
is ready?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rr := reconcile.Result{ | ||
RequeueAfter: time.Second * 5, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
honestly I'm not happy to depend on RequeueAfter
but I guess there's no other way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a way to not depend on RequeueAfter
, but it will take more time to implement. I would like to do it in a separate PR.
if err := s.k8sClient.Get(ctx, types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace}, svc); err != nil { | ||
if k8serrors.IsNotFound(err) { | ||
initInProcess = false | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if err := s.k8sClient.Get(ctx, types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace}, svc); err != nil { | |
if k8serrors.IsNotFound(err) { | |
initInProcess = false | |
} | |
} | |
if err := s.k8sClient.Get(ctx, types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace}, svc); k8serrors.IsNotFound(err) { | |
initInProcess = false | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://perconadev.atlassian.net/browse/K8SPXC-1152 fix unit test fix for pvc restores refactor fix tests fix `security-context` test add unit-test improvements add TODO comment
f86ff81
to
df69c14
Compare
commit: 190e857 |
paused, err := k8s.PauseCluster(ctx, r.client, cluster) | ||
if err != nil { | ||
return rr, errors.Wrapf(err, "stop cluster %s", cluster.Name) | ||
} | ||
if !paused { | ||
log.Info("waiting for cluster to stop", "cluster", cr.Spec.PXCCluster, "msg", err.Error()) | ||
return rr, nil | ||
} | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this commented code?
var oldHAProxySize int32 | ||
if cluster.Spec.HAProxy != nil { | ||
oldHAProxySize = cluster.Spec.HAProxy.Size | ||
switch statusState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should move code for each case to a separate function? this would make code more readable
if err != nil { | ||
err = errors.Wrapf(err, "get cluster %s", cr.Spec.PXCCluster) | ||
if otherRestore != nil { | ||
err = errors.Errorf("unable to continue, concurent restore job %s running now", otherRestore.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: concurent
-> concurrent
rJobsList := &api.PerconaXtraDBClusterRestoreList{} | ||
err := cl.List( | ||
ctx, | ||
rJobsList, | ||
&client.ListOptions{ | ||
Namespace: cr.Namespace, | ||
}, | ||
) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "get restore jobs list") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can write this like that:
rJobsList := &api.PerconaXtraDBClusterRestoreList{}
if err := cl.List(
ctx,
rJobsList,
&client.ListOptions{
Namespace: cr.Namespace,
},
); err != nil {
return nil, errors.Wrap(err, "get restore jobs list")
}
Since the same error is not used after that context
|
||
func setStatus(ctx context.Context, cl client.Client, cr *api.PerconaXtraDBClusterRestore, state api.BcpRestoreStates, comments string) error { | ||
cr.Status.State = state | ||
switch state { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this swtich here really needed since are handling only one case?
bcp := &api.PerconaXtraDBClusterBackup{} | ||
err := cl.Get(ctx, types.NamespacedName{Name: cr.Spec.BackupName, Namespace: cr.Namespace}, bcp) | ||
if err != nil { | ||
err = errors.Wrapf(err, "get backup %s", cr.Spec.BackupName) | ||
return bcp, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to other cases, we can use short statement if style we use throughout the codebase since this error here is not and should not be used in another part of the function.
https://perconadev.atlassian.net/browse/K8SPXC-1152
CHANGE DESCRIPTION
Problem:
If operator pod is restarted during a restore, it can't continue to the restore process.
Cause:
The current design of the restore process is not designed to continue on operator restart.
Solution:
We should refactor the restore code so that the operator can catch up with the current state of the restore and continue.
CHECKLIST
Jira
Needs Doc
) and QA (Needs QA
)?Tests
compare/*-oc.yml
)?Config/Logging/Testability