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 #9

Closed
wants to merge 1 commit into from

Conversation

RemcodM
Copy link
Contributor

@RemcodM RemcodM commented Aug 10, 2020

fixes #8

This commit changes the Watch function inside the tombstone package to also emit an initial event besides the fsnotify events. This initial event is called 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.

In order for this change to work, the tombstone.EventHandler type was changed. It now requires a function with 3 arguments: The graveyard, the tombstone and the operation instead of an fsnotify.Event. Reason being that the initial event is not an fsnotify.Event. The functions implementing a tombstone.EventHandler are changed accordingly.

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 initial event besides the `fsnotify` events. This initial event
is called 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.

In order for this change to work, the `tombstone.EventHandler` type was
changed. It now requires a function with 3 arguments: The graveyard, the
tombstone and the operation instead of an `fsnotify.Event`. Reason being
that the initial event is not an `fsnotify.Event`. The functions implementing
an `tombstone.EventHandler` are changed accordingly.

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.
@karlkfi
Copy link
Owner

karlkfi commented Aug 10, 2020

Thanks for the bug report, but I think this refactor is probably unnecessary. It's probably cleaner to just trigger the event handler with a fake create event, since the struct, type, and constants are public. https://github.com/fsnotify/fsnotify/blob/master/fsnotify.go#L16-L32

files, err := ioutil.ReadDir(graveyard)
if err != nil {
	return fmt.Errorf("failed to read graveyard dir: %v", err)
}

for _, f := range files {
	event := fsnotify.Event{
		Name:	filepath.Join(graveyard, f.Name())
		Op:		fsnotify.Create,
	}
	eventHandler(event)
}

@RemcodM
Copy link
Contributor Author

RemcodM commented Aug 10, 2020

Superseded by #10

@RemcodM RemcodM closed this Aug 10, 2020
alessiovolpe referenced this pull request in Musixmatchdev/kubexit May 31, 2022
Bumps [sigs.k8s.io/yaml](https://github.com/kubernetes-sigs/yaml) from 1.2.0 to 1.3.0.
- [Release notes](https://github.com/kubernetes-sigs/yaml/releases)
- [Changelog](https://github.com/kubernetes-sigs/yaml/blob/master/RELEASE.md)
- [Commits](kubernetes-sigs/yaml@v1.2.0...v1.3.0)

---
updated-dependencies:
- dependency-name: sigs.k8s.io/yaml
  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
2 participants