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.

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

EVEREST-1689 | Use new PSMDB finalizers #603

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

mayankshah1607
Copy link
Member

@mayankshah1607 mayankshah1607 commented Dec 12, 2024

CHANGE DESCRIPTION

Problem:
EVEREST-1689

In PSMDB operator 1.17.0, the finalizer names were changed to to contain fully qualified domain names to comply with kubernetes standards. This PR updates the finalizers to use the new names

CHECKLIST

Helm chart
- [ ] Is the helm chart updated with the new changes? (if applicable)

Jira

  • Is the Jira ticket created and referenced properly?

Tests

  • Is an Integration test/test case added for the new feature/change?
  • Are unit tests added where appropriate?

Signed-off-by: Mayank Shah <[email protected]>
@@ -74,7 +74,7 @@ const (
pxcGapsReasonString = "BinlogGapDetected"

deletePXCBackupFinalizer = "delete-s3-backup"
deletePSMDBBackupFinalizer = "delete-backup"
deletePSMDBBackupFinalizer = "percona.com/delete-backup"
Copy link

Choose a reason for hiding this comment

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

Could this change cause any issues during upgrades?
i.e. if an existing cluster does not have the correct finalizer yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

i.e. if an existing cluster does not have the correct finalizer yet?

I think that's fine, PSMDB operator will recognize the old finalizer as well, until 1.20.0. So both types of finalizers should work...

ref: https://github.com/percona/percona-server-mongodb-operator/releases/tag/v1.17.0

Copy link

Choose a reason for hiding this comment

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

would we need to take any action with 1.20.0? If yes, shall there be a ticket for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so.. Once users upgrade to Everest 1.5.0, they will move to the new finalizers. So no action is required by the time we support PSMDBO 1.20. Am I missing something?

Copy link

Choose a reason for hiding this comment

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

I'm not sure. Wouldn't the old DB be stuck with the old finalizer which is not managed by anyone?
Maybe a test would be good for this.

Another topic - could this change trigger an unwanted restart of a DB?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't the old DB be stuck with the old finalizer which is not managed by anyone?

Right, yeah. I assumed that the PSMDB operator would remove the old one, but that's not the case. I just updated the code to remove the old finalizers and replace them with the new ones.

Maybe a test would be good for this.

I'm not sure how you would write a test for this. To have a reliable test, you would need to provision a DB with an older (v1.4.0) everest-operator, then upgrade the everest-operator to the current one, and then check that old finalizers don't exist.. our tests run only 1 version of the operator, and that's the current one. Do you have any ideas to achieve this easily?

Another topic - could this change trigger an unwanted restart of a DB?

It won't, since the finalizers are applied on the PDMSB CR, and do not affect the StatefulSet pods. (tested it anyway to be extra sure)

Copy link

Choose a reason for hiding this comment

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

re the tests - in the worst case we could test at least manually if it works ok.
But maybe we could create the DB, add finalizers directly in the PSMDB CR and trigger a reconciliation to check if they're removed properly? Not sure, haven't tried this approach yet.

Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant