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.

Sign up for GitHub

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

Fix opensearch-env always sources the environment from hardcoded file #875

Merged
merged 1 commit into from
Jul 21, 2021
Merged

Fix opensearch-env always sources the environment from hardcoded file #875

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion distribution/src/bin/opensearch-env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ fi

export HOSTNAME=$HOSTNAME

${source.path.env}
if [ -z "$OPENSEARCH_PATH_CONF" ]; then
${source.path.env}
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently this sources this file. This has other environment variables which won't be set if there is an environment variable OPENSEARCH_PATH_CONF in the current shell.
This might create problems without the user realizing why.

I think this needs be properly communicated in documentation and by logging a warning message.

Copy link
Collaborator

@tlfeng tlfeng Jul 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I agree there needs a properly notice to the users to aware the other environment variable in ${source.path.env}, because there is one:

OpenSearch/distribution/packages/src/common/env/opensearch

Line 31 in 34550c5

OPENSEARCH_STARTUP_SLEEP_TIME=5

But during my test, I found the variables in above file can still be set through systemd service file:

OpenSearch/distribution/packages/src/common/systemd/opensearch.service

Line 15 in 34550c5

EnvironmentFile=-${path.env}
. After the users commenting out this line manually, they can override the value of OPENSEARCH_PATH_CONF, and start several OpenSearch processes in the same host.
So in my opinion, the users understand what they are doing because they still need to do the above additional step, and common users will not be affected by the change in the PR.
While, if the user starts multiple OpenSearch processes without using the systemd service (though I don't know if there is a way to skip the systemd sevice), the value of OPENSEARCH_STARTUP_SLEEP_TIME is not set and the user may not realize.

Copy link
Contributor

@adnapibar adnapibar Jul 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That's (OPENSEARCH_STARTUP_SLEEP_TIME=5 ) the only uncommented one but there are other variables that might have been modified. The service can be started without systemd. We can assume that the users know what they are doing but if we're changing existing behaviour it should be properly communicated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks Rabi 😄 I see, you are right.
With the change in the PR, when user set custom OPENSEARCH_PATH_CONF, and started OpenSearch process without systemd (such as running the opensearch bash script directly) the default values in ${source.path.env} will not be loaded, so it needs a notice.

fi

if [ -z "$OPENSEARCH_PATH_CONF" ]; then
echo "OPENSEARCH_PATH_CONF must be set to the configuration path"
Expand Down