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.

Sign up for GitHub

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

Jump to bottom

Stop depending on default StorageClass' presence for ScyllaClusters in our manifests, examples, scripts and tests #2009

Merged
merged 4 commits into from
Jul 12, 2024
Merged

Stop depending on default StorageClass' presence for ScyllaClusters in our manifests, examples, scripts and tests #2009

merged 4 commits into from
Jul 12, 2024

Conversation

rzetelskik
Copy link
Member

@rzetelskik rzetelskik commented Jul 8, 2024 •
edited
Loading

Description of your changes: This PR addresses the issue of ScyllaClusters in our manifests, examples, scripts and tests often depending on the presence of a default StorageClass. This is problematic because there can only be one, indicated with an annotation, so setting/unsetting a default tends to break immutability.

Simultaneously I've tried not to break existing workflows and not have to change all of our CI jobs by expecting a StorageClass to be set explicitly. Therefore:

  • test options are extended with an optional scyllacluster-storageclass-name flag, which defaults to empty string, which is then interpreted as falling back to default storageclass (not setting storageClassName),
  • all ScyllaClusters in our manifests and examples have storageClassName set to scylladb-local-xfs by default
  • e2e/deploy scripts take SO_SCYLLACLUSTER_STORAGECLASS_NAME which propagates to Scylla Manager's and tests' clusters; if SO_SCYLLACLUSTER_STORAGECLASS_NAME is explicitly set and empty, storageClassName is not set, falling back to default storageclass

Which issue is resolved by this Pull Request:
Resolves #1990

/kind feature
/priority important-soon

/cc

@scylla-operator-bot scylla-operator-bot
Copy link
Contributor

@rzetelskik: GitHub didn't allow me to request PR reviews from the following users: rzetelskik.

Note that only scylladb members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Description of your changes: WIP

Which issue is resolved by this Pull Request:
Resolves #1990

/cc

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@scylla-operator-bot scylla-operator-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 8, 2024
@rzetelskik
Copy link
Member Author

/test e2e-gke-multi-datacenter-parallel

@scylla-operator-bot scylla-operator-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 8, 2024
@rzetelskik rzetelskik force-pushed the explicit-storageclass branch 2 times, most recently from 6cdeb78 to 48effab Compare July 10, 2024 09:02
@rzetelskik rzetelskik changed the title [WIP] Set explicit storageClassName in our manifests, examples and fixtures Stop using default StorageClass for ScyllaClusters in our manifests, examples, scripts and tests Jul 10, 2024
@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2024
@rzetelskik rzetelskik changed the title Stop using default StorageClass for ScyllaClusters in our manifests, examples, scripts and tests Stop depending on default StorageClass' presence for ScyllaClusters in our manifests, examples, scripts and tests Jul 10, 2024
@rzetelskik rzetelskik added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Jul 10, 2024
@scylla-operator-bot scylla-operator-bot bot removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jul 10, 2024
@rzetelskik
Copy link
Member Author

This is going to require explicitly setting SO_SCYLLACLUSTER_STORAGECLASS_NAME in e2e-gke-serial presubmit to pass. Before I send a PR for the prow job, let's verify the approach.

/cc zimnx tnozicka

tnozicka
tnozicka reviewed Jul 10, 2024
View reviewed changes
pkg/cmd/tests/options.go Outdated Show resolved Hide resolved
hack/.ci/run-e2e-gke-release.sh Outdated Show resolved Hide resolved
hack/.ci/run-e2e-gke.sh Outdated Show resolved Hide resolved
@rzetelskik rzetelskik changed the title Stop depending on default StorageClass' presence for ScyllaClusters in our manifests, examples, scripts and tests [WIP] Stop depending on default StorageClass' presence for ScyllaClusters in our manifests, examples, scripts and tests Jul 10, 2024
@scylla-operator-bot scylla-operator-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2024
@rzetelskik rzetelskik changed the title [WIP] Stop depending on default StorageClass' presence for ScyllaClusters in our manifests, examples, scripts and tests Stop depending on default StorageClass' presence for ScyllaClusters in our manifests, examples, scripts and tests Jul 10, 2024
@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2024
@rzetelskik rzetelskik requested a review from tnozicka July 10, 2024 14:20
zimnx
zimnx approved these changes Jul 10, 2024
View reviewed changes
Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

lgtm
/assign tnozicka

@scylla-operator-bot scylla-operator-bot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2024
tnozicka
tnozicka reviewed Jul 11, 2024
View reviewed changes
hack/.ci/lib/e2e.sh Show resolved Hide resolved
hack/ci-deploy.sh Outdated Show resolved Hide resolved
hack/ci-deploy.sh Outdated Show resolved Hide resolved
@tnozicka
Copy link
Member

/approve
/lgtm

@scylla-operator-bot scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2024
@scylla-operator-bot scylla-operator-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rzetelskik, tnozicka, zimnx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@scylla-operator-bot scylla-operator-bot bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2024
@rzetelskik
Copy link
Member Author

/hold for https://github.com/scylladb/scylla-operator-release/pull/274

@scylla-operator-bot scylla-operator-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2024
@tnozicka
Copy link
Member

/lgtm
thanks

/hold cancel
https://github.com/scylladb/scylla-operator-release/pull/274 has landed

/retest

@scylla-operator-bot scylla-operator-bot bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 12, 2024
@scylla-operator-bot scylla-operator-bot bot merged commit 4a1f717 into scylladb:master Jul 12, 2024
14 checks passed
@rzetelskik rzetelskik deleted the explicit-storageclass branch July 12, 2024 13:05
@rzetelskik rzetelskik mentioned this pull request Jul 12, 2024
Closed
@tnozicka tnozicka mentioned this pull request Jul 17, 2024
Closed
1 task
@rzetelskik rzetelskik mentioned this pull request Jul 31, 2024
Merged
@rzetelskik rzetelskik mentioned this pull request Aug 16, 2024
Open
@tnozicka tnozicka added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnozicka tnozicka tnozicka left review comments

@zimnx zimnx zimnx approved these changes

Assignees

@tnozicka tnozicka

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use explicit storageclass in all of our manifests
3 participants
@rzetelskik @tnozicka @zimnx