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

Feature: finalizers #136

Merged
merged 15 commits into from
May 15, 2018
Merged

Feature: finalizers #136

merged 15 commits into from
May 15, 2018

Conversation

ewoutp
Copy link
Contributor

@ewoutp ewoutp commented May 11, 2018

fixes #143

This PR adds a finalizer to dbserver Pods and a preStop hook to prevent it from being deleted before all data is safe.
It also adds a finalizer to all PVCs to prevent them from being removed while their member still exists.

TODO:

  • Prevent orphaned resources
  • Add finalizer to agents to prevent removing to many at the same time
  • Run all tests

@ewoutp ewoutp changed the title Feature/pod finalizers Feature: finalizers May 11, 2018
@ewoutp ewoutp added the 9 WIP label May 11, 2018
Copy link
Member

@neunhoef neunhoef left a comment

Choose a reason for hiding this comment

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

LGTM. Note one question about the approach, which is almost certainly fine.


To achieve this, the server containers in the `Pods` have
a `preStop` hook configured and finalizers are added to the `Pods`
ands `PersistentVolumeClaims`.
Copy link
Member

Choose a reason for hiding this comment

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

"ands" -> "and"

if err := os.Chmod(targetPath, 0755); err != nil {
cliLog.Fatal().Err(err).Msg("Failed to chmod")
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Just for the record (one more time): LOL about this trick.

}
// Ensure the cleanout is triggered
log.Debug().Msg("Server is not yet clean out. Triggering a clean out now")
if err := cluster.CleanOutServer(ctx, memberStatus.ID); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I cannot immediately see how often this is called. It seems that as long as the cleanout is ongoing, a new cleanout is ordered every time this routine is called, which happens every time runPodFinalizers is run. I found a loop doing this but I could not immediately determine how fast this loop could turn. I do not think it is a real problem to call CleanOutServer again, but I have a bad feeling if many of these calls would be done in rapid succession, whilst it is trying to clean itself out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The interval depends on external events.
So while it is unlikely that these calls are made in rapid succession, it is possible.

@ewoutp ewoutp merged commit 4dcf8f8 into master May 15, 2018
@ewoutp ewoutp deleted the feature/pod-finalizers branch May 15, 2018 08:17
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.

2 participants