Skip to content
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.

Sign up for GitHub

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-1509: Fix reconciler error when PiTR enabled #1879

Merged
merged 8 commits into from
Jan 30, 2025
Merged

K8SPXC-1509: Fix reconciler error when PiTR enabled #1879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
56d9024
K8SPXC-1509: Fix reconciler error when PiTR enabled
egegunes Nov 23, 2024
a92b87c
Merge branch 'main' into K8SPXC-1509
egegunes Nov 25, 2024
357575d
Merge branch 'main' into K8SPXC-1509
hors Dec 2, 2024
17d12b2
Merge branch 'main' into K8SPXC-1509
egegunes Jan 9, 2025
571c7b9
Merge branch 'main' into K8SPXC-1509
egegunes Jan 15, 2025
ba854da
Merge branch 'main' into K8SPXC-1509
egegunes Jan 22, 2025
c3439e3
Merge branch 'main' into K8SPXC-1509
egegunes Jan 23, 2025
6647ff7
Merge branch 'main' into K8SPXC-1509
egegunes Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 2 additions & 27 deletions pkg/controller/pxc/backup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
logf "sigs.k8s.io/controller-runtime/pkg/log"

api "github.com/percona/percona-xtradb-cluster-operator/pkg/apis/pxc/v1"
"github.com/percona/percona-xtradb-cluster-operator/pkg/k8s"
"github.com/percona/percona-xtradb-cluster-operator/pkg/naming"
"github.com/percona/percona-xtradb-cluster-operator/pkg/pxc/app/deployment"
)
Expand All @@ -40,32 +39,8 @@ func (r *ReconcilePerconaXtraDBCluster) reconcileBackups(ctx context.Context, cr
return errors.Wrap(err, "failed to check if restore is running")
}
if cr.Status.Status == api.AppStateReady && cr.Spec.Backup.PITR.Enabled && !cr.Spec.Pause && !restoreRunning {
initImage, err := k8s.GetInitImage(ctx, cr, r.client)
if err != nil {
return errors.Wrap(err, "failed to get init image")
}
binlogCollector, err := deployment.GetBinlogCollectorDeployment(cr, initImage)
if err != nil {
return errors.Errorf("get binlog collector deployment for cluster '%s': %v", cr.Name, err)
}
err = setControllerReference(cr, &binlogCollector, r.scheme)
if err != nil {
return errors.Wrapf(err, "set controller reference for binlog collector deployment '%s'", binlogCollector.Name)
}

currentCollector := appsv1.Deployment{}
err = r.client.Get(context.TODO(), types.NamespacedName{Name: binlogCollector.Name, Namespace: binlogCollector.Namespace}, &currentCollector)
if err != nil && k8serrors.IsNotFound(err) {
if err := r.client.Create(context.TODO(), &binlogCollector); err != nil && !k8serrors.IsAlreadyExists(err) {
return errors.Wrapf(err, "create binlog collector deployment for cluster '%s'", cr.Name)
}
} else if err != nil {
return errors.Wrapf(err, "get binlog collector deployment '%s'", binlogCollector.Name)
}

currentCollector.Spec = binlogCollector.Spec
if err := r.client.Update(context.TODO(), &currentCollector); err != nil {
return errors.Wrapf(err, "update binlog collector deployment '%s'", binlogCollector.Name)
if err := r.reconcileBinlogCollector(ctx, cr); err != nil {
return errors.Wrap(err, "reconcile binlog collector")
Comment on lines +42 to +43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with @egegunes that if can also add a unit test for the controller if we have some time

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i spend some time on this but binlog collector is only reconciled after cluster is ready and unfortunately on envtest we have no means to make cluster ready

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright thanks for checking this

}
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/controller/pxc/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,7 +1109,7 @@ func (r *ReconcilePerconaXtraDBCluster) createOrUpdate(ctx context.Context, cr *
}

if k8serrors.IsNotFound(err) {
log.V(1).Info("Creating object", "object", obj.GetName())
log.V(1).Info("Creating object", "object", obj.GetName(), "kind", obj.GetObjectKind())
return r.client.Create(ctx, obj)
}

Expand All @@ -1126,7 +1126,7 @@ func (r *ReconcilePerconaXtraDBCluster) createOrUpdate(ctx context.Context, cr *
obj.SetResourceVersion(oldObject.GetResourceVersion())
}

log.V(1).Info("Updating object", "object", obj.GetName())
log.V(1).Info("Updating object", "object", obj.GetName(), "kind", obj.GetObjectKind())

return r.client.Update(ctx, obj)
}
Expand Down
46 changes: 46 additions & 0 deletions pkg/controller/pxc/pitr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package pxc

import (
"context"

"github.com/pkg/errors"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
logf "sigs.k8s.io/controller-runtime/pkg/log"

api "github.com/percona/percona-xtradb-cluster-operator/pkg/apis/pxc/v1"
"github.com/percona/percona-xtradb-cluster-operator/pkg/k8s"
"github.com/percona/percona-xtradb-cluster-operator/pkg/pxc/app/deployment"
)

func (r *ReconcilePerconaXtraDBCluster) reconcileBinlogCollector(ctx context.Context, cr *api.PerconaXtraDBCluster) error {
log := logf.FromContext(ctx)

initImage, err := k8s.GetInitImage(ctx, cr, r.client)
if err != nil {
return errors.Wrap(err, "failed to get init image")
}

binlogCollector, err := deployment.GetBinlogCollectorDeployment(cr, initImage)
if err != nil {
return errors.Wrapf(err, "get binlog collector deployment for cluster '%s'", cr.Name)
}

err = setControllerReference(cr, &binlogCollector, r.scheme)
if err != nil {
return errors.Wrapf(err, "set controller reference for binlog collector deployment '%s'", binlogCollector.Name)
}

res, err := controllerutil.CreateOrUpdate(ctx, r.client, &binlogCollector, func() error { return nil })
if err != nil {
return errors.Wrap(err, "create or update binlog collector")
}

switch res {
case controllerutil.OperationResultCreated:
log.Info("Created binlog collector", "name", binlogCollector.Name)
case controllerutil.OperationResultUpdated:
log.Info("Updated binlog collector", "name", binlogCollector.Name)
}
Comment on lines +38 to +43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we log res regardless of its status and we are switching only for these 2 cases? The status has an underlying type of string, which already has the information we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can do something like this, but i prefer the current version tbh

Suggested change
switch res {
case controllerutil.OperationResultCreated:
log.Info("Created binlog collector", "name", binlogCollector.Name)
case controllerutil.OperationResultUpdated:
log.Info("Updated binlog collector", "name", binlogCollector.Name)
}
if res != controllerutil.OperationResultNone {
log.Info("Reconciled binlog collector", "name", binlogCollector.Name, "operation", res)
}


return nil
}
Loading