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

Ensure tombstones created before kubexit started are read #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RemcodM
Copy link
Contributor

@RemcodM RemcodM commented Aug 10, 2020

Fixes #8
Replaces #9, this should be a better PR with less unneeded refactoring

This commit changes the Watch function inside the tombstone package to
also emit an fake 'Created' event besides the real fsnotify events. This
initial event is send out immediatly when Watch is called and the watcher
has been setup.

This change allows kubexit to detect tombstones written before kubexit was
started. This prevents possible race conditions as described by #8.

This change on its own introduces a new bug where the tombstone is written
as part of an initial event, but the child process will still start because
child.Start() is being called after the watcher has been setup. To overcome
this issue, the shutdown state of the child is tracked in a new flag, which is
set if ShutdownNow() or ShutdownWithTimeout() is executed.

This commit changes the `Watch` function inside the `tombstone` package to
also emit an fake 'Created' event besides the real `fsnotify` events. This
initial event is send out immediatly when `Watch` is called and the watcher
has been setup.

This change allows kubexit to detect tombstones written before kubexit was
started. This prevents possible race conditions as described by karlkfi#8.

This change on its own introduces a new bug where the tombstone is written
as part of an initial event, but the child process will still start because
`child.Start()` is being called after the watcher has been setup. To overcome
this issue, the shutdown state of the child is tracked in a new flag, which is
set if `ShutdownNow()` or `ShutdownWithTimeout()` is executed.
@RemcodM
Copy link
Contributor Author

RemcodM commented Aug 10, 2020

New PR since I decided to make the changes from scratch. I hope you will like this one better 😄

@karlkfi
Copy link
Owner

karlkfi commented Aug 12, 2020

If a death dep is already dead, do we still want to wait for the birth deps? That's what your new shutdown flag in child.Start does. It might make more sense to just exit early.

With the new Watch change, any existing tombstones get dealt with synchronously, instead of asynchronously. I think I can make it so ShutdownWithTimeout errors if it's not started, and then fail fast within Watch using an EventHandler that returns an error. If the EventHandler errors asynchronously, the error would just get logged, but if it errors synchronously then Watch can error, which would cause a fatal exit in main.

@tnachen
Copy link

tnachen commented Jan 4, 2021

Any plan to merge this or @RemcodM's? I'm running on a patched one forever, if this is no longer maintained I guess we just have to fork this.

alessiovolpe referenced this pull request in Musixmatchdev/kubexit May 31, 2022
Bumps [github.com/fsnotify/fsnotify](https://github.com/fsnotify/fsnotify) from 1.4.9 to 1.5.1.
- [Release notes](https://github.com/fsnotify/fsnotify/releases)
- [Changelog](https://github.com/fsnotify/fsnotify/blob/master/CHANGELOG.md)
- [Commits](fsnotify/fsnotify@v1.4.9...v1.5.1)

---
updated-dependencies:
- dependency-name: github.com/fsnotify/fsnotify
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kubexit does not shutdown child when tombstone is written before it is started
3 participants