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

Proposal: extract triggers to separate CRDs #1559

Open
bo0tzz opened this issue Feb 18, 2025 · 3 comments
Open

Proposal: extract triggers to separate CRDs #1559

bo0tzz opened this issue Feb 18, 2025 · 3 comments
Labels
enhancement New feature or request

Comments

@bo0tzz
Copy link
Contributor

bo0tzz commented Feb 18, 2025

Describe the feature you'd like to have.
I propose to remove/deprecate the trigger field from the existing CRDs, and instead create separate CRDs to specify triggers. For example, there could be a ScheduledTrigger resource with a cron spec, which creates a Trigger resource that in turn instructs volsync to run the appropriate Job. 1

What is the value to the end user? (why is it a priority?)
Triggering the mover would be more explicit and interoperable. It would be clearer when a run is expected (and if that run is failing to start), it would be easier to request run from separate namespaces or other kubernetes tooling, and so forth. In addition, the Trigger resource's status field could provide more durable detail about the results of a run, and further integration could be added to for example link a particular Trigger resource to a specific restic snapshot.

Prior art
This proposal is mainly inspired by the cloudnative-pg operator, which handles its backup triggers in this way: A ScheduledBackup creates a Backup which prompts the operator to start the appropriate task. Backup objects can also be created manually or from other sources. A usage example is here, where a Backup is created from YAML in order to snapshot a particular moment.

A somewhat similar shape can also be seen in the core workload APIs, with the CronJob -> Job relation.

Related issues
I think this change could directly solve issues #627 and #1259.
I also think it could play a part in solutions for #601, #702, #1235, and #1421.

Implementation
From a (brief) look at the code, I understand that the execution of a job is driven from the .status.nextSyncTime field. Setting this field from a new Trigger controller should mean that no changes to the existing controllers are necessary for this proposed change to work. The implementation of the new controllers should be trivial.

I don't entirely know what creating a new resource entails, but with some guidance I would be willing to create a PR for this.

Footnotes

  1. There might be a better naming scheme than this.

@bo0tzz bo0tzz added the enhancement New feature or request label Feb 18, 2025
@tesshuflower
Copy link
Contributor

I'll have to discuss with John next week, this would be a pretty big change as it affects the api and usage. Changes like this would probably mean a version change in our CRD, and I would be concerned about migrating users from the old to the new.

That being said, it does sound like it has some advantages, I think we'd have to weigh this against the added complexity and also see what this means across all the movers.

@bo0tzz
Copy link
Contributor Author

bo0tzz commented Feb 19, 2025

Changes like this would probably mean a version change in our CRD, and I would be concerned about migrating users from the old to the new.

I fully understand the concern. I think this change can coexist with the existing trigger field (with the latter being deprecated but not removed) so that it would not (immediately) be a breaking change. Users could maybe create some strange corners if they tried to use both at once though.

@tesshuflower
Copy link
Contributor

It doesn't look like we're going to do this at this point. Too much change when it comes to migrating current users off the current behaviour. We could re-evaluate at some point in the future, but I think for the moment we want to keep the current scheduling mechanism.

We may however look at enhancing the status - particularly for when a job is continually failing as it will write the failure, but then end up resetting the status when it retries. Would adding another field in the status such as lastSuccessfulSyncTime be helpful?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: No status
Development

No branches or pull requests

2 participants