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

Add support for SPKS and implement sts resizing #225

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Conversation

TheBigLee
Copy link
Member

@TheBigLee TheBigLee commented Aug 26, 2024

Summary

  • This PR adds support for the SPKS MariaDB and Redis offerings
  • It also implements the sts deletion logic for PVC resizing using composition functions

Checklist

  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • Update tests.
  • Link this PR to related issues.

@TheBigLee TheBigLee force-pushed the spks_sts_deleter branch 3 times, most recently from 82a5bcb to 845bd8d Compare August 30, 2024 13:30
@TheBigLee TheBigLee changed the title Add support for SPKS redis Add support for SPKS and implement sts resizing Aug 30, 2024
@TheBigLee TheBigLee requested review from a team, Kidswiss, wejdross and zugao and removed request for a team August 30, 2024 13:44
@zugao
Copy link
Collaborator

zugao commented Sep 2, 2024

Did we reuse the scripts or they have been just implemented in this PR? I feel like this PR is full of bombs, have you tested it throughly?

@TheBigLee
Copy link
Member Author

Did we reuse the scripts or they have been just implemented in this PR? I feel like this PR is full of bombs, have you tested it throughly?

We did reuse them from the vshnredis, but have been adjusted slightly to work with this setup.
It has been tested thoroughly. What bombs are you refering to?

@TheBigLee TheBigLee force-pushed the spks_sts_deleter branch 3 times, most recently from 5c61b48 to 3dc2630 Compare September 3, 2024 16:35
@TheBigLee TheBigLee added the enhancement New feature or request label Sep 3, 2024
Copy link
Contributor

@Kidswiss Kidswiss left a comment

Choose a reason for hiding this comment

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

I like the approach.

I've added a few comments about it.

Also, it would be great if this was generic for any release, because right now the code exists more or less identical for two spks services and still with the race condtion for the other services.

Copy link
Member

@wejdross wejdross left a comment

Choose a reason for hiding this comment

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

I'd add PrometheusRule as well to check if annotation crossplane.io/paused is set for longer than 15 minutes, so we don't end up with partially broken resources and catch early issues.

@TheBigLee TheBigLee force-pushed the spks_sts_deleter branch 2 times, most recently from a063a3b to 70f0798 Compare September 4, 2024 13:40
@TheBigLee TheBigLee added the minor label Sep 4, 2024
@TheBigLee
Copy link
Member Author

I'd add PrometheusRule as well to check if annotation crossplane.io/paused is set for longer than 15 minutes, so we don't end up with partially broken resources and catch early issues.

This is obsolete, as we use trap to ensure the value is removed

@TheBigLee TheBigLee force-pushed the spks_sts_deleter branch 2 times, most recently from bc3a043 to 147e34d Compare September 4, 2024 14:02
Copy link
Member

@wejdross wejdross left a comment

Choose a reason for hiding this comment

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

I'm ok with that, just one nitpick. LGTM

@TheBigLee TheBigLee merged commit 65a606f into master Sep 10, 2024
7 checks passed
@TheBigLee TheBigLee deleted the spks_sts_deleter branch September 10, 2024 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants