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

[ML] Fix deletion forecast overflow #110

Merged

Conversation

hendrikmuhs
Copy link

@hendrikmuhs hendrikmuhs commented May 30, 2018

Use forecast ID to create a subdirectory when overflowing models to disk for forecasting. This fixes a failing 2nd forecast call because the 1st one deleted the tmp directory.

The change required adding mkdir permission to seccomp.

I have an integration test ready, to be committed in elasticsearch, see elastic/elasticsearch#30969.

Non-issue: unreleased feature

BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_clone, 24, 0),
BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_statfs, 23, 0),
BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_dup2, 22, 0),
BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_mkdir, 21, 0), // for forecast temp storage
Copy link
Author

Choose a reason for hiding this comment

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

^ the only change

@@ -53,20 +53,21 @@ const struct sock_filter FILTER[] = {
// Only applies to X86_64 arch. Jump to disallow for calls using the x32 ABI
BPF_JUMP(BPF_JMP | BPF_JGT | BPF_K, UPPER_NR_LIMIT, 34, 0),
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 change to 35 so it jumps to disallow. It's very easy to miss I'll make a PR either adding a comment or refactor the filter.

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM

@hendrikmuhs hendrikmuhs force-pushed the fix-deletion-forecast-overflow branch from af77263 to a82a739 Compare May 30, 2018 11:31
@hendrikmuhs hendrikmuhs merged commit 22a57a2 into elastic:master May 30, 2018
hendrikmuhs pushed a commit to hendrikmuhs/ml-cpp that referenced this pull request May 30, 2018
Use forecast ID to create a subdirectory when overflowing models to disk for forecasting. This fixes a failing 2nd forecast call because the 1st one deleted the tmp directory.
davidkyle pushed a commit to davidkyle/ml-cpp that referenced this pull request Jun 1, 2018
Use forecast ID to create a subdirectory when overflowing models to disk for forecasting. This fixes a failing 2nd forecast call because the 1st one deleted the tmp directory.
hendrikmuhs pushed a commit to hendrikmuhs/elasticsearch that referenced this pull request Jun 4, 2018
hendrikmuhs pushed a commit to elastic/elasticsearch that referenced this pull request Jun 5, 2018
…p#110 (#30969)

Improve test to run overflow forecast a 2nd time as regression test for elastic/ml-cpp#110
martijnvg added a commit to elastic/elasticsearch that referenced this pull request Jun 5, 2018
* es/master:
  Take into account the return value of TcpTransport.readMessageLength(...) in Netty4SizeHeaderFrameDecoder
  Move caching of the size of a directory to `StoreDirectory`. (#30581)
  Clarify docs about boolean operator precedence. (#30808)
  Docs: remove notes on sparsity. (#30905)
  Fix MatchPhrasePrefixQueryBuilderTests#testPhraseOnFieldWithNoTerms
  run overflow forecast a 2nd time as regression test for elastic/ml-cpp#110 (#30969)
  Improve documentation of dynamic mappings. (#30952)
  Decouple MultiValueMode. (#31075)
  Docs: Clarify constraints on scripted similarities. (#31076)
  Update get.asciidoc (#31084)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jun 5, 2018
* elastic/master:
  [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient
  TEST:  Retry synced-flush if ongoing ops on primary (elastic#30978)
  Fix docs build.
  Only auto-update license signature if all nodes ready (elastic#30859)
  Add BlobContainer.writeBlobAtomic() (elastic#30902)
  Add a doc value format to binary fields. (elastic#30860)
  Take into account the return value of TcpTransport.readMessageLength(...) in Netty4SizeHeaderFrameDecoder
  Move caching of the size of a directory to `StoreDirectory`. (elastic#30581)
  Clarify docs about boolean operator precedence. (elastic#30808)
  Docs: remove notes on sparsity. (elastic#30905)
  Fix MatchPhrasePrefixQueryBuilderTests#testPhraseOnFieldWithNoTerms
  run overflow forecast a 2nd time as regression test for elastic/ml-cpp#110 (elastic#30969)
  Improve documentation of dynamic mappings. (elastic#30952)
  Decouple MultiValueMode. (elastic#31075)
  Docs: Clarify constraints on scripted similarities. (elastic#31076)
hendrikmuhs pushed a commit to elastic/elasticsearch that referenced this pull request Jun 6, 2018
…p#110 (#30969)

Improve test to run overflow forecast a 2nd time as regression test for elastic/ml-cpp#110
@sophiec20 sophiec20 added :ml and removed :ml labels Jun 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants