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

(KB20267) Add a task to clean up index locks #5

Merged
merged 2 commits into from
Jul 5, 2018
Merged

(KB20267) Add a task to clean up index locks #5

merged 2 commits into from
Jul 5, 2018

Conversation

jarretlavallee
Copy link
Contributor

Add a very simple task to find and delete index.lock files in the file sync cache directory. It doesn't have any conditional logic, so let me know if you want me to modify it.

This commit adds a new task to find and delete `index.lock` files in the
file sync directory and ensure that the puppetserver service is started.
@jarretlavallee jarretlavallee added the enhancement New feature or request label Jul 3, 2018
@abottchen
Copy link
Collaborator

Should we check to confirm that pe-puppetserver is not running before executing the task? My concern is customers running this task when they aren't having the problem and corrupting a normal deploy by deleting the locks. If pe-puppetserver is dead, we know a deploy cannot be running, thus avoiding this possibility.

@jarretlavallee
Copy link
Contributor Author

@abottchen Good idea. Should we stop the service as a part of the task or fail if the puppetserver service is running?

@abottchen
Copy link
Collaborator

If I'm understanding the bug properly, the failed sync kills the puppetserver. If that is true, then I think we should fail if it is running. since that would imply they are not hitting the bug and thus shouldn't be running this task.

However, if it is possible for puppetserver to still be running in spite of this having happened, then I think it makes sense to stop it at the beginning of this task to ensure no deploys happen during it.

@MartyEwings
Copy link
Collaborator

I agree I think we should stop the service. If the task is run in error and we remove a valid lock file we could get some weird results, but if we stop the service, there really shouldn't be a lock file unless you have hit this bug, so no harm done

Copy link
Collaborator

@MartyEwings MartyEwings left a comment

Choose a reason for hiding this comment

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

I think some logic to make sure we dont remove a valid lock file would be best. Maybe stopping the server just to be sure? or even checking for the offending message in the recent logs before proceeding

This commit stops the puppetserver service prior to removing locks on
the file sync cache.
@jarretlavallee
Copy link
Contributor Author

This change just blatantly stops the service, removes any locks and starts the service. I feel like it may be better to have some conditional logic and a force parameter, but tasks are kind of a pain. It would be nice to use puppet resources to get the status across all platforms without having to parse the DSL. Maybe it is easier to just use the puppet resources with an output in yaml. Let me know what you think of the blunt approach or if I should implement some more logic and parameters in it.

@MartyEwings
Copy link
Collaborator

i am a blunt change sort of guy, but echoing @abottchen comments maybe good foundations is what we need to go for here, id love to strike the balance but im also biased with getting this off the ground fast! :)

@abottchen
Copy link
Collaborator

I think this is fine. As long as we document the fact that it will stop the service so users know, we are good. There is still the overall question I have about sending output to stdout rather than structured json output, but I can save that for a larger conversation.

@MartyEwings MartyEwings merged commit a291d8d into puppetlabs:master Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants