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

Automated Local Disk Setup #1107

Merged
merged 4 commits into from May 12, 2023
Merged

Automated Local Disk Setup #1107

merged 4 commits into from May 12, 2023

Conversation

zimnx
Copy link
Collaborator

@zimnx zimnx commented Nov 29, 2022

The main painpoint of ScyllaDB on Kubernetes was configuring the
disks. Each cloud provider sets them up in various location, under
different names. Different instance types comes with different number of disks
and different sizes.
To improve user experience we introduce automatic local disk setup.

v1alpha1.NodeConfig API was extended with additional setting related to
local disks setup.
Scylla Operator will manage a DaemonSet responsible for creating an
RAID0 array out of discovered disks, formatting it to desired filesystem and
mounting in provided location together with provided mount options.

This feature is considered experimental, and is subject for deprecation
and incompatible changes. Using it, may require manual steps during
upgrades.

Fixes #1096

Requirements:

@zimnx zimnx force-pushed the mz/local-disk-setup branch 17 times, most recently from 13a6a08 to d4fa947 Compare December 6, 2022 08:57
@zimnx zimnx force-pushed the mz/local-disk-setup branch 12 times, most recently from 2e1243e to 819dffb Compare January 12, 2023 22:49
@zimnx zimnx marked this pull request as ready for review March 27, 2023 21:05
@zimnx zimnx marked this pull request as draft April 12, 2023 12:32
@zimnx zimnx force-pushed the mz/local-disk-setup branch 6 times, most recently from a168d56 to 7054809 Compare April 24, 2023 12:27
@zimnx zimnx marked this pull request as ready for review April 25, 2023 11:58
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.

part 1

pkg/util/algorithms/math_test.go Outdated Show resolved Hide resolved
}

// Signed is a constraint that any signed integer type satisfies.
type Signed interface {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@zimnx zimnx Apr 27, 2023

Choose a reason for hiding this comment

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

https://pkg.go.dev/golang.org/x/exp#section-readme

Warning: Packages here are experimental and unreliable. Some may one day be promoted to the main repository or other subrepository, or they may be modified arbitrarily or even disappear altogether.

Copy link
Member

Choose a reason for hiding this comment

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

I though it might not have been there when you started.

What's the worst case to happen when they remove it? I assume we'd "fork"/copy that file and maintain it on our own. But if they keep it, we don't have to review, maintain and update our copy, so to me it seems like we can only gain or end up in the same place. They will likely just move it so we only endup adjusting the import path at some point. And we have version pinning and vendoring.

btw. we already use a lot of deps in v0.x.x that have the same (non)commitment.

examples/common/nodeconfig-alpha.yaml Outdated Show resolved Hide resolved
pkg/api/scylla/v1alpha1/types_nodeconfig.go Outdated Show resolved Hide resolved
pkg/api/scylla/v1alpha1/types_nodeconfig.go Outdated Show resolved Hide resolved
pkg/controller/nodeconfig/resource.go Show resolved Hide resolved
pkg/controller/nodesetup/status.go Outdated Show resolved Hide resolved
pkg/controller/nodesetup/sync_filesystems.go Outdated Show resolved Hide resolved
pkg/disks/raid.go Outdated Show resolved Hide resolved
pkg/disks/fs.go Outdated Show resolved Hide resolved
@zimnx zimnx force-pushed the mz/local-disk-setup branch 3 times, most recently from bb979b5 to 827c5c0 Compare April 28, 2023 12:09
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.

There is a lot of good work in this PR! And a lot of pain felt when trying to figure out all the compatibility with different mdadm versions / setups.

Modulo the regex matching I am ready to land this when the CI gets green.

Consistency wise, please make sure this returns 0

git diff HEAD..master -- ./cmd/ ./pkg/ | grep -i -E -e 'if .*;.*{' | wc -l

pkg/cmd/operator/nodesetupdaemon.go Show resolved Hide resolved
pkg/controller/nodesetup/sync_raids.go Outdated Show resolved Hide resolved
pkg/controller/nodesetup/sync_raids.go Outdated Show resolved Hide resolved
test/e2e/utils/image/manifests.go Outdated Show resolved Hide resolved
@zimnx
Copy link
Collaborator Author

zimnx commented May 8, 2023

Consistency wise, please make sure this returns 0

git diff HEAD..master -- ./cmd/ ./pkg/ | grep -i -E -e 'if .*;.*{' | wc -l

These aren't inconsistent

$ grep -rni --exclude-dir=vendor -E -e 'if .*;.*{' | wc -l
3692

nor I don't get why this syntax suddenly started to be pointed out without any argument or discussion behind.
Although, I changed all added ones to suggested syntax - we are circling around pointless changes for too long, lets start moving forward.

@tnozicka
Copy link
Member

nor I don't get why this syntax suddenly started to be pointed out without any argument or discussion behind.

suddenly is a bit strong word - I recall the 2 of us having a discussion about it in the past and the readability and length line was pointed out on several PRs before. Given the magnitude of this PR I felt the grep command is easier then individual comments. If you disagree, I imagine there are more polite ways to raise it, same way as the other point you were trying to make.

Although, I changed all added ones to suggested syntax - we are circling around pointless changes for too long, lets start moving forward.

This is not something that's holding this PR.

@tnozicka
Copy link
Member

tnozicka commented May 10, 2023

thanks for the updates and the enhanced regexp logic. the only major one open I see is to use the fixed image in #1107 (comment)

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.

/approve
/lgtm

zimnx added 4 commits May 12, 2023 14:20
The main painpoint of ScyllaDB on Kubernetes was configuring the
disks. Each cloud provider sets them up in various location, under
different names. Different instance types comes with different number of
disks and different sizes.
To improve user experience we introduce automatic local disk setup.

v1alpha1.NodeConfig API was extended with additional setting related to
local disks setup.
Scylla Operator will manage a DaemonSet responsible for creating an
RAID arrays out of discovered disks, formatting it to desired filesystem
and creating a persistently mounted mount in provided location using
provided mount options.

This feature is considered experimental, and is subject for deprecation
and incompatible changes. Using it, may require manual steps during
upgrades.

Co-authored-by: Tomáš Nožička <tomas.nozicka@scylladb.com>
- Persistent mounts
@zimnx zimnx merged commit 34369da into scylladb:master May 12, 2023
17 checks passed
@zimnx zimnx deleted the mz/local-disk-setup branch May 12, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manage setup of local NVMes on Kubernetes nodes
3 participants