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 local provider support for compaction #682

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

abdasgupta
Copy link
Contributor

@abdasgupta abdasgupta commented Sep 24, 2023

How to categorize this PR?

/area control-plane
/kind bug

What this PR does / why we need it:
This PR fixes the bug that was not letting the compaction job use local storage. Without this PR, compaction jobs were not running for gardener local setup with local provider.

Which issue(s) this PR fixes:
Fixes #709

Special notes for your reviewer:

Release note:

Local storage provider for backups is now supported for snapshot compaction jobs.

@abdasgupta abdasgupta requested a review from a team as a code owner September 24, 2023 11:34
@gardener-robot gardener-robot added area/control-plane Control plane related kind/bug Bug needs/review Needs review size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Sep 24, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 24, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 24, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 25, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 27, 2023
@abdasgupta
Copy link
Contributor Author

/test pull-etcd-druid-e2e-kind-alpha-features

@abdasgupta
Copy link
Contributor Author

/test pull-etcd-druid-e2e-kind

@abdasgupta abdasgupta added this to the v0.20.0 milestone Sep 29, 2023
Copy link
Contributor

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

@abdasgupta thanks for the PR. I have a few comments. Please address them. Additionally, you might also need to use an init container to change file permissions for the store provider directories, as seen here:

if r.Config.FeatureGates[features.UseEtcdWrapper] {
if targetProvider == druidutils.Local {
// init container to change file permissions of the folders used as store to 65532 (nonroot)
// used only with local provider
job.Spec.Template.Spec.InitContainers = []corev1.Container{
{
Name: "change-backup-bucket-permissions",
Image: "alpine:3.18.2",
Command: []string{"sh", "-c", "--"},
Args: []string{fmt.Sprintf("%s%s%s%s", "chown -R 65532:65532 /home/nonroot/", *targetStore.Container, " /home/nonroot/", *sourceStore.Container)},
VolumeMounts: volumeMounts,
SecurityContext: &corev1.SecurityContext{
RunAsGroup: pointer.Int64(0),
RunAsNonRoot: pointer.Bool(false),
RunAsUser: pointer.Int64(0),
},
},
}
}

You may consult with @aaronfern for help with this.

controllers/compaction/reconciler.go Outdated Show resolved Hide resolved
controllers/compaction/reconciler.go Outdated Show resolved Hide resolved
controllers/compaction/reconciler.go Outdated Show resolved Hide resolved
controllers/compaction/reconciler.go Show resolved Hide resolved
controllers/compaction/reconciler.go Show resolved Hide resolved
@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Sep 29, 2023
@shreyas-s-rao shreyas-s-rao changed the title Fix the bug that was not letting use local storage for compaction. Add local provider support for compaction Sep 29, 2023
@aaronfern
Copy link
Contributor

Compaction job would not need an init container as it used an emptyDir volume, and that's fresh for every run, so no permission change is needed

@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 3, 2023
@abdasgupta
Copy link
Contributor Author

/test pull-etcd-druid-e2e-kind

@shreyas-s-rao shreyas-s-rao modified the milestones: v0.20.0, v0.21.0 Oct 4, 2023
@abdasgupta
Copy link
Contributor Author

/test pull-etcd-druid-e2e-kind

@gardener-robot gardener-robot added size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) and removed size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Oct 13, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 13, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 13, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 13, 2023
Copy link
Contributor

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

@abdasgupta thanks for making the requested changes. Overall looks good, except for couple of nits. PTAL, thanks

test/utils/etcd.go Show resolved Hide resolved
controllers/compaction/reconciler.go Show resolved Hide resolved
controllers/compaction/reconciler.go Show resolved Hide resolved
controllers/compaction/reconciler.go Show resolved Hide resolved
controllers/compaction/reconciler.go Show resolved Hide resolved
Copy link
Contributor

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review labels Oct 16, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 16, 2023
@abdasgupta abdasgupta merged commit db1d2f0 into gardener:master Oct 16, 2023
1 check passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Oct 16, 2023
@abdasgupta abdasgupta deleted the fix-cmpct-local branch October 16, 2023 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/control-plane Control plane related kind/bug Bug needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Compaction job does not work with local storage
7 participants