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

Delayed cronjob re-enabling after cluster upgrade/resume #593

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

plaharanne
Copy link
Contributor

@plaharanne plaharanne commented Feb 8, 2024

Summary of changes

The AfterClusterUpdateSubHandler is now split in two. The first one is as it used to be and the second one is to re-enable the cronjob.
This new handler is not added to the depends_on list because it can be delayed by 1h in some cases. It means that the success notification will be sent and the handler will be executed later (if an issue occur, the error decorator will do the trick).
The delay is applied if delay_cronjob=True is in the status. It is set to True in the upgrade handler.

Checklist

  • Link to issue this PR refers to: https://github.com/crate/cloud/issues/1621
  • Relevant changes are reflected in CHANGES.rst
  • Added or changed code is covered by tests
  • Documentation has been updated if necessary
  • Changed code does not contain any breaking changes (or this is a major version change)

@plaharanne plaharanne force-pushed the p/cluster-update-timeout branch 2 times, most recently from 116a7b6 to be39cd1 Compare February 8, 2024 16:30
Base automatically changed from p/cluster-update-timeout to master February 8, 2024 16:32
@plaharanne plaharanne force-pushed the p/delay-cronjob branch 2 times, most recently from 9581fa4 to 18fa540 Compare February 8, 2024 17:10
@plaharanne plaharanne marked this pull request as ready for review February 9, 2024 10:16
@plaharanne plaharanne changed the title Delayed cronjob re-enabling after cluster upgrade Delayed cronjob re-enabling after cluster upgrade/resume Feb 9, 2024
Copy link
Contributor

@juanpardo juanpardo left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

Aren't any tests affected by not immediately re-enabling the snapshot job?

crate/operator/operations.py Outdated Show resolved Hide resolved
@plaharanne plaharanne force-pushed the p/delay-cronjob branch 8 times, most recently from 45ca945 to f63f530 Compare February 15, 2024 08:51
crate/operator/config.py Outdated Show resolved Hide resolved
@@ -344,3 +347,36 @@ async def resume_sql_exporter_configmap(
"""
await raise_on_namespace_terminating(namespace)
await update_sql_exporter_configmap(namespace, name, logger)


@kopf.timer(
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool solution 👍

@plaharanne plaharanne force-pushed the p/delay-cronjob branch 5 times, most recently from 26037cb to bd1496b Compare February 15, 2024 14:24
Copy link
Contributor

@tomach tomach left a comment

Choose a reason for hiding this comment

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

LGTM! 💯

@plaharanne plaharanne merged commit 9aa6026 into master Feb 19, 2024
5 checks passed
@plaharanne plaharanne deleted the p/delay-cronjob branch February 19, 2024 08:13
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.

None yet

4 participants