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

Check for and log information about stale osquery database lock files #2006

Merged
merged 19 commits into from
Dec 30, 2024

Conversation

RebeccaMahany
Copy link
Contributor

@RebeccaMahany RebeccaMahany commented Dec 17, 2024

Relates to #2004.

launcher is currently completely unable to recover if the osquery lock file is left behind. However, we want to be cautious about removing this file. For now, we detect whether there's a stale lock file, and collect + log information about it if so.

Also adds a test for launching an instance, plus a test for detecting lock file presence.

@RebeccaMahany RebeccaMahany added the features-improvements Features and Improvements label Dec 17, 2024
James-Pickett
James-Pickett previously approved these changes Dec 17, 2024
@RebeccaMahany RebeccaMahany marked this pull request as ready for review December 17, 2024 22:08
James-Pickett
James-Pickett previously approved these changes Dec 17, 2024
zackattack01
zackattack01 previously approved these changes Dec 17, 2024
@RebeccaMahany RebeccaMahany changed the title Check for and clean up old osquery database lock files before starting process Check for and log information about stale osquery database lock files Dec 18, 2024
@RebeccaMahany RebeccaMahany marked this pull request as draft December 18, 2024 19:20
@RebeccaMahany RebeccaMahany marked this pull request as ready for review December 19, 2024 16:06
James-Pickett
James-Pickett previously approved these changes Dec 19, 2024
@RebeccaMahany RebeccaMahany marked this pull request as ready for review December 19, 2024 18:16
James-Pickett
James-Pickett previously approved these changes Dec 19, 2024
@@ -80,3 +82,39 @@ func isExitOk(err error) bool {
}
return false
}

func getProcessesHoldingFile(ctx context.Context, pathToFile string) ([]*process.Process, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a fairly expensive call. Might be okay, given how infrequent it occurs. But we should know that it's not cheap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It takes about 6 seconds in test, which is definitely not nothing. But we'll only hit this if the lock file exists -- and in that case, we'll never successfully start up osquery anyway, so the delay is a moot point. I figure we can remove this once we have more info from the logs to figure out how to detect/remediate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah...

zackattack01
zackattack01 previously approved these changes Dec 20, 2024
@@ -33,3 +40,41 @@ func platformArgs() []string {
func isExitOk(_ error) bool {
return false
}

func getProcessesHoldingFile(ctx context.Context, pathToFile string) ([]*process.Process, error) {
cmd, err := allowedcmd.Lsof(ctx, "-t", pathToFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish we had a better option, but I concur we do not.

@@ -80,3 +82,39 @@ func isExitOk(err error) bool {
}
return false
}

func getProcessesHoldingFile(ctx context.Context, pathToFile string) ([]*process.Process, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah...

@RebeccaMahany RebeccaMahany added this pull request to the merge queue Dec 30, 2024
Merged via the queue into kolide:main with commit bb27062 Dec 30, 2024
32 checks passed
@RebeccaMahany RebeccaMahany deleted the becca/rm-old-db-lock branch December 30, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
features-improvements Features and Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants