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

Added support for feature flags in opensearch.yml #4959

Merged

Conversation

ntantri
Copy link
Contributor

@ntantri ntantri commented Oct 27, 2022

This change introduces a static store "settings"
in FeatureFlags.java file to enable the isEnabled method to fetch flag settings defined in opensearch.yml.

Description

This PR addresses issue #4102, wherein, it allows FeatureFlags to be fetched from opensearch.yml and not from the environment variable or system property.

Issues Resolved

[#4102]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@ntantri ntantri requested review from a team and reta as code owners October 27, 2022 20:54
@ntantri ntantri closed this Oct 27, 2022
@ntantri ntantri reopened this Oct 27, 2022
@ntantri ntantri marked this pull request as draft October 27, 2022 20:56
@ntantri ntantri force-pushed the support-feature-flags-via-yaml branch 3 times, most recently from e97ceb1 to c22e311 Compare October 27, 2022 21:06
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@ntantri ntantri force-pushed the support-feature-flags-via-yaml branch from c22e311 to e8cb508 Compare October 27, 2022 21:08
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@ntantri ntantri force-pushed the support-feature-flags-via-yaml branch from e8cb508 to 9543074 Compare November 3, 2022 00:17
@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

Gradle Check (Jenkins) Run Completed with:

@mch2
Copy link
Member

mch2 commented Nov 23, 2022

@tan31989 This looks like a great start! Are you looking for feedback on this draft or still iterating?

One bit of feedback - As an additional test, you can flip SegmentReplicationIT to use node settings at startup by removing the assumeFeatureFlag beforeClass method and pass the settings to startNode where internalCluster().startNode(); is invoked.

@ntantri
Copy link
Contributor Author

ntantri commented Nov 24, 2022

@mch2 thanks for the inputs. Apologies that it's taking too much time. I had some personal things to take care of.
I should be able to fix this asap and test it.

@ntantri ntantri force-pushed the support-feature-flags-via-yaml branch from 9543074 to d8247ca Compare November 25, 2022 00:55
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2022

Codecov Report

Merging #4959 (57c2125) into main (28e9b11) will increase coverage by 0.03%.
The diff coverage is 63.63%.

@@             Coverage Diff              @@
##               main    #4959      +/-   ##
============================================
+ Coverage     70.98%   71.02%   +0.03%     
- Complexity    58669    58727      +58     
============================================
  Files          4763     4764       +1     
  Lines        279945   279966      +21     
  Branches      40418    40421       +3     
============================================
+ Hits         198731   198846     +115     
+ Misses        64988    64894      -94     
  Partials      16226    16226              
Impacted Files Coverage Δ
...a/org/opensearch/test/OpenSearchIntegTestCase.java 57.79% <0.00%> (+1.12%) ⬆️
...pensearch/common/settings/FeatureFlagSettings.java 50.00% <50.00%> (ø)
.../java/org/opensearch/common/util/FeatureFlags.java 80.00% <88.88%> (+30.00%) ⬆️
...org/opensearch/common/settings/SettingsModule.java 85.81% <100.00%> (+0.29%) ⬆️
server/src/main/java/org/opensearch/node/Node.java 87.17% <100.00%> (+0.01%) ⬆️
...opensearch/persistent/PersistentTasksExecutor.java 22.22% <0.00%> (-44.45%) ⬇️
.../opensearch/client/indices/CloseIndexResponse.java 56.25% <0.00%> (-35.00%) ⬇️
...ion/admin/cluster/node/info/PluginsAndModules.java 53.12% <0.00%> (-34.38%) ⬇️
...nsearch/index/shard/IndexShardClosedException.java 66.66% <0.00%> (-33.34%) ⬇️
...opensearch/index/reindex/BulkByScrollResponse.java 48.38% <0.00%> (-28.23%) ⬇️
... and 498 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ntantri
Copy link
Contributor Author

ntantri commented Nov 25, 2022

@mch2 was debugging the tests by adding the settings to internalCluster().startNode(settings), and apparently there are validations on these settings being passed.

java.lang.IllegalArgumentException: unknown setting [opensearch.experimental.feature.extensions.enabled] 
please check that any required plugins are installed, or check the breaking changes 
documentation for removed settings
	at __randomizedtesting.SeedInfo.seed([F366529BF2371B1C:722EBD885B6BAAE8]:0)
	at org.opensearch.common.settings.AbstractScopedSettings.validate(AbstractScopedSettings.java:591)

So, if we add these settings to opensearch.yaml, do we effectively get that ingested during the testing?

@dblock
Copy link
Member

dblock commented Nov 28, 2022

I think we have more American spelling of initialize vs. British initialise. I believe in computers we typically use initialize, care to rename here (and possibly elsewhere, maybe a separate PR)?

@mch2
Copy link
Member

mch2 commented Nov 29, 2022

@mch2 was debugging the tests by adding the settings to internalCluster().startNode(settings), and apparently there are validations on these settings being passed.

java.lang.IllegalArgumentException: unknown setting [opensearch.experimental.feature.extensions.enabled] 
please check that any required plugins are installed, or check the breaking changes 
documentation for removed settings
	at __randomizedtesting.SeedInfo.seed([F366529BF2371B1C:722EBD885B6BAAE8]:0)
	at org.opensearch.common.settings.AbstractScopedSettings.validate(AbstractScopedSettings.java:591)

So, if we add these settings to opensearch.yaml, do we effectively get that ingested during the testing?

@tan31989 This means that the flag itself is not registered yet as a setting, even though it is properly getting passed in your test.

In SettingsModule.java we loop over the existing featureFlags link, and if enabled register the settings that are specific to the feature behind the flag. Before this step happens, we will need to register settings for the flags themselves.

Example for just the segrep setting:

        registerSetting(FeatureFlags.REPLICATION_TYPE_ENABLED_SETTING);

        for (Map.Entry<String, List<Setting>> featureFlaggedSetting : IndexScopedSettings.FEATURE_FLAGGED_INDEX_SETTINGS.entrySet()) {
            if (FeatureFlags.isEnabled(featureFlaggedSetting.getKey())) {
                featureFlaggedSetting.getValue().forEach(feature -> registerSetting(feature));
            }
        }

And in FeatureFlags.java

    /**
     * Gates the visibility of the index setting that allows changing of replication type.
     * Once the feature is ready for production release, this feature flag can be removed.
     */
    public static final String REPLICATION_TYPE = "opensearch.experimental.feature.replication_type.enabled";

    public static final Setting<Boolean> REPLICATION_TYPE_ENABLED_SETTING = Setting.boolSetting(
        REPLICATION_TYPE,
        false,
        Setting.Property.NodeScope,
        Setting.Property.Final
    );

After this is done for a particular flag it will register all its settings, which for segrep is the replication type setting.

With this code if you were to flip the flag off in your test, you'd see a similar error indicating the type setting is unknown, while the enabled setting still exists with a final value of false.

java.lang.IllegalArgumentException: unknown setting [index.replication.type] please check that any required plugins are installed, or check the breaking changes documentation for removed settings

@ntantri ntantri force-pushed the support-feature-flags-via-yaml branch from d8247ca to 32a2a31 Compare December 8, 2022 10:28
@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2022

Gradle Check (Jenkins) Run Completed with:

@ntantri ntantri force-pushed the support-feature-flags-via-yaml branch from 32a2a31 to 5ee23a4 Compare December 8, 2022 10:57
@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2022

Gradle Check (Jenkins) Run Completed with:

@ntantri ntantri force-pushed the support-feature-flags-via-yaml branch from 5ee23a4 to 5cc38c5 Compare December 9, 2022 05:50
@dreamer-89 dreamer-89 merged commit 241bd42 into opensearch-project:main Jan 4, 2023
@dreamer-89 dreamer-89 added the backport 2.x Backport to 2.x branch label Jan 4, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-4959-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 241bd42ab4d0bff0c6d232df0076d2568e326667
# Push it to GitHub
git push --set-upstream origin backport/backport-4959-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-4959-to-2.x.

@dreamer-89
Copy link
Member

@tan31989 : The PR to backport to 2.x failed, care to raise a manual backport PR

@mch2
Copy link
Member

mch2 commented Jan 4, 2023

@tan31989 Thank you for this change!

@mch2 mch2 mentioned this pull request Jan 4, 2023
6 tasks
ntantri added a commit to ntantri/OpenSearch that referenced this pull request Jan 5, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

(cherry picked from commit 241bd42)
ntantri added a commit to ntantri/OpenSearch that referenced this pull request Jan 5, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

(cherry picked from commit 241bd42)
ntantri added a commit to ntantri/OpenSearch that referenced this pull request Jan 5, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

(cherry picked from commit 241bd42)
ntantri added a commit to ntantri/OpenSearch that referenced this pull request Jan 5, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

(cherry picked from commit 241bd42)
@ntantri
Copy link
Contributor Author

ntantri commented Jan 5, 2023

Added a backport PR here #5707

ntantri added a commit to ntantri/OpenSearch that referenced this pull request Jan 5, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

(cherry picked from commit 241bd42)
ntantri added a commit to ntantri/OpenSearch that referenced this pull request Jan 7, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

(cherry picked from commit 241bd42)
ntantri added a commit to ntantri/OpenSearch that referenced this pull request Jan 8, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

(cherry picked from commit 241bd42)
ntantri added a commit to ntantri/OpenSearch that referenced this pull request Jan 8, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

(cherry picked from commit 241bd42)
ntantri added a commit to ntantri/OpenSearch that referenced this pull request Jan 8, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

(cherry picked from commit 241bd42)
ntantri added a commit to ntantri/OpenSearch that referenced this pull request Jan 8, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

(cherry picked from commit 241bd42)
ntantri added a commit to ntantri/OpenSearch that referenced this pull request Jan 8, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

(cherry picked from commit 241bd42)
ntantri added a commit to ntantri/OpenSearch that referenced this pull request Jan 8, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

(cherry picked from commit 241bd42)
ntantri added a commit to ntantri/OpenSearch that referenced this pull request Jan 8, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

(cherry picked from commit 241bd42)
ntantri added a commit to ntantri/OpenSearch that referenced this pull request Jan 19, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

(cherry picked from commit 241bd42)
andrross pushed a commit to andrross/OpenSearch that referenced this pull request Jan 19, 2023
…#4959)

This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

Signed-off-by: Nagaraj Tantri <[email protected]>
kotwanikunal pushed a commit that referenced this pull request Jan 19, 2023
This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

Signed-off-by: Nagaraj Tantri <[email protected]>

Signed-off-by: Nagaraj Tantri <[email protected]>
Co-authored-by: Nagaraj Tantri <[email protected]>
kotwanikunal pushed a commit that referenced this pull request Jan 25, 2023
This change introduces a static store "settings"
in FeatureFlags.java file to enable isEnabled method to
fetch flag settings defined in opensearch.yml.

Signed-off-by: Nagaraj Tantri <[email protected]>

Signed-off-by: Nagaraj Tantri <[email protected]>

Signed-off-by: Nagaraj Tantri <[email protected]>
Co-authored-by: Nagaraj Tantri <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants