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

Remove dual-channel-replication Feature Flag's Protection #908

Merged
merged 6 commits into from
Aug 27, 2024

Conversation

naglera
Copy link
Contributor

@naglera naglera commented Aug 14, 2024

Currently, the dual-channel-replication feature flag is immutable if enable-protected-configs is enabled, which is the default behavior. This PR proposes to make the dual-channel-replication flag mutable, allowing it to be changed dynamically without restarting the cluster.

Motivation:
The ability to change the dual-channel-replication flag dynamically is essential for testing and validating the feature on real clusters running in production environments. By making the flag mutable, we can enable or disable the feature without disrupting the cluster's operations, facilitating easier testing and experimentation. Additionally, this change would provide more flexibility for users to enable or disable the feature based on their specific requirements or operational needs without requiring a cluster restart.

Currently, the `dual-channel-replication` feature flag is immutable if
`enable-protected-configs` is enabled, which is the default behavior.
This PR proposes to make the `dual-channel-replication` flag mutable,
allowing it to be changed dynamically without restarting the cluster.

Motivation:

The ability to change the `dual-channel-replication` flag dynamically is
essential for testing and validating the feature on real clusters
running in production environments. By making the flag mutable, we can
enable or disable the feature without disrupting the cluster's
operations, facilitating easier testing and experimentation.
Additionally, this change would provide more flexibility for users
to enable or disable the feature based on their specific requirements or
operational needs without requiring a cluster restart.

Signed-off-by: naglera <anagler123@gmail.com>
@madolson
Copy link
Member

This title isn't right, it's removing that it's a protected config, which has wider implications that just making it a mutable config. Making it protected doesn't make sense, was there a reason @PingXie?

@naglera naglera changed the title Make dual-channel-replication feature flag mutable Remove Protection From dual-channel-replication Feature Flag Aug 15, 2024
@naglera naglera changed the title Remove Protection From dual-channel-replication Feature Flag Remove dual-channel-replication Feature Flag's Protection Aug 15, 2024
@PingXie
Copy link
Member

PingXie commented Aug 16, 2024

This title isn't right, it's removing that it's a protected config, which has wider implications that just making it a mutable config. Making it protected doesn't make sense, was there a reason @PingXie?

I don't remember the exact context now but I guess "protected" was introduced as a guard rail to prevent "fat-fingering" while keeping the config mutable. In a sense, this setting is kind of "semi-immutable". BTW, I have always been in favor of "immutable" but "mutable" was not a blocker to me either.

I think two more things need to be done as part of this PR:

  1. A description of the expected behavior. For example, what would happen if a user flips this config while a dual-channel full sync is in progress?
  2. a test suit that validates such a contract is upheld by the server.

@madolson
Copy link
Member

I don't remember the exact context now but I guess "protected" was introduced as a guard rail to prevent "fat-fingering" while keeping the config mutable. In a sense, this setting is kind of "semi-immutable". BTW, I have always been in favor of "immutable" but "mutable" was not a blocker to me either.

Ok, protected feels wrong to me, protected was intended for security configurations to make sure nobody can maliciously escalate their permissions. This feels like we aren't sure if something can be changed.

A description of the expected behavior. For example, what would happen if a user flips this config while a dual-channel full sync is in progress?

I don't know what Amit had in mind, but it the sync should continue with full-sync. I don't think flipping these configs should abort the ongoing synchronization.

a test suit that validates such a contract is upheld by the server.

What contract are you referring to?

turn on/off during the sync

Signed-off-by: naglera <anagler123@gmail.com>
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.44%. Comparing base (131857e) to head (cdebc55).
Report is 63 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #908      +/-   ##
============================================
+ Coverage     70.37%   70.44%   +0.07%     
============================================
  Files           112      113       +1     
  Lines         61492    61728     +236     
============================================
+ Hits          43273    43486     +213     
- Misses        18219    18242      +23     
Files with missing lines Coverage Δ
src/config.c 78.69% <ø> (ø)

... and 31 files with indirect coverage changes

Signed-off-by: Amit Nagler <58042354+naglera@users.noreply.github.com>
@madolson
Copy link
Member

madolson commented Aug 19, 2024

From core discussion, let's document that the sync finishes the replication with dual channel even when the flag is flipped part way through. (Ideally it should finish with the initial settings, but for now the scope is just dual channel).

Signed-off-by: naglera <anagler123@gmail.com>
@naglera
Copy link
Contributor Author

naglera commented Aug 21, 2024

Added tests to verify that sync continues normally in case of dual-channel config changed.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Good. I didn't review the test.

valkey.conf Outdated Show resolved Hide resolved
Signed-off-by: naglera <anagler123@gmail.com>
Copy link
Member

@PingXie PingXie left a comment

Choose a reason for hiding this comment

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

LGTM. Nits only. Thanks @naglera

tests/integration/dual-channel-replication.tcl Outdated Show resolved Hide resolved
tests/integration/dual-channel-replication.tcl Outdated Show resolved Hide resolved
tests/integration/dual-channel-replication.tcl Outdated Show resolved Hide resolved
tests/integration/dual-channel-replication.tcl Outdated Show resolved Hide resolved
Co-authored-by: Ping Xie <pingxie@outlook.com>
Signed-off-by: Amit Nagler <58042354+naglera@users.noreply.github.com>
@madolson madolson merged commit 1ff2a3b into valkey-io:unstable Aug 27, 2024
47 checks passed
madolson pushed a commit that referenced this pull request Sep 2, 2024
Currently, the `dual-channel-replication` feature flag is immutable if
`enable-protected-configs` is enabled, which is the default behavior.
This PR proposes to make the `dual-channel-replication` flag mutable,
allowing it to be changed dynamically without restarting the cluster.

**Motivation:**
The ability to change the `dual-channel-replication` flag dynamically is
essential for testing and validating the feature on real clusters
running in production environments. By making the flag mutable, we can
enable or disable the feature without disrupting the cluster's
operations, facilitating easier testing and experimentation.
Additionally, this change would provide more flexibility for users to
enable or disable the feature based on their specific requirements or
operational needs without requiring a cluster restart.

---------

Signed-off-by: naglera <anagler123@gmail.com>
madolson pushed a commit that referenced this pull request Sep 3, 2024
Currently, the `dual-channel-replication` feature flag is immutable if
`enable-protected-configs` is enabled, which is the default behavior.
This PR proposes to make the `dual-channel-replication` flag mutable,
allowing it to be changed dynamically without restarting the cluster.

**Motivation:**
The ability to change the `dual-channel-replication` flag dynamically is
essential for testing and validating the feature on real clusters
running in production environments. By making the flag mutable, we can
enable or disable the feature without disrupting the cluster's
operations, facilitating easier testing and experimentation.
Additionally, this change would provide more flexibility for users to
enable or disable the feature based on their specific requirements or
operational needs without requiring a cluster restart.

---------

Signed-off-by: naglera <anagler123@gmail.com>
@enjoy-binbin enjoy-binbin added the release-notes This issue should get a line item in the release notes label Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes This issue should get a line item in the release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants