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

Expose external seeds in ScyllaCluster #1321

Merged
merged 5 commits into from Sep 6, 2023
Merged

Expose external seeds in ScyllaCluster #1321

merged 5 commits into from Sep 6, 2023

Conversation

rzetelskik
Copy link
Member

@rzetelskik rzetelskik commented Aug 10, 2023 •
edited

Description of your changes:
Currently, ScyllaDB Operator does not provide any support, neither automated, nor manual, for controlling external seeds propagated to ScyllaDB cluster nodes.
Controlling the provision of external seeds is a prerequisite for supporting both manual and automated setup of multi datacenter ScyllaDB clusters, a feature long requested by many of its users, and as such is a vital step on our roadmap.

This PR adds an option to expose external seeds in ScyllaClusters and extends the operator with necessary changes. It also adjusts our E2E framework and datainserter to support multi datacenter deployments and adds an E2E covering a multidatacenter deployment using external seeds.

The implementation is aligned with the proposal #1306.

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

Prerequisites:

@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 Aug 10, 2023
@rzetelskik rzetelskik added this to the v1.10 milestone Aug 10, 2023
@scylla-operator-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@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. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 10, 2023
@rzetelskik rzetelskik force-pushed the external-seeds branch 3 times, most recently from 499f34c to 047d7a6 Compare August 10, 2023 20:17
@rzetelskik rzetelskik changed the title [WIP] Expose external seeds in ScyllaCluster Expose external seeds in ScyllaCluster Aug 11, 2023
@rzetelskik rzetelskik marked this pull request as ready for review August 11, 2023 07:36
@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 Aug 11, 2023
tnozicka
tnozicka reviewed Aug 15, 2023
View reviewed changes
Copy link
Member

@tnozicka tnozicka 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 like to avoid having the same comments twice but please ping me here when the proposal lands and this incorporates the changes to align with it.)

@rzetelskik rzetelskik marked this pull request as draft August 16, 2023 09:40
@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 Aug 16, 2023
@rzetelskik rzetelskik removed this from the v1.10 milestone Aug 17, 2023
@rzetelskik rzetelskik marked this pull request as ready for review August 18, 2023 11:01
@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 Aug 18, 2023
@rzetelskik
Copy link
Member Author

/hold
Waiting for prerequisites to merge

@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 Aug 18, 2023
@rzetelskik rzetelskik force-pushed the external-seeds branch 2 times, most recently from 32a1b79 to e5ad535 Compare August 23, 2023 14:30
@rzetelskik
Copy link
Member Author

/retest-required

1 similar comment
@rzetelskik
Copy link
Member Author

/retest-required

@rzetelskik
Copy link
Member Author

/test all

@rzetelskik
Copy link
Member Author

/hold cancel

@rzetelskik rzetelskik force-pushed the external-seeds branch 2 times, most recently from e4f0abe to 2966c56 Compare August 29, 2023 14:05
@rzetelskik rzetelskik requested a review from zimnx August 29, 2023 14:10
@rzetelskik
Copy link
Member Author

@zimnx @tnozicka thanks, I believe I replied to all of your comments. @tnozicka one comment awaiting a reply #1321 (comment)

@rzetelskik
Copy link
Member Author

/retest

tnozicka
tnozicka reviewed Sep 5, 2023
View reviewed changes
pkg/sidecar/identity/member.go Outdated Show resolved Hide resolved
test/e2e/utils/helpers.go Outdated Show resolved Hide resolved
test/e2e/utils/helpers.go Outdated Show resolved Hide resolved
test/e2e/utils/datainserter.go Show resolved Hide resolved
@scylla-operator-bot scylla-operator-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 5, 2023
@rzetelskik
Copy link
Member Author

/test images

tnozicka
tnozicka reviewed Sep 5, 2023
View reviewed changes
pkg/sidecar/identity/member.go Outdated
}

return svc.Spec.ClusterIP, nil
res := make([]string, 0, len(externalSeeds)+1)
copy(res, externalSeeds)
Copy link
Member

Choose a reason for hiding this comment

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

return values need to be processed or explicitly ignored.
(to me, using append looks more idiomatic and consistent but I don't feel strong)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@tnozicka
Copy link
Member

tnozicka commented Sep 5, 2023 •
edited

thanks for the updates

/approve
(and lgtm)

/assign @zimnx

@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 Sep 5, 2023
zimnx
zimnx approved these changes Sep 6, 2023
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

@scylla-operator-bot scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2023
@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

@tnozicka
Copy link
Member

tnozicka commented Sep 6, 2023

/retest

@scylla-operator-bot scylla-operator-bot bot merged commit 65974c4 into scylladb:master Sep 6, 2023
11 checks passed
@scylla-operator-bot
Copy link
Contributor

@rzetelskik: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gke 047d7a6 link true /test e2e-gke
ci/prow/verify d558e12 link unknown /test verify
ci/prow/e2e-gke-parallel d558e12 link unknown /test e2e-gke-parallel

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

@rzetelskik rzetelskik deleted the external-seeds branch September 6, 2023 16:01
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

@zimnx zimnx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose external seeds on ScyllaClusters
3 participants
@rzetelskik @tnozicka @zimnx