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

Optimize Registration Logic for Snapshot Compaction controller #671

Conversation

seshachalam-yv
Copy link
Contributor

@seshachalam-yv seshachalam-yv commented Aug 18, 2023

How to categorize this PR?
/area storage
/kind enhancement

What this PR does / why we need it:
This PR optimizes the logic behind registering snapshot compaction-controller. Instead of always running the compaction-controller and checking the CLI flag --enable-backup-compaction just before deploying the compaction job, the new approach registers the compaction-controller based on the presence of the CLI flag --enable-backup-compaction.

Which issue(s) this PR fixes:
Fixes #655 (comment)

Special notes for your reviewer:

Release note:

None

@seshachalam-yv seshachalam-yv requested a review from a team as a code owner August 18, 2023 06:39
@gardener-robot gardener-robot added needs/review Needs review area/storage Storage related kind/enhancement Enhancement, improvement, extension size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Aug 18, 2023
@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) 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 Aug 18, 2023
@seshachalam-yv
Copy link
Contributor Author

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

@seshachalam-yv seshachalam-yv added this to the v0.20.0 milestone Aug 21, 2023
controllers/manager.go Outdated Show resolved Hide resolved
docs/deployment/feature-gates.md Outdated Show resolved Hide resolved
@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 Aug 22, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 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 Aug 22, 2023
@unmarshall
Copy link
Contributor

unmarshall commented Aug 22, 2023

With these changes, the controller will only run if either EnableSnapshotCompaction feature gate or the CLI flag --enable-backup-compaction is enabled. A TODO comment has been added to highlight the planned removal of the CLI flag.

Taking an example of KAPI feature-gates, you can see that once the feature is GA then it is always enabled and cannot be disabled. This is how feature-gates are used in several k8s components. A CLI flag for the same has a different purpose and that is to enable or disable a feature/functionality irrespective of its state (alpha/beta/GA).

Q: What is our interpretation of feature gates? Is it in line with all upstream k8s components or is it used to disable/enable features/functionality irrespective of its state(alpha/beta/GA)? The reason i asked this question is that you have put this feature gate along with etcd-wrapper feature gate. Once etcd-wrapper feature gate is GA, then it will always be used and there will not be any option to disable it.

Also have a look at Feature-Gates-Removed where a lot of GA feature gates have been removed as for GA features, consumers cannot influence the decision to include or exclude them.

My recommendation is to have only a single interpretation of feature gates. One cannot have both interpretations. For example for etcd-events there are no snapshot leases and therefore you would always want to disable compaction controller for etcd-events. From this perspective it is not really a feature gate (if you interpret it the way upstream k8s does it and also the way it has been done for etcd-wrapper).

@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 Aug 29, 2023
@seshachalam-yv seshachalam-yv changed the title Move Enablement of Snapshot Compaction Jobs to Feature Gate EnableSnapshotCompaction Optimize Registration Logic for Snapshot Compaction Jobs Aug 29, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 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 Aug 29, 2023
@seshachalam-yv seshachalam-yv changed the title Optimize Registration Logic for Snapshot Compaction Jobs Optimize Registration Logic for Snapshot Compaction controller Aug 29, 2023
Refine the approach to registering snapshot compaction controller. Instead of always running the compaction-controller and checking the CLI flag `--enable-backup-compaction` just before deploying the compaction job, the updated logic registers the compaction-controller based on the CLI flag `--enable-backup-compaction`.
@seshachalam-yv seshachalam-yv force-pushed the feature/enable-snapshot-compaction-feature-gate branch from a603972 to 234c2ee Compare August 29, 2023 11:04
@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 Aug 29, 2023
@seshachalam-yv
Copy link
Contributor Author

/test all

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.

@seshachalam-yv thanks for making the required changes.
/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Aug 30, 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 Aug 30, 2023
Copy link
Contributor

@abdasgupta abdasgupta left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. LGTM

@seshachalam-yv seshachalam-yv merged commit 422d258 into gardener:master Sep 2, 2023
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Sep 2, 2023
@seshachalam-yv seshachalam-yv deleted the feature/enable-snapshot-compaction-feature-gate branch September 2, 2023 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/storage Storage related kind/enhancement Enhancement, improvement, extension 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/s Size of pull request is small (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.

Move enablement of snapshot compaction jobs to a feature gate EnableSnapshotCompaction
9 participants