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

Fix waiting for new events #10593

Merged
merged 11 commits into from
Feb 14, 2019
Merged

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Feb 5, 2019

  • Adding more syscalls to the whitelist in libbeat
    I have removed the seccomp policy of journalbeat. Four new syscalls has been added to the common policy instead: fstatfs, getrlimit, ppoll and splice.
  • Eliminate possible deadlock when closing the client
  • Previously, journalbeat was unable to read new entries from the journal after it has been started. Now it is fixed, and the reader code is much simpler now.
  • updated vendored github.com/coreos/go-systemd/sdjournal to latest release (no changes)

@kvch kvch added in progress Pull request is currently in progress. review regression Journalbeat labels Feb 5, 2019
@kvch kvch requested a review from a team as a code owner February 5, 2019 22:04
@kvch kvch added the needs_backport PR is waiting to be backported to other branches. label Feb 5, 2019
@andrewkroh
Copy link
Member

andrewkroh commented Feb 5, 2019

I don't see anything egregious in what you are adding so I think you should just add these syscalls to the default policy in libbeat.

Confirming what you said in the description, the diff is:

26a27
> 					"fstatfs",
38a40
> 					"getrlimit",
65a68
> 					"ppoll",
91a95
> 					"splice",

@kvch kvch force-pushed the fix-journalbeat-seccomp-policy branch from 81393c8 to cff68ba Compare February 12, 2019 09:35
@kvch kvch removed the in progress Pull request is currently in progress. label Feb 12, 2019
@kvch kvch requested review from urso and ph and removed request for a team February 12, 2019 09:46
}

// error while reading next entry
if c < 0 {
Copy link

Choose a reason for hiding this comment

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

according to docs c is uint64. Why does this compile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

@urso urso Feb 12, 2019

Choose a reason for hiding this comment

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

This c is not from the call to Wait, but from Next. You even shadow/redefine c when calling Wait.
Sometimes it's a good idea not to shadow/reuse variable names.

See: https://godoc.org/github.com/coreos/go-systemd/sdjournal#Journal.Next

Seems like sdjournal API is somewhat inconsistent here.

How about wrapping Wait, so to reduce the chance of confusion (and reducing complexity in the Next method itself):

func journalWait(j *Journal, dur time.Duration) (has bool, err error) {
  switch c := j.Wait(dur) {
  case sdjournal.SD_JOURNAL_NOP:
    return false, nil
  case sdjournal.SD_JOURNAL_APPEND, sdjournal.SD_JOURNAL_INVALIDATE:
    return true, nil
  default:
    if c < 0 {
 	  return false, fmt.Errorf("error while waiting for event: %+v", syscall.Errno(-c))
    }
    return false, fmt.Errorf("Unknown return code from Wait: %d\n", c)
  }
}

This can be used like:

for {
  ...

  c, err := r.journal.Next()
  if err != nil && err != io.EOF {
    return nil, err
  }
  if c == 0 {
    has, err := journalWait(r.journal, 100 * time.Millisecond)
    if err != nil {
      return nil, err
    }

    if !has {
      continue
    }
  }

  ...
}

Interestingly all other calls on journal properly return error values, but Wait does not. Was Wait added by the original developers or some contributor?
Maybe we can open a PR/issue to enhance consistency in the sdjournal API. And add support for getting the FD itself. Plus expose some other missing functions like sd_journal_process & co.

Copy link

Choose a reason for hiding this comment

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

TIL go compiler does not warn/error on not-satisfiable comparisons on uint types: https://play.golang.org/p/g0NFrotaKd-

Copy link

Choose a reason for hiding this comment

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

Question: What shall we do if Wait returns an unknown value >0?

  1. treat it as error (my sample code) ?
  2. treat it like SD_JOURNAL_APPEND (current implementation) ?
  3. treat it like SD_JOURNAL_NOP ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my implementation (or at least what I intended to implement before messing up with copy-pasting my previous code... :D), an error is logged and nil is returned from Next. Thus, the reader tries to read a new entry again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait seems like it was added later, when sd-journal.h became a dependency.
I would like to open a PR there to add support for the functions we need to check for events without using Wait. (As you proposed it in a previous PR.)

Copy link

Choose a reason for hiding this comment

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

This conditional c < 0 is still a no-op, as it will always return false. Let's remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@kvch kvch force-pushed the fix-journalbeat-seccomp-policy branch from 165a817 to eb2d1a8 Compare February 12, 2019 10:16
@kvch
Copy link
Contributor Author

kvch commented Feb 12, 2019

Failing tests are unrelated.

@kvch
Copy link
Contributor Author

kvch commented Feb 12, 2019

Follow up issue for testing: #10699
Follow up issue for documentation: #10700

@kvch
Copy link
Contributor Author

kvch commented Feb 12, 2019

Failing tests are still unrelated.

@kvch kvch force-pushed the fix-journalbeat-seccomp-policy branch from 5b1a08f to b679db7 Compare February 13, 2019 17:31
@kvch
Copy link
Contributor Author

kvch commented Feb 14, 2019

Failing tests are unrelated.

@kvch kvch merged commit 1dc58e0 into elastic:master Feb 14, 2019
kvch added a commit to kvch/beats that referenced this pull request Feb 14, 2019
* Adding more syscalls to the whitelist in `libbeat`
 I have removed the seccomp policy of `journalbeat`. Four new syscalls has been added to the common policy instead: `fstatfs`, `getrlimit`, `ppoll` and `splice`.
* Eliminate possible deadlock when closing the client
* Previously, `journalbeat` was unable to read new entries from the journal after it has been started. Now it is fixed, and the reader code is much simpler now.
* updated vendored `github.com/coreos/go-systemd/sdjournal` to latest release (no changes)

(cherry picked from commit 1dc58e0)
@kvch kvch added v7.2.0 and removed needs_backport PR is waiting to be backported to other branches. labels Feb 14, 2019
@kvch kvch added the v7.0.0 label Feb 14, 2019
kvch added a commit to kvch/beats that referenced this pull request Feb 15, 2019
* Adding more syscalls to the whitelist in `libbeat`
 I have removed the seccomp policy of `journalbeat`. Four new syscalls has been added to the common policy instead: `fstatfs`, `getrlimit`, `ppoll` and `splice`.
* Eliminate possible deadlock when closing the client
* Previously, `journalbeat` was unable to read new entries from the journal after it has been started. Now it is fixed, and the reader code is much simpler now.
* updated vendored `github.com/coreos/go-systemd/sdjournal` to latest release (no changes)

(cherry picked from commit 1dc58e0)
kvch added a commit that referenced this pull request Feb 15, 2019
* Adding more syscalls to the whitelist in `libbeat`
 I have removed the seccomp policy of `journalbeat`. Four new syscalls has been added to the common policy instead: `fstatfs`, `getrlimit`, `ppoll` and `splice`.
* Eliminate possible deadlock when closing the client
* Previously, `journalbeat` was unable to read new entries from the journal after it has been started. Now it is fixed, and the reader code is much simpler now.
* updated vendored `github.com/coreos/go-systemd/sdjournal` to latest release (no changes)

(cherry picked from commit 1dc58e0)
kvch added a commit to kvch/beats that referenced this pull request Feb 19, 2019
* Adding more syscalls to the whitelist in `libbeat`
 I have removed the seccomp policy of `journalbeat`. Four new syscalls has been added to the common policy instead: `fstatfs`, `getrlimit`, `ppoll` and `splice`.
* Eliminate possible deadlock when closing the client
* Previously, `journalbeat` was unable to read new entries from the journal after it has been started. Now it is fixed, and the reader code is much simpler now.
* updated vendored `github.com/coreos/go-systemd/sdjournal` to latest release (no changes)

(cherry picked from commit 1dc58e0)
@kvch kvch added the v6.7.0 label Feb 19, 2019
kvch added a commit that referenced this pull request Mar 4, 2019
* Adding more syscalls to the whitelist in `libbeat`
 I have removed the seccomp policy of `journalbeat`. Four new syscalls has been added to the common policy instead: `fstatfs`, `getrlimit`, `ppoll` and `splice`.
* Eliminate possible deadlock when closing the client
* Previously, `journalbeat` was unable to read new entries from the journal after it has been started. Now it is fixed, and the reader code is much simpler now.
* updated vendored `github.com/coreos/go-systemd/sdjournal` to latest release (no changes)

(cherry picked from commit 1dc58e0)
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
* Adding more syscalls to the whitelist in `libbeat`
 I have removed the seccomp policy of `journalbeat`. Four new syscalls has been added to the common policy instead: `fstatfs`, `getrlimit`, `ppoll` and `splice`.
* Eliminate possible deadlock when closing the client
* Previously, `journalbeat` was unable to read new entries from the journal after it has been started. Now it is fixed, and the reader code is much simpler now.
* updated vendored `github.com/coreos/go-systemd/sdjournal` to latest release (no changes)
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
* Adding more syscalls to the whitelist in `libbeat`
 I have removed the seccomp policy of `journalbeat`. Four new syscalls has been added to the common policy instead: `fstatfs`, `getrlimit`, `ppoll` and `splice`.
* Eliminate possible deadlock when closing the client
* Previously, `journalbeat` was unable to read new entries from the journal after it has been started. Now it is fixed, and the reader code is much simpler now.
* updated vendored `github.com/coreos/go-systemd/sdjournal` to latest release (no changes)

(cherry picked from commit 1dc58e0)
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