Added support for feature flags in opensearch.yml by ntantri · Pull Request #4959 · opensearch-project/OpenSearch

ntantri · 2022-10-27T20:54:23Z

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.

github-actions · 2022-10-27T21:07:49Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5813/
CommitID: abc27ac
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-10-27T21:09:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5815/
CommitID: e97ceb1
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-10-27T21:09:27Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5814/
CommitID: c958dbd
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-10-27T21:11:58Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5816/
CommitID: c22e311
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-10-27T21:20:12Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5812/
CommitID: abc27ac
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-10-27T21:39:17Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5817/
CommitID: e8cb508
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-11-03T00:43:52Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/6272/
CommitID: 9543074
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

mch2 · 2022-11-23T17:47:29Z

@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 · 2022-11-24T00:14:00Z

@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.

github-actions · 2022-11-25T01:23:40Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/7332/
CommitID: d8247ca

codecov-commenter · 2022-11-25T01:24:27Z

Codecov Report

❌ Patch coverage is 63.63636% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.06%. Comparing base (28e9b11) to head (9aa0a93).
⚠️ Report is 4339 commits behind head on main.

Files with missing lines	Patch %	Lines
...a/org/opensearch/test/OpenSearchIntegTestCase.java	0.00%	5 Missing ⚠️
...pensearch/common/settings/FeatureFlagSettings.java	50.00%	2 Missing ⚠️
.../java/org/opensearch/common/util/FeatureFlags.java	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4959      +/-   ##
============================================
+ Coverage     70.98%   71.06%   +0.07%     
- Complexity    58669    58762      +93     
============================================
  Files          4763     4764       +1     
  Lines        279945   279966      +21     
  Branches      40418    40421       +3     
============================================
+ Hits         198731   198966     +235     
+ Misses        64988    64856     -132     
+ Partials      16226    16144      -82

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ntantri · 2022-11-25T05:19:50Z

@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 · 2022-11-28T17:55:32Z

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 · 2022-11-29T21:52:46Z

@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

github-actions · 2022-12-08T10:48:54Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7782/
CommitID: 32a2a31
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-08T11:19:19Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7783/
CommitID: 5ee23a4
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

ntantri · 2023-01-04T15:43:05Z

Random tests keep failing 🤔
Like:

1 org.opensearch.snapshots.SegmentReplicationSnapshotIT.testSnapshotOnSegRep_RestoreOnSegRepDuringIngestion
1 org.opensearch.indices.replication.SegmentReplicationIT.testReplicationAfterPrimaryRefreshAndFlush

Should those be marked as flaky and raised a separate ticket to fix?

mch2 · 2023-01-04T16:11:58Z

Random tests keep failing 🤔

Like:
1 org.opensearch.snapshots.SegmentReplicationSnapshotIT.testSnapshotOnSegRep_RestoreOnSegRepDuringIngestion

1 org.opensearch.indices.replication.SegmentReplicationIT.testReplicationAfterPrimaryRefreshAndFlush
Should those be marked as flaky and raised a separate ticket to fix?

Yes let's mute and look at them separately as they are segrep specific. I don't want to block this pr on them.

opensearch-trigger-bot · 2023-01-04T17:10:21Z

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 · 2023-01-04T17:12:32Z

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

mch2 · 2023-01-04T18:11:38Z

@tan31989 Thank you for this change!

…#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 <nagarajtantri@gmail.com> (cherry picked from commit 241bd42)

ntantri · 2023-01-05T07:56:24Z

Added a backport PR here #5707

…#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 <nagarajtantri@gmail.com> (cherry picked from commit 241bd42)

ntantri requested review from a team and reta as code owners October 27, 2022 20:54

ntantri closed this Oct 27, 2022

ntantri reopened this Oct 27, 2022

ntantri marked this pull request as draft October 27, 2022 20:56

ntantri force-pushed the support-feature-flags-via-yaml branch 3 times, most recently from e97ceb1 to c22e311 Compare October 27, 2022 21:06

ntantri force-pushed the support-feature-flags-via-yaml branch from c22e311 to e8cb508 Compare October 27, 2022 21:08

ntantri force-pushed the support-feature-flags-via-yaml branch from e8cb508 to 9543074 Compare November 3, 2022 00:17

ntantri force-pushed the support-feature-flags-via-yaml branch from 9543074 to d8247ca Compare November 25, 2022 00:55

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

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

ntantri force-pushed the support-feature-flags-via-yaml branch from 5ee23a4 to 5cc38c5 Compare December 9, 2022 05:50

mch2 approved these changes Jan 4, 2023

View reviewed changes

dreamer-89 merged commit 241bd42 into opensearch-project:main Jan 4, 2023

dreamer-89 added the backport 2.x Backport to 2.x branch label Jan 4, 2023

mch2 mentioned this pull request Jan 4, 2023

Muting flaky SegmentReplication ITs. #5700

Merged

6 tasks

dreamer-89 mentioned this pull request Jan 4, 2023

Feature flags should be defined in opensearch.yml #4102

Closed

kotwanikunal mentioned this pull request Jan 10, 2023

[Test Failure]RecoveryIT IndexStatsIT Tests Failure #5766

Closed

dreamer-89 mentioned this pull request Mar 23, 2023

[Segment Replication] Remove deprecated feature flag check preventing SegmentReplicationSnapshotIT tests run #6810

Merged

6 tasks

Rishikesh1159 mentioned this pull request Mar 23, 2023

Opensearch: [Segment Replication] Add new Test manifest to test all plugin compatibility with Segment Replication enabled. opensearch-project/opensearch-build#3345

Closed

dbwiddis mentioned this pull request Aug 2, 2023

[BUG] Instructions to ./gradlew run -Dopensearch.experimental.feature.extensions.enabled=true don't enable extensions opensearch-project/opensearch-sdk-java#916

Closed

msfroh mentioned this pull request Oct 29, 2024

[BUG] FeatureFlags.isEnabled is kinda slow #16519

Closed

Conversation

ntantri commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

github-actions Bot commented Oct 27, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions Bot commented Oct 27, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions Bot commented Oct 27, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions Bot commented Oct 27, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions Bot commented Oct 27, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions Bot commented Oct 27, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions Bot commented Nov 3, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

mch2 commented Nov 23, 2022

Uh oh!

ntantri commented Nov 24, 2022

Uh oh!

github-actions Bot commented Nov 25, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

codecov-commenter commented Nov 25, 2022 • edited by codecov Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ntantri commented Nov 25, 2022

Uh oh!

dblock commented Nov 28, 2022

Uh oh!

mch2 commented Nov 29, 2022

Uh oh!

github-actions Bot commented Dec 8, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions Bot commented Dec 8, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

ntantri commented Jan 4, 2023

Uh oh!

mch2 commented Jan 4, 2023

Uh oh!

opensearch-trigger-bot Bot commented Jan 4, 2023

Uh oh!

dreamer-89 commented Jan 4, 2023

Uh oh!

mch2 commented Jan 4, 2023

Uh oh!

ntantri commented Jan 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ntantri commented Oct 27, 2022 •

edited

Loading

codecov-commenter commented Nov 25, 2022 •

edited by codecov Bot

Loading