Fix opensearch-env always sources the environment from hardcoded file #875
Conversation
|
❌ DCO Check Failed 3d313af3446fa7a7c5a1c9462e672101ca543673 |
|
✅ Gradle Wrapper Validation success 3d313af3446fa7a7c5a1c9462e672101ca543673 |
|
✅ Gradle Precommit success 3d313af3446fa7a7c5a1c9462e672101ca543673 |
Signed-off-by: Xue Zhou <xuezhou@amazon.com>
|
✅ DCO Check Passed c2c93ba |
|
✅ Gradle Wrapper Validation success c2c93ba |
|
✅ Gradle Precommit success c2c93ba |
| @@ -78,7 +78,9 @@ fi | |||
|
|
|||
| export HOSTNAME=$HOSTNAME | |||
|
|
|||
| ${source.path.env} | |||
| if [ -z "$OPENSEARCH_PATH_CONF" ]; then | |||
| ${source.path.env} | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree there needs a properly notice to the users to aware the other environment variable in ${source.path.env}, because there is one:
But during my test, I found the variables in above file can still be set through systemd service file: . 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
✅ Gradle Wrapper Validation success c2c93ba |
|
✅ DCO Check Passed c2c93ba |
nknize
added
bug
|
✅ Gradle Precommit success c2c93ba |
|
❌ Gradle Precommit failure 745155185fab8405353da16a9e3924c91bf8eb6a |
|
✅ Gradle Wrapper Validation success 745155185fab8405353da16a9e3924c91bf8eb6a |
|
❌ DCO Check Failed 745155185fab8405353da16a9e3924c91bf8eb6a |
|
❌ DCO Check Failed 3f259df25a79e5e3031efb401ca0fa2d70563375 |
|
✅ Gradle Wrapper Validation success 3f259df25a79e5e3031efb401ca0fa2d70563375 |
|
✅ Gradle Precommit success 3f259df25a79e5e3031efb401ca0fa2d70563375 |
xuezhou25
force-pushed
the
new_branch
branch
2 times, most recently
from
7451551
to
c2c93ba
Compare
|
✅ Gradle Wrapper Validation success 745155185fab8405353da16a9e3924c91bf8eb6a |
|
✅ DCO Check Passed c2c93ba |
|
❌ Gradle Precommit failure 745155185fab8405353da16a9e3924c91bf8eb6a |
|
✅ Gradle Wrapper Validation success c2c93ba |
|
✅ Gradle Precommit success c2c93ba |
|
@xuezhou25 what is the original problem that you are trying to fix with this? why would you need this override? @adnapibar It sounds like we don't want this change because of the undesirable side effects? |
The original problem is https://github.com/opensearch-project/OpenSearch/issues/633 |
Ok, that is clear. So this is a feature that allows one to override
With ^ this LGTM. |
|
start gradle check |
There might be some edge cases and you're right we do need some tests to verify. But at least documenting it should make it clear to the users. |
…opensearch-project#875) distribution/bin/opensearch-env always sources the environment from the default environment file /etc/default/opensearch. This is an issue if we want to run multiple instances of OpenSearch on the same host. This change lets users override the default behavior by not sourcing the default environment file in case OPENSEARCH_PATH_CONF is set. Signed-off-by: Xue Zhou <xuezhou@amazon.com>
…opensearch-project#875) distribution/bin/opensearch-env always sources the environment from the default environment file /etc/default/opensearch. This is an issue if we want to run multiple instances of OpenSearch on the same host. This change lets users override the default behavior by not sourcing the default environment file in case OPENSEARCH_PATH_CONF is set. Signed-off-by: xuezhou25 <85715413+xuezhou25@users.noreply.github.com> Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
…opensearch-project#875) distribution/bin/opensearch-env always sources the environment from the default environment file /etc/default/opensearch. This is an issue if we want to run multiple instances of OpenSearch on the same host. This change lets users override the default behavior by not sourcing the default environment file in case OPENSEARCH_PATH_CONF is set. Signed-off-by: xuezhou25 <85715413+xuezhou25@users.noreply.github.com> Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
…#875) (#2908) backport commit e3d86ba to 1.x branch distribution/bin/opensearch-env always sources the environment from the default environment file /etc/default/opensearch. This is an issue if we want to run multiple instances of OpenSearch on the same host. This change lets users override the default behavior by not sourcing the default environment file in case OPENSEARCH_PATH_CONF is set. Signed-off-by: xuezhou25 <xuezhou25@amazon.com>
…#875) (#2909) backport commit e3d86ba to 1.3 branch distribution/bin/opensearch-env always sources the environment from the default environment file /etc/default/opensearch. This is an issue if we want to run multiple instances of OpenSearch on the same host. This change lets users override the default behavior by not sourcing the default environment file in case OPENSEARCH_PATH_CONF is set. Signed-off-by: xuezhou25 <xuezhou25@amazon.com>
Description
Fix line 81 of opensearch-env always sources the environment from hardcoded file. Let users override the value of OPENSEARCH_PATH_CONF.
Issues Resolved
#633
Check List
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.