tests/979-path-conf-env-override

[swatichauhan814]

Description

Add packaging tests to verify that a pre-set OPENSEARCH_PATH_CONF environment variable is not overridden when opensearch-env is sourced

• ArchiveTests.java - test60CustomPathConfOverride: sets OPENSEARCH_PATH_CONF via shell env before starting an archive-based node; asserts the custom config directory (and its node.name) is used instead of the default $OPENSEARCH_HOME/config.

• PackageTests.java - test85CustomPathConfViaSystemdDropIn: injects OPENSEARCH_PATH_CONF via a systemd drop-in Environment= directive (the correct mechanism since sh.getEnv() does not propagate into systemd services); asserts the custom config directory is used, not /etc/opensearch.

• PackageTests.java - test86DefaultEnvFileSourcedWhenPathConfAbsent: regression guard confirming that when no external override exists, the env file is still sourced and OpenSearch starts normally with the package default config path.

Related Issues

Resolves #[https://github.com//issues/979]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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]

PR Reviewer Guide 🔍

(Review updated until commit 9f7c37c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Missing startOpenSearch/stopOpenSearch test60CustomPathConfOverride uses assertWhileRunning but never calls startOpenSearch() before it. assertWhileRunning typically expects OpenSearch to already be started (it asserts state while it's running), so the test will likely fail because no node is running when the HTTP request is made. Compare with test70CustomPathConfAndJvmOptions which uses withCustomConfig that handles lifecycle. Also, the test does not restrict itself to archive distributions (assumeThat(distribution().isArchive(), is(true))), though it's in ArchiveTests it should still confirm the test class isn't executed for packages. public void test60CustomPathConfOverride() throws Exception { // Verify that pre-setting OPENSEARCH_PATH_CONF in the shell environment causes OpenSearch to // use the custom config directory instead of the default $OPENSEARCH_HOME/config. Platforms.onLinux(() -> { Path tempDir = createTempDir("custom-path-conf-override"); Path customConf = tempDir.resolve("opensearch"); FileUtils.copyDirectory(installation.config, customConf); final String customNodeName = "custom-node-conf-override"; append(customConf.resolve("opensearch.yml"), "\nnode.name: " + customNodeName + "\n"); sh.getEnv().put("OPENSEARCH_PATH_CONF", customConf.toString()); try { assertWhileRunning(() -> { final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_cat/nodes?h=name")).trim(); assertThat(nodesResponse, equalTo(customNodeName)); }); } finally { sh.getEnv().remove("OPENSEARCH_PATH_CONF"); rm(tempDir); } }); } Test isolation risk test85CustomPathConfViaSystemdDropIn calls stopOpenSearch() at the very beginning, which assumes OpenSearch is running from a prior test. If tests are run in isolation or the prior test failed to leave the service running, this call may fail. Additionally, if the test fails between startOpenSearch() and stopOpenSearch(), the finally block removes the drop-in and reloads systemd but does not stop the running service, potentially leaving a stale OpenSearch process pointing to a deleted tempDir, which could affect subsequent tests. stopOpenSearch(); Path tempDir = createTempDir("pkg-custom-path-conf"); Path customConf = tempDir.resolve("opensearch"); FileUtils.copyDirectory(installation.config, customConf); sh.run("chown -R opensearch:opensearch " + tempDir); final String customNodeName = "pkg-custom-path-conf-node"; append(customConf.resolve("opensearch.yml"), "\nnode.name: " + customNodeName + "\n"); // Drop-in file that sets OPENSEARCH_PATH_CONF before opensearch-env is sourced. // This simulates a user who exports the variable system-wide (e.g., in /etc/environment // or a systemd override) rather than relying on the env file. Path dropInDir = Paths.get("/etc/systemd/system/opensearch.service.d"); Path dropInFile = dropInDir.resolve("test-path-conf-override.conf"); try { sh.run("mkdir -p " + dropInDir); Files.write(dropInFile, singletonList("[Service]\nEnvironment=OPENSEARCH_PATH_CONF=" + customConf)); sh.run("systemctl daemon-reload"); startOpenSearch(); final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_cat/nodes?h=name")).trim(); assertThat(nodesResponse, equalTo(customNodeName)); stopOpenSearch(); } finally { rm(dropInFile); sh.run("systemctl daemon-reload"); rm(tempDir); } Ordering dependency test86DefaultEnvFileSourcedWhenPathConfAbsent asserts the drop-in file created by test85 does not exist. This creates an implicit ordering/cleanup dependency: if test85 fails before its finally block executes fully, or if test86 runs before test85's cleanup in a different order, the assertion will fail. Consider removing the drop-in file defensively at the start of test86 rather than asserting. Path dropInFile = Paths.get("/etc/systemd/system/opensearch.service.d/test-path-conf-override.conf"); assertFalse("test drop-in must not exist before this test", Files.exists(dropInFile));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 9f7c37c

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Ensure service is stopped on test failure If startOpenSearch() or the assertion fails, stopOpenSearch() is skipped, leaving the service running with the drop-in and causing subsequent tests to fail or hang. Move stopOpenSearch() into the finally block (guarded appropriately) to ensure the service is always stopped before the drop-in is removed. qa/os/src/test/java/org/opensearch/packaging/test/PackageTests.java [398-411] try { sh.run("mkdir -p " + dropInDir); Files.write(dropInFile, singletonList("[Service]\nEnvironment=OPENSEARCH_PATH_CONF=" + customConf)); sh.run("systemctl daemon-reload"); startOpenSearch(); final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_cat/nodes?h=name")).trim(); assertThat(nodesResponse, equalTo(customNodeName)); - stopOpenSearch(); } finally { + try { stopOpenSearch(); } catch (Exception ignored) {} rm(dropInFile); sh.run("systemctl daemon-reload"); rm(tempDir); } Suggestion importance[1-10]: 7 __ Why: Valid concern: if startOpenSearch() or the assertion fails, the service remains running with the drop-in in place, potentially affecting subsequent tests. Moving stopOpenSearch() into the finally block improves test isolation and robustness.	Medium
	Fix ownership on copied config directory The custom config directory is copied from installation.config but likely retains root/original ownership, while the archive distribution typically runs OpenSearch as a non-root user. This can cause the node to fail to start due to permission errors on the config directory. Ensure the copied customConf is chowned to the OpenSearch user before starting the node. qa/os/src/test/java/org/opensearch/packaging/test/ArchiveTests.java [338-347] sh.getEnv().put("OPENSEARCH_PATH_CONF", customConf.toString()); try { + sh.run("chown -R " + ARCHIVE_OWNER + ":" + ARCHIVE_OWNER + " " + tempDir); assertWhileRunning(() -> { final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_cat/nodes?h=name")).trim(); assertThat(nodesResponse, equalTo(customNodeName)); }); } finally { sh.getEnv().remove("OPENSEARCH_PATH_CONF"); rm(tempDir); } Suggestion importance[1-10]: 5 __ Why: Reasonable concern about file ownership on the copied config directory, which could cause permission failures when OpenSearch runs as a non-root user. However, the correctness depends on the test infrastructure's user setup which is not fully visible in the diff.	Low

---

Previous suggestions

Suggestions up to commit 1c247d9

Category	Suggestion	Impact
Possible issue	Guard test against non-package distributions Calling stopOpenSearch() unconditionally may fail if OpenSearch is not currently running when the test starts. Also, this test lacks the assumeTrue("packages only", distribution.isPackage()) check that installation.envFile implicitly requires; without it the test could fail on non-package distributions. qa/os/src/test/java/org/opensearch/packaging/test/PackageTests.java [380-383] +assumeTrue("packages only", distribution.isPackage()); assumeTrue("systemd only", isSystemd()); assertPathsExist(installation.envFile); stopOpenSearch(); Suggestion importance[1-10]: 6 __ Why: Adding an explicit distribution.isPackage() assumption makes the test more robust and consistent with test86, since installation.envFile and systemd drop-ins are package-specific.	Low
	Ensure service stop on test failure If startOpenSearch(), the assertion, or stopOpenSearch() throws, the finally block runs rm(dropInFile) but OpenSearch may still be running, leaving stale state that breaks subsequent tests (e.g. test86 which asserts the drop-in doesn't exist). Ensure OpenSearch is stopped in the finally block regardless of test outcome. qa/os/src/test/java/org/opensearch/packaging/test/PackageTests.java [398-411] try { sh.run("mkdir -p " + dropInDir); Files.write(dropInFile, singletonList("[Service]\nEnvironment=OPENSEARCH_PATH_CONF=" + customConf)); sh.run("systemctl daemon-reload"); startOpenSearch(); final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_cat/nodes?h=name")).trim(); assertThat(nodesResponse, equalTo(customNodeName)); - stopOpenSearch(); } finally { + try { stopOpenSearch(); } catch (Exception ignored) {} rm(dropInFile); sh.run("systemctl daemon-reload"); rm(tempDir); } Suggestion importance[1-10]: 6 __ Why: Moving stopOpenSearch() into the finally block prevents a failing assertion from leaving OpenSearch running and interfering with subsequent tests like test86.	Low
General	Make temp directory cleanup resilient The rm(tempDir) call in the finally block may fail if OpenSearch is still running and holding file handles, or if the process ran as a different user and created files with restricted permissions. Consider ensuring OpenSearch is stopped before cleanup and handling potential cleanup errors so they don't mask the real test failure. qa/os/src/test/java/org/opensearch/packaging/test/ArchiveTests.java [338-348] sh.getEnv().put("OPENSEARCH_PATH_CONF", customConf.toString()); try { assertWhileRunning(() -> { final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_cat/nodes?h=name")).trim(); assertThat(nodesResponse, equalTo(customNodeName)); }); } finally { sh.getEnv().remove("OPENSEARCH_PATH_CONF"); - rm(tempDir); + try { + rm(tempDir); + } catch (Exception e) { + // best-effort cleanup + } } Suggestion importance[1-10]: 3 __ Why: assertWhileRunning typically stops OpenSearch before returning, so cleanup issues are unlikely. Wrapping rm in a swallowed exception could also hide real problems.	Low

Suggestions up to commit fdfeee3

Category	Suggestion	Impact
Possible issue	Ensure server is stopped on failure If startOpenSearch(), the request, or the assertion throws, stopOpenSearch() is skipped and a running server will leak into subsequent tests (also causing the drop-in cleanup to potentially fail). Move stopOpenSearch() into the finally block to guarantee shutdown regardless of assertion outcome. qa/os/src/test/java/org/opensearch/packaging/test/PackageTests.java [398-411] try { sh.run("mkdir -p " + dropInDir); Files.write(dropInFile, singletonList("[Service]\nEnvironment=OPENSEARCH_PATH_CONF=" + customConf)); sh.run("systemctl daemon-reload"); startOpenSearch(); final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_cat/nodes?h=name")).trim(); assertThat(nodesResponse, equalTo(customNodeName)); - stopOpenSearch(); } finally { + try { stopOpenSearch(); } catch (Exception ignored) {} rm(dropInFile); sh.run("systemctl daemon-reload"); rm(tempDir); } Suggestion importance[1-10]: 6 __ Why: Moving stopOpenSearch() into the finally block is a reasonable robustness improvement to prevent test state leakage if the assertion fails, though it's a moderate reliability concern rather than critical.	Low
	Start the server before asserting assertWhileRunning typically expects OpenSearch to already be running, but this test never invokes startOpenSearch(). Without starting the server, the HTTP request will fail. Wrap the assertion with an explicit startOpenSearch()/stopOpenSearch() (and ensure stop happens in the finally block) so the custom config is actually exercised. qa/os/src/test/java/org/opensearch/packaging/test/ArchiveTests.java [331-348] Platforms.onLinux(() -> { Path tempDir = createTempDir("custom-path-conf-override"); Path customConf = tempDir.resolve("opensearch"); FileUtils.copyDirectory(installation.config, customConf); final String customNodeName = "custom-node-conf-override"; append(customConf.resolve("opensearch.yml"), "\nnode.name: " + customNodeName + "\n"); sh.getEnv().put("OPENSEARCH_PATH_CONF", customConf.toString()); try { - assertWhileRunning(() -> { - final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_cat/nodes?h=name")).trim(); - assertThat(nodesResponse, equalTo(customNodeName)); - }); + startOpenSearch(); + final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_cat/nodes?h=name")).trim(); + assertThat(nodesResponse, equalTo(customNodeName)); } finally { + try { stopOpenSearch(); } catch (Exception ignored) {} sh.getEnv().remove("OPENSEARCH_PATH_CONF"); rm(tempDir); } }); Suggestion importance[1-10]: 2 __ Why: The suggestion misunderstands assertWhileRunning, which is a helper that typically starts and stops OpenSearch around the provided assertion. The existing code is likely correct, and the proposed rewrite may not be necessary.	Low

Suggestions up to commit e2d6802

Category	Suggestion	Impact
Possible issue	Fix custom config dir ownership The custom config directory is copied from installation.config but its ownership/permissions may not be readable by the opensearch user that runs the archive service, causing startup failure. Ensure the copied directory is owned by the appropriate user (as done in PackageTests with chown), otherwise the node may fail to start and the test will report a misleading error. qa/os/src/test/java/org/opensearch/packaging/test/ArchiveTests.java [338-347] sh.getEnv().put("OPENSEARCH_PATH_CONF", customConf.toString()); try { + sh.run("chown -R " + ARCHIVE_OWNER + ":" + ARCHIVE_OWNER + " " + tempDir); assertWhileRunning(() -> { final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_cat/nodes?h=name")).trim(); assertThat(nodesResponse, equalTo(customNodeName)); }); } finally { sh.getEnv().remove("OPENSEARCH_PATH_CONF"); rm(tempDir); } Suggestion importance[1-10]: 6 __ Why: Valid concern - the archive test copies config to a temp dir without adjusting ownership, which could cause permission issues when the opensearch user tries to read it. However, the archive tests typically run as the same user that started the service, so impact may be limited.	Low
General	Cleanup leftover drop-in defensively If test85 fails before reaching its finally block (e.g., JVM crash) the drop-in file may remain and cause test86 to fail with a misleading assertion. Instead of asserting absence, proactively remove any leftover drop-in and reload systemd to make the regression guard robust against prior test failures. qa/os/src/test/java/org/opensearch/packaging/test/PackageTests.java [421-423] // Ensure no leftover drop-in from previous test could inject OPENSEARCH_PATH_CONF. Path dropInFile = Paths.get("/etc/systemd/system/opensearch.service.d/test-path-conf-override.conf"); -assertFalse("test drop-in must not exist before this test", Files.exists(dropInFile)); +if (Files.exists(dropInFile)) { + rm(dropInFile); + sh.run("systemctl daemon-reload"); +} Suggestion importance[1-10]: 5 __ Why: Reasonable robustness improvement to prevent cascading test failures if a prior test aborted unexpectedly. Moderate impact on test reliability.	Low
	Write drop-in file as separate lines Files.write with singletonList writes each list element followed by a line separator, so the embedded \n between [Service] and Environment= will be written literally as an escape sequence only if the string contains \n — but here it's a real newline, which is fine. However, using singletonList with a multi-line string is fragile; prefer writing the two lines as separate list entries to ensure proper line separation across platforms. qa/os/src/test/java/org/opensearch/packaging/test/PackageTests.java [399-401] sh.run("mkdir -p " + dropInDir); -Files.write(dropInFile, singletonList("[Service]\nEnvironment=OPENSEARCH_PATH_CONF=" + customConf)); +Files.write(dropInFile, java.util.Arrays.asList("[Service]", "Environment=OPENSEARCH_PATH_CONF=" + customConf)); sh.run("systemctl daemon-reload"); Suggestion importance[1-10]: 3 __ Why: Minor style improvement for readability; the original code works correctly since \n is a real newline in Java string literals. Low impact.	Low

[github-actions]

❌ Gradle check result for e2d6802: FAILURE

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]

Persistent review updated to latest commit fdfeee3

[github-actions]

Persistent review updated to latest commit 1c247d9

[github-actions]

Persistent review updated to latest commit 9f7c37c

[github-actions]

✅ Gradle check result for 9f7c37c: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.42%. Comparing base (d603ae1) to head (9f7c37c).

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22436      +/-   ##
============================================
- Coverage     73.46%   73.42%   -0.04%     
+ Complexity    76477    76417      -60     
============================================
  Files          6104     6104              
  Lines        346568   346568              
  Branches      49883    49883              
============================================
- Hits         254598   254481     -117     
- Misses        71693    71765      +72     
- Partials      20277    20322      +45     

☔ View full report in Codecov by Harness.
📢 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.