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

Fix race condition in config handling (v8) #2931

Merged
merged 5 commits into from
May 22, 2024

Conversation

f-blass
Copy link
Contributor

@f-blass f-blass commented May 17, 2024

Description of the Change

The CLI encounters race conditions when handling config files if multiple commands are executed in parallel.

Currently, the CLI writes a new config for every command executed due to the code in command_parser.go. However, it also deletes all temporary config files when reading the config, leading to race conditions between multiple processes. This PR introduces two changes to mitigate this problem:

  1. Ignore "File Not Found" errors when attempting to delete a file. This change addresses the issue where multiple processes list and delete temporary files simultaneously.
  2. Only delete temp config files older than 5 minutes. This change resolves the problem where one process creates a temporary config that it later wants to rename, while a second process deletes this temp config in the meantime. Orphan files should be rare, as they are normally pruned by the signal handler, as seen in
    func catchSignal(sig chan os.Signal, tempConfigFileName string) {
    <-sig
    _ = os.Remove(tempConfigFileName)
    os.Exit(2)

Why Is This PR Valuable?

Enables usage of the CF CLI in scripts which execute commands in parallel

Applicable Issues

fixes #2232

How Urgent Is The Change?

Medium

Other Relevant Parties

Anyone using the CLI in scripts with parallel execution. Multiple users have encountered this issue, as discussed in #2232.

Related PRs

Copy link

linux-foundation-easycla bot commented May 17, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@gururajsh
Copy link
Member

Hi @f-blass,

Thank you for your contribution. Looks like some tests need to be addressed as part of this fix. Can you please look into it.

@f-blass
Copy link
Contributor Author

f-blass commented May 17, 2024

Hi @gururajsh
thank you! Should be fixed now.

Copy link
Member

@gururajsh gururajsh left a comment

Choose a reason for hiding this comment

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

Changes look good. Can you please create PRs for main and v7 branches as well? Asking for v7, since its a fix for an issue.

@f-blass f-blass changed the title Fix race condition in config handling Fix race condition in config handling (v8) May 21, 2024
@f-blass
Copy link
Contributor Author

f-blass commented May 21, 2024

Hi @gururajsh
thank you. PRs for v7 and main branch are created and linked.

Copy link
Member

@gururajsh gururajsh left a comment

Choose a reason for hiding this comment

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

LGTM

@gururajsh gururajsh merged commit 9f67c62 into cloudfoundry:v8 May 22, 2024
12 checks passed
@f-blass f-blass deleted the fix-config-race-cond branch May 22, 2024 14:32
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.

2 participants