Skip to content

Runtime-config: Handle absolute file paths when working directory is not / #6224

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

Merged
merged 5 commits into from
Sep 26, 2024

Conversation

CharlieTLe
Copy link
Member

@CharlieTLe CharlieTLe commented Sep 20, 2024

We started using a BucketClient for the filesystem where the default directory is an empty string:

f.StringVar(&cfg.Directory, prefix+"filesystem.dir", "", "Local filesystem storage directory.")

When we create the new bucket,

uses filepath.Abs("") which will use the current working directory as the root directory.

What this PR does:

Handles absolute file paths for the runtime config file when using the filesystem and the working directory is not /.

Which issue(s) this PR fixes:
Fixes: #6219

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CharlieTLe CharlieTLe changed the title Set default filesystem directory to "/" for runtime_config Set default filesystem directory to "/" for filesystem.dir Sep 21, 2024
@CharlieTLe CharlieTLe force-pushed the fix-default-root-path-for-runtime-config branch from a6e1d16 to 93a1668 Compare September 21, 2024 02:18
@CharlieTLe CharlieTLe requested a review from yeya24 September 21, 2024 02:18
We started using a BucketClient for the filesystem where the default directory is an empty string: https://github.com/cortexproject/cortex/blob/af9e20c54ee97f409008f3e86541c5dfa5038e22/pkg/storage/bucket/filesystem/config.go#L17

When we create the new bucket, https://github.com/cortexproject/cortex/blob/526a6d935a948119cf74033a9f79391786022222/vendor/github.com/thanos-io/objstore/providers/filesystem/filesystem.go#L46 uses `filepath.Abs("")` which will use the current working directory as the root directory.

Fixes: #6219

Signed-off-by: Charlie Le <[email protected]>
@CharlieTLe CharlieTLe force-pushed the fix-default-root-path-for-runtime-config branch from 93a1668 to 6dbccee Compare September 21, 2024 18:01
@CharlieTLe CharlieTLe changed the title Set default filesystem directory to "/" for filesystem.dir Add logic for handling filesystem dir with runtime config Sep 21, 2024
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks. Can we add changelog and test?

Allows for testing what happens when cortex is run from a directory that
is not the root directory.

Signed-off-by: Charlie Le <[email protected]>
@pull-request-size pull-request-size bot added size/M and removed size/S labels Sep 21, 2024
@CharlieTLe CharlieTLe changed the title Add logic for handling filesystem dir with runtime config Runtime-config: Handle absolute file paths when working directory is not / Sep 21, 2024
Signed-off-by: Charlie Le <[email protected]>
@CharlieTLe CharlieTLe force-pushed the fix-default-root-path-for-runtime-config branch from ff343f6 to ebd158f Compare September 21, 2024 23:56
@CharlieTLe CharlieTLe requested a review from yeya24 September 22, 2024 00:01
@yeya24
Copy link
Contributor

yeya24 commented Sep 25, 2024

@CharlieTLe Sorry for the late review, can we fix changelog?

@CharlieTLe
Copy link
Member Author

@yeya24 Updated

@CharlieTLe CharlieTLe merged commit 296fd47 into master Sep 26, 2024
16 checks passed
@friedrichg friedrichg deleted the fix-default-root-path-for-runtime-config branch January 15, 2025 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--runtime-config.file path behaviour is broken in in 1.18
2 participants