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

Jump to bottom

Suppress error message when /proc/sys/vm/max_map_count is not exists. #35933

Merged
merged 2 commits into from
Feb 12, 2019
Merged

Suppress error message when /proc/sys/vm/max_map_count is not exists. #35933

merged 2 commits into from
Feb 12, 2019

Conversation

Milly
Copy link
Contributor

@Milly Milly commented Nov 27, 2018

If /proc/sys/vm/max_map_count is not exists,
Before patch: $(cat /proc/sys/vm/max_map_count) is evaluated before test, and show error message like cat: /proc/sys/vm/max_map_count: No such file or directory.
After patch: Separate a file exists test and call cat.

@Milly
Copy link
Contributor Author

Milly commented Nov 27, 2018

#27236

@cbuescher cbuescher added >enhancement :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts labels Nov 27, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@dliappis dliappis self-requested a review January 30, 2019 13:03
@dliappis
Copy link
Contributor

Thank you for the PR and sorry for the time taken to review.

I can see the issue here since -a is not a shell control operator (i.e. doesn't short circuit). I was able to reproduce the problem with something like (note the syste typo is intentional to simulate a file not present):

$ MAX_MAP_COUNT=200000; if [ -n "$MAX_MAP_COUNT" -a -f /proc/system/vm/max_map_count -a "$MAX_MAP_COUNT" -gt $(cat /proc/syste/vm/max_map_count) ]; then echo "max_map_count needs to change"; else echo "no change needed"; fi
cat: /proc/syste/vm/max_map_count: No such file or directory
-bash: [: too many arguments
no change needed

vs your suggestion:

$ MAX_MAP_COUNT=200000; if [ -n "$MAX_MAP_COUNT" -a -f /proc/system/vm/max_map_count ] && ["$MAX_MAP_COUNT" -gt $(cat /proc/syste/vm/max_map_count) ]; then echo "max_map_count needs to change"; else echo "no change needed"; fi
no change needed

Normally we'd like to have some tests to ensure this is working as expected, as shown e.g. here: https://github.com/elastic/elasticsearch/pull/31285/files. However, I think that it will be rather hard to mock /proc/sys/vm/max_map_count (make it go away) for packaging tests. I am not sure in which environments this happens, I am guessing /proc/sys/vm/max_map_count is unavailable with certain OS's inside Docker e.g. on certain k8s platforms.

@danielmitterdorfer are we ok to contain the complexity here by not seeking a way to test this?

dliappis
dliappis approved these changes Feb 11, 2019
View reviewed changes
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

LGTM from me, I left a comment on why I think adding a test, albeit useful, would be a complicated thing, hence, this can be merge as is.

@dliappis
Copy link
Contributor

@elasticmachine test this please

@dliappis
Copy link
Contributor

@Milly would it be possible that you merge master into the branch of your fork used for this PR (and push to the same) so that we can resolve some CI failures?

@danielmitterdorfer
Copy link
Member

@danielmitterdorfer are we ok to contain the complexity here by not seeking a way to test this?

Yes, I agree with your reasoning.

@danielmitterdorfer
Copy link
Member

@elasticmachine test this please

@danielmitterdorfer
Copy link
Member

@Milly I have merged the latest master now into your fork on your behalf so we can get a successful CI build and we'll take it from there. /cc: @dliappis.

@danielmitterdorfer
Copy link
Member

The packaging tests failed while starting a Windows Vagrant box which seems unrelated to me.

@elasticmachine run elasticsearch-ci/packaging

@dliappis
Copy link
Contributor

Thanks @danielmitterdorfer, all CI is green now, so will merge.

@dliappis dliappis merged commit eac14af into elastic:master Feb 12, 2019
dliappis pushed a commit to dliappis/elasticsearch that referenced this pull request Feb 12, 2019
@Milly @dliappis
Don't fail init script if /proc/.../max_map_count absent (elastic#3…
fb0c9fe 
…5933)

Currently init scripts fail when `/proc/sys/vm/max_map_count` is not present
with `-bash: [: too many arguments`.

Fix conditional logic to avoid trying to set the `max_map_count` sysctl if not
present.

Relates elastic#27236
dliappis pushed a commit to dliappis/elasticsearch that referenced this pull request Feb 12, 2019
@Milly @dliappis
Don't fail init script if /proc/.../max_map_count absent (elastic#3…
28ef76d 
…5933)

Currently init scripts fail when `/proc/sys/vm/max_map_count` is not present
with `-bash: [: too many arguments`.

Fix conditional logic to avoid trying to set the `max_map_count` sysctl if not
present.

Relates elastic#27236
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 13, 2019
@jasontedor
Merge branch 'master' into soft-delete-policy-off-by-one
6d23d8f 
* master:
  Remove _type term filters from cluster alert watches (elastic#38819)
  Adjust log and unmute testFailOverOnFollower (elastic#38762)
  Fix line separators in JSON logging tests (elastic#38771)
  Fix synchronization in LocalCheckpointTracker#contains (elastic#38755)
  muted test
  Remove TLSv1.2 pinning in ssl reload tests (elastic#38651)
  Don't fail init script if `/proc/.../max_map_count` absent (elastic#35933)
  Format Watcher.status.lastChecked and lastMetCondition (elastic#38626)
  SQL: Implement `::` cast operator (elastic#38774)
  Ignore failing test
dliappis added a commit that referenced this pull request Feb 13, 2019
@dliappis
Don't fail init script if /proc/.../max_map_count absent (#38789)
470ac0e 
Currently init scripts fail when `/proc/sys/vm/max_map_count` is not present
with `-bash: [: too many arguments`.

Fix conditional logic to avoid trying to set the `max_map_count` sysctl if not
present.

Backport of: #35933
Relates: #27236
dliappis added a commit that referenced this pull request Feb 13, 2019
@dliappis
Don't fail init script if /proc/.../max_map_count absent (#38791)
a66efba 
Currently init scripts fail when `/proc/sys/vm/max_map_count` is not present
with `-bash: [: too many arguments`.

Fix conditional logic to avoid trying to set the `max_map_count` sysctl if not
present.

Backport of: #35933
Relates: #27236
dliappis added a commit that referenced this pull request Feb 13, 2019
@dliappis
Don't fail init script if /proc/.../max_map_count absent (#38792)
a328654 
Currently init scripts fail when `/proc/sys/vm/max_map_count` is not present
with `-bash: [: too many arguments`.

Fix conditional logic to avoid trying to set the `max_map_count` sysctl if not
present.

Backport of: #35933
Relates: #27236
@Milly Milly deleted the suppress-error-no-max_map_count branch March 14, 2019 00:54
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dliappis dliappis dliappis approved these changes

Assignees
No one assigned
Labels
>bug :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts Team:Delivery Meta label for Delivery team v6.7.0 v7.0.0-rc1 v7.2.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Milly @elasticmachine @dliappis @danielmitterdorfer @jakelandis @mark-vieira @michaelbaamonde @cbuescher