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

feat: allow overwrite opensearch home #6956

Merged

Conversation

shyim
Copy link
Contributor

@shyim shyim commented Apr 3, 2023

Description

Right now it is not possible to set a fixed OPENSEARCH_HOME directory.

This is required for Nix packaging as the OpenSearch files lives inside the nix store (/nix/store/random-string-opensearch/bin/opensearch). Therefore we needed to patch this to allow us to set a custom directory.

If you need maybe more background to understand we setup a own empty Opensearch home and symlink required files: https://github.com/NixOS/nixpkgs/blob/fc46a9d1e2b30321a2497e413eb1562c9cfda5b6/nixos/modules/services/search/opensearch.nix#L185-L217

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)

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2023

Gradle Check (Jenkins) Run Completed with:

@shyim
Copy link
Contributor Author

shyim commented Apr 3, 2023

Missing SHA for lucene-core-9.6.0-snapshot-a3ae27f.jar. Run "gradle updateSHAs" to create them

I guess this is not related to this PR

@reta
Copy link
Collaborator

reta commented Apr 3, 2023

Missing SHA for lucene-core-9.6.0-snapshot-a3ae27f.jar. Run "gradle updateSHAs" to create them

I guess this is not related to this PR

Nope, #6959

@shyim shyim force-pushed the allow-overwrite-opensearch-home branch from 3c796e3 to cb95c96 Compare April 4, 2023 07:44
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Gradle Check (Jenkins) Run Completed with:

@dblock
Copy link
Member

dblock commented Apr 4, 2023

Needs a CHANGELOG. Would also search documentation for where we mention this, and/or edit/add this, and see if we're breaking any contracts. I wish we could actually test this, too ...

I think we can backport to 2.x, @reta lmk if you feel like this is a breaking change. IMO while the behavior of setting OPENSEARCH_HOME before was "has no effect", we never spelled it out anywhere.

@shyim shyim force-pushed the allow-overwrite-opensearch-home branch from cb95c96 to 8165c9f Compare April 4, 2023 17:50
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Gradle Check (Jenkins) Run Completed with:

@reta
Copy link
Collaborator

reta commented Apr 4, 2023

I think we can backport to 2.x, @reta lmk if you feel like this is a breaking change. IMO while the behavior of setting OPENSEARCH_HOME before was "has no effect", we never spelled it out anywhere.

@dblock I am a bit concerned about this change (@shyim I think I do understand the issue you are trying to solve): the opensearch-env has a strict assumptions that all the bits (shell scripts, libs, bundled jdk) are located in the exact same folder as this file itself. But with this change the opensearch-env assumptions become irrelevant: you could run the script from one location but it could pick the bits from anywhere literally (whatever OPENSEARCH_HOME says).

It opens a whole class of issues (possibly even security ones?), @peterzhuamazon do you folks have an opinion on that?

@peterzhuamazon
Copy link
Member

Thanks @shyim for the contribution and @dblock @reta for the review!

@peterzhuamazon
Copy link
Member

Hi @opensearch-project/opensearch-core could you give an approval on this?
Thanks.

@peterzhuamazon peterzhuamazon added the backport 2.x Backport to 2.x branch label Apr 5, 2023
@peterzhuamazon
Copy link
Member

@reta I think this is more of a minor verison change than patch version.
Thoughts? Currently only adding backport to 2.x and 1.x.

Thanks.

@dblock dblock merged commit 0202de5 into opensearch-project:main Apr 6, 2023
15 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 1.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-1.x 1.x
# Navigate to the new working tree
pushd ../.worktrees/backport-1.x
# Create a new branch
git switch --create backport/backport-6956-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0202de5849dcdcef61e1a2ad3b6a270574fbf2ed
# Push it to GitHub
git push --set-upstream origin backport/backport-6956-to-1.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-1.x

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

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

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

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-6956-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0202de5849dcdcef61e1a2ad3b6a270574fbf2ed
# Push it to GitHub
git push --set-upstream origin backport/backport-6956-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-6956-to-2.x.

@dblock
Copy link
Member

dblock commented Apr 6, 2023

Looks like this will need manual backports if you want it in 2.x and 1.x :(

@@ -46,6 +46,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Change http code on create index API with bad input raising NotXContentException from 500 to 400 ([#4773](https://github.com/opensearch-project/OpenSearch/pull/4773))
- Change http code for DecommissioningFailedException from 500 to 400 ([#5283](https://github.com/opensearch-project/OpenSearch/pull/5283))
- Improve summary error message for invalid setting updates ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792))
- Changed `opensearch-env` to respect already set `OPENSEARCH_HOME` environment variable ([#6956](https://github.com/opensearch-project/OpenSearch/pull/6956/))
Copy link
Member

Choose a reason for hiding this comment

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

This should be in the [Unreleased 2.x] section since it is to be backported and released. This is also likely the reason the cherry pick failed on the automated backport.

@peterzhuamazon
Copy link
Member

Hi @shyim can you take a look at above comments?
Thanks!

@shyim
Copy link
Contributor Author

shyim commented Apr 7, 2023

Do I have to make now the cherry picks manually for 1.x/2.x?

@shyim shyim deleted the allow-overwrite-opensearch-home branch April 7, 2023 10:40
@vamsi-amazon
Copy link
Member

vamsi-amazon commented Apr 10, 2023

sql windows integration tests are failing with below error.

Exec output and error:
| Output for cmd:The following usage of the path operator in batch-parameter
| substitution is invalid: %~nxI
|
|
| For valid formats type CALL /? or FOR /?
| The syntax of the command is incorrect.

Is this error related to these changes. I haven't made any changes to batch file in sql code base. I still couldn't figure out the root cause. @shyim Could you please help me out here?

Integ tests kind of spin up an opensearch cluster, so it seems to me these are related. I will confirm if this is caused by my changes, by creating a new PR in sql with almost no changes.

This PR has only read me change: https://github.com/opensearch-project/sql/actions/runs/4660531784/jobs/8248701010#step:4:2063 but still fails, so probably the issue should be in upstream. @peterzhuamazon @dblock

The other related issues : opensearch-project/security#2657

@scrawfor99
Copy link
Contributor

scrawfor99 commented Apr 10, 2023

To echo @vamsi-amazon, Security and Job Scheduler are also failing. I looked into this on the Security end and it does not appear to be related. I am going to check some stuff here, but this may require an expedited patch or revert.

The updated code could cause the error reported if the %I variable does not contain a valid path. We need to make sure %I contains a valid path before attempting to use the ~nx modifier. If %OPENSEARCH_HOME% does not contain a valid path, then the ~nx modifier will not work.

Something like this needs to be added:

for %%I in (opensearch.bat) do (
    set "OPENSEARCH_HOME=%%~dpI"
)


to fix the issue.

I think that #7079 should fix the issue but I am not sure how to test its impact on downstream repos without merging. @dblock what would you recommend?

peterzhuamazon added a commit that referenced this pull request Apr 11, 2023
…issues on OPENSEARCH_HOME assignment introduced in #6956 (#7080)

* Resolve windows opensearch-env.bat having variable delayed expansion issues on OPENSEARCH_HOME assignment

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Remove changelog entry

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

---------

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 11, 2023
…issues on OPENSEARCH_HOME assignment introduced in #6956 (#7080)

* Resolve windows opensearch-env.bat having variable delayed expansion issues on OPENSEARCH_HOME assignment

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Remove changelog entry

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

---------

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
(cherry picked from commit 810b985)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
peterzhuamazon pushed a commit that referenced this pull request Apr 11, 2023
…issues on OPENSEARCH_HOME assignment introduced in #6956 (#7080) (#7109)

* Resolve windows opensearch-env.bat having variable delayed expansion issues on OPENSEARCH_HOME assignment



* Remove changelog entry



---------


(cherry picked from commit 810b985)

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 11, 2023
…issues on OPENSEARCH_HOME assignment introduced in #6956 (#7080)

* Resolve windows opensearch-env.bat having variable delayed expansion issues on OPENSEARCH_HOME assignment

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Remove changelog entry

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

---------

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
(cherry picked from commit 810b985)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
peterzhuamazon pushed a commit that referenced this pull request Apr 11, 2023
…issues on OPENSEARCH_HOME assignment introduced in #6956 (#7080) (#7111)

* Resolve windows opensearch-env.bat having variable delayed expansion issues on OPENSEARCH_HOME assignment



* Remove changelog entry



---------


(cherry picked from commit 810b985)

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
austintlee pushed a commit to austintlee/OpenSearch that referenced this pull request Apr 28, 2023
…issues on OPENSEARCH_HOME assignment introduced in opensearch-project#6956 (opensearch-project#7080)

* Resolve windows opensearch-env.bat having variable delayed expansion issues on OPENSEARCH_HOME assignment

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Remove changelog entry

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

---------

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.x backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants