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

Document UpgradeSuspensionWindow #336

Merged
merged 2 commits into from
Jun 19, 2024
Merged

Conversation

bastjan
Copy link
Contributor

@bastjan bastjan commented Jun 19, 2024

@bastjan bastjan requested a review from a team June 19, 2024 12:09

Matching `UpgradeJob` objects won't start the upgrade during the time window.
Skipped jobs will be marked as successful with reason skipped.
Success and finish hooks will be executed as normal.
Copy link
Member

Choose a reason for hiding this comment

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

How is it handled currently with start hooks for a noop upgradejob? I assume start and success/finish hooks are all executed, right?

Do we need to ensure that we don't have success/finish hooks that expect certain actions to be executed in a corresponding start hook? I'm thinking about a certain customer's scale-down then scale-up requirements.

Copy link
Contributor Author

@bastjan bastjan Jun 19, 2024

Choose a reason for hiding this comment

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

Start, Success, and Finish are all executed for noop jobs.
Skipped jobs would only execute Success and Finish.

The hook would see the reason for Success and Finish and we should extend the script there.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, just something that we need to be aware of before we roll this out. It introduces a new corner case where only some of the hooks are executed, thereby potentially violating some expectations on the behavior that we had previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I would not expect too many of those weird jobs though. Since they should™️ be idempotent anyways.

Co-authored-by: Adrian Haas <[email protected]>
@bastjan bastjan merged commit 4af1ffa into master Jun 19, 2024
1 check passed
@bastjan bastjan deleted the doc-UpgradeSuspensionWindow branch June 19, 2024 13:18
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.

2 participants