[filestorage]: Provide an option to the user to create a directory #34985

VihasMakwana · 2024-09-03T16:46:51Z

Description:
This PR introduces a new option that will create a directory for the user if it doesn't already exist.

Link to tracking Issue: #34939

Testing: Added

atoulme · 2024-09-03T16:54:51Z

extension/storage/filestorage/README.md

@@ -24,6 +24,8 @@ The default timeout is `1s`.

 `fsync` when set, will force the database to perform an fsync after each write.  This helps to ensure database integrity if there is an interruption to the database process, but at the cost of performance.  See [DB.NoSync](https://pkg.go.dev/go.etcd.io/bbolt#DB) for more information.

+`create_directory` when set, will create the data storage directory if it does not already exist. The directory will be created with `0755 (rwxr-x--)` permissions.


Can we make permissions configurable?

Did that. PTAL

djaglowski · 2024-09-04T13:29:03Z

Failing windows test is unrelated. See #35002

VihasMakwana · 2024-09-04T15:15:25Z

There's another unrelated failure due to a dead link. It reports

https://linux.die.net/man/1/strings → Status: 403

the link works fine on my browser though

extension/storage/filestorage/config.go

andrzej-stencel · 2024-09-05T13:08:40Z

extension/storage/filestorage/config.go

@@ -65,7 +65,7 @@ func (cfg *Config) Validate() error {
 	for _, dir := range dirs {
 		if info, err := os.Stat(dir); err != nil {
 			if !cfg.CreateDirectory && os.IsNotExist(err) {
-				return fmt.Errorf("directory must exist: %w", err)
+				return fmt.Errorf("directory must exist: %w. You can enable the create_directory option to automatically create it.", err)


Actually this whole logic is used not only for the directory configuration option, but also for the compaction::directory option. Only the validation of the directory option should be modified. The validation for compaction::directory setting should be unchanged.

@andrzej-stencel we also create the compaction directory, if it doesn't exist. See here

opentelemetry-collector-contrib/extension/storage/filestorage/extension.go

Lines 30 to 47 in 49f8015

if config.CreateDirectory {

var dirs []string

if config.Compaction.OnStart {

dirs = []string{config.Directory, config.Compaction.Directory}

} else {

dirs = []string{config.Directory}

}

perm, err := strconv.ParseInt(config.DirectoryPermissions, 8, 32)

if err != nil {

logger.Debug("Error while parsing permissions, switching to default mode (0750)", zap.String("DirectoryPerms", config.DirectoryPermissions))

perm = 0750

}

for _, dir := range dirs {

if err := createDirectory(dir, os.FileMode(perm)); err != nil {

return nil, err

}

}

}

On a side note, I've updated docs to mention the same

create_directory when set, will create the data storage and compaction directory if it does not already exist. The directory will be created with 0750 (rwxr-x--) permissions, by default. Use directory_permissions to customize directory creation permissions.

Oh, missed that, thanks. I suppose that's OK, thanks for updating the docs.

I see the following issue in this logic though. The compaction directory is only created when compaction::on_start is enabled, but not when compaction::on_rebound is enabled. This means that if I use the following configuration:

extensions: file_storage: compaction: directory: non-existent-compaction-dir on_rebound: true create_directory: true directory: non-existent-storage-dir

then the collector will not create the compaction directory non-existent-compaction-dir either on start or later, when rebound compaction is being performed, resulting in an error trying to compact.

I think ideally in this case the compaction directory should be created not on start, but only if/when needed, if/when the rebound compaction is actually performed.

Alternatively, only have the create_directory option affect the root directory option, leaving space for creating a separate compaction::create_directory option controlling the creation of the compaction directory. I would be fine with either approach.

I see the following issue in this logic though. The compaction directory is only created when compaction::on_start is enabled, but not when compaction::on_rebound is enabled.

@andrzej-stencel I filed #35114 for this.

As per my understanding, we should check for compaction directory when either of the options (on_start or on_rebound) are enabled

@andrzej-stencel with #35114 merged, we now create compaction directory on rebound or start.

I think ideally in this case the compaction directory should be created not on start, but only if/when needed, if/when the rebound compaction is actually performed.

Rebound compaction takes place periodically, usually every 5 seconds by default. Checking the compaction directory with every iteration would be an inefficient. What do you think?

Rebound compaction takes place periodically, usually every 5 seconds by default.

This is inaccurate. The check takes place periodically, but the compaction is only triggered when the conditions are met.

But I suppose creating the directory on collector's start when rebound compaction is enabled is OK too.

Thank you for making these changes and for the tests Vihas! 🙏

I think one last change is needed, adding OnRebound in newLocalFileStorage function.

This is inaccurate. The check takes place periodically, but the compaction is only triggered when the conditions are met.

Yes, sorry I missed that.

extension/storage/filestorage/extension.go

extension/storage/filestorage/config.go

extension/storage/filestorage/extension.go

extension/storage/filestorage/config_test.go

andrzej-stencel · 2024-09-10T10:29:24Z

extension/storage/filestorage/config.go

@@ -65,7 +65,7 @@ func (cfg *Config) Validate() error {
 	for _, dir := range dirs {
 		if info, err := os.Stat(dir); err != nil {
 			if !cfg.CreateDirectory && os.IsNotExist(err) {
-				return fmt.Errorf("directory must exist: %w", err)
+				return fmt.Errorf("directory must exist: %w. You can enable the create_directory option to automatically create it.", err)


Oh, missed that, thanks. I suppose that's OK, thanks for updating the docs.

I see the following issue in this logic though. The compaction directory is only created when compaction::on_start is enabled, but not when compaction::on_rebound is enabled. This means that if I use the following configuration:

extensions: file_storage: compaction: directory: non-existent-compaction-dir on_rebound: true create_directory: true directory: non-existent-storage-dir

then the collector will not create the compaction directory non-existent-compaction-dir either on start or later, when rebound compaction is being performed, resulting in an error trying to compact.

I think ideally in this case the compaction directory should be created not on start, but only if/when needed, if/when the rebound compaction is actually performed.

Alternatively, only have the create_directory option affect the root directory option, leaving space for creating a separate compaction::create_directory option controlling the creation of the compaction directory. I would be fine with either approach.

Co-authored-by: Andrzej Stencel <[email protected]>

VihasMakwana · 2024-09-10T13:53:52Z

@andrzej-stencel @djaglowski I've added more test coverage. PTAL!

extension/storage/filestorage/extension.go

Co-authored-by: Andrzej Stencel <[email protected]>

VihasMakwana · 2024-09-11T15:06:08Z

extension/storage/filestorage/README.md

@@ -124,7 +127,7 @@ following troubleshooting method works for bbolt-managed files. As such, there i

 When troubleshooting components that use the File Storage extension, it is sometimes helpful to read the raw contents of
 files created by the extension for the component.  The simplest way to read files
-created by the File Storage extension is to use the strings utility ([Linux](https://linux.die.net/man/1/strings),
+created by the File Storage extension is to use the strings utility ([Linux](https://man7.org/linux/man-pages/man1/strings.1.html),


The check-links is failing, so I'm updating this just to see if it passes.
I'll raise another PR if that's more convenient.

Fixed via #35145

VihasMakwana · 2024-09-11T16:55:35Z

@djaglowski I think this is good to go. The prometheus test failure is a known issue.

…pen-telemetry#34985) **Description:** This PR introduces a new option that will create a directory for the user if it doesn't already exist. **Link to tracking Issue:** open-telemetry#34939 **Testing:** Added --------- Co-authored-by: Daniel Jaglowski <[email protected]> Co-authored-by: Andrzej Stencel <[email protected]>

VihasMakwana added 3 commits September 3, 2024 22:09

chore: add an option to create a directory

076ee3b

chore: doc and changelog

1bc4998

fix: use 0750 mode

9095bc3

VihasMakwana requested a review from djaglowski as a code owner September 3, 2024 16:46

VihasMakwana requested a review from a team September 3, 2024 16:46

github-actions bot assigned fatsheep9146 Sep 3, 2024

github-actions bot added the extension/storage label Sep 3, 2024

github-actions bot requested review from atoulme and dmitryax September 3, 2024 16:47

fix: move directory logic to CreateExtension

1d3c900

atoulme reviewed Sep 3, 2024

View reviewed changes

atoulme approved these changes Sep 3, 2024

View reviewed changes

djaglowski added the Run Windows Enable running windows test on a PR label Sep 3, 2024

VihasMakwana added 2 commits September 3, 2024 23:33

chore: customize directory permissions

0c9b570

chore: customize directory permissions docs tests

ed37d63

VihasMakwana requested a review from atoulme September 3, 2024 18:07

fix: checks and extension tests windows

02a1e14

Merge branch 'main' into filestorage-directory-enhancement

5b4ea61

andrzej-stencel reviewed Sep 5, 2024

View reviewed changes

extension/storage/filestorage/config.go Outdated Show resolved Hide resolved

andrzej-stencel reviewed Sep 5, 2024

View reviewed changes

extension/storage/filestorage/config.go Outdated Show resolved Hide resolved

VihasMakwana added 2 commits September 5, 2024 16:17

fix: improve naming

99f3630

chore: update error message

49f8015

VihasMakwana requested a review from andrzej-stencel September 5, 2024 10:53

andrzej-stencel reviewed Sep 5, 2024

View reviewed changes

extension/storage/filestorage/extension.go Outdated Show resolved Hide resolved

fix: update validate, docs, tests

2249df4

VihasMakwana requested a review from andrzej-stencel September 5, 2024 13:26

fix: lint

7061378

VihasMakwana and others added 3 commits September 6, 2024 18:25

fix: lint

c3491e3

fix: lint

b586af5

Merge branch 'main' into filestorage-directory-enhancement

5d6d4d6

andrzej-stencel reviewed Sep 10, 2024

View reviewed changes

VihasMakwana and others added 3 commits September 10, 2024 16:08

Update extension/storage/filestorage/config_test.go

0eff6ab

Co-authored-by: Andrzej Stencel <[email protected]>

chore: update tests

d8fdebb

chore: add more tests and fix some tests

bf87aef

VihasMakwana requested a review from andrzej-stencel September 10, 2024 13:55

VihasMakwana added 2 commits September 10, 2024 19:38

fix: lint

ec3cb55

Merge branch 'main' into filestorage-directory-enhancement

a0d4a32

andrzej-stencel reviewed Sep 11, 2024

View reviewed changes

extension/storage/filestorage/extension.go Outdated Show resolved Hide resolved

andrzej-stencel reviewed Sep 11, 2024

View reviewed changes

extension/storage/filestorage/extension.go Outdated Show resolved Hide resolved

VihasMakwana and others added 2 commits September 11, 2024 17:25

Merge branch 'main' into filestorage-directory-enhancement

5e9d98e

Update extension/storage/filestorage/extension.go

d407f36

Co-authored-by: Andrzej Stencel <[email protected]>

andrzej-stencel approved these changes Sep 11, 2024

View reviewed changes

fix: rename

86021ff

VihasMakwana mentioned this pull request Sep 11, 2024

[OTel]: enable persistence by default in our OTel distribution elastic/elastic-agent#5347

Closed

7 tasks

VihasMakwana force-pushed the filestorage-directory-enhancement branch from d4330c0 to 86021ff Compare September 11, 2024 15:04

fix: update failing link

740d390

VihasMakwana commented Sep 11, 2024

View reviewed changes

VihasMakwana mentioned this pull request Sep 11, 2024

[filestorage] - update man page for strings #35145

Merged

Merge branch 'main' into filestorage-directory-enhancement

a9ed772

djaglowski approved these changes Sep 11, 2024

View reviewed changes

djaglowski merged commit 0fb57cb into open-telemetry:main Sep 11, 2024
171 of 172 checks passed

github-actions bot added this to the next release milestone Sep 11, 2024

This was referenced Sep 12, 2024

[extension/filestorage] - Provide an option to the user to create a directory for them #34939

Closed

[chore][otel] Enable persistence in otel.yml elastic/elastic-agent#5549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[filestorage]: Provide an option to the user to create a directory #34985

[filestorage]: Provide an option to the user to create a directory #34985

VihasMakwana commented Sep 3, 2024

atoulme Sep 3, 2024

VihasMakwana Sep 3, 2024

djaglowski commented Sep 4, 2024

VihasMakwana commented Sep 4, 2024

andrzej-stencel Sep 5, 2024

VihasMakwana Sep 5, 2024

andrzej-stencel Sep 10, 2024

VihasMakwana Sep 10, 2024

VihasMakwana Sep 11, 2024 •

edited

Loading

andrzej-stencel Sep 11, 2024 •

edited

Loading

VihasMakwana Sep 11, 2024

andrzej-stencel Sep 10, 2024

VihasMakwana commented Sep 10, 2024

VihasMakwana Sep 11, 2024

VihasMakwana Sep 11, 2024

VihasMakwana commented Sep 11, 2024

		@@ -24,6 +24,8 @@ The default timeout is `1s`.

		`fsync` when set, will force the database to perform an fsync after each write. This helps to ensure database integrity if there is an interruption to the database process, but at the cost of performance. See [DB.NoSync](https://pkg.go.dev/go.etcd.io/bbolt#DB) for more information.

		`create_directory` when set, will create the data storage directory if it does not already exist. The directory will be created with `0755 (rwxr-x--)` permissions.

	if config.CreateDirectory {
	var dirs []string
	if config.Compaction.OnStart {
	dirs = []string{config.Directory, config.Compaction.Directory}
	} else {
	dirs = []string{config.Directory}
	}
	perm, err := strconv.ParseInt(config.DirectoryPermissions, 8, 32)
	if err != nil {
	logger.Debug("Error while parsing permissions, switching to default mode (0750)", zap.String("DirectoryPerms", config.DirectoryPermissions))
	perm = 0750
	}
	for _, dir := range dirs {
	if err := createDirectory(dir, os.FileMode(perm)); err != nil {
	return nil, err
	}
	}
	}

[filestorage]: Provide an option to the user to create a directory #34985

[filestorage]: Provide an option to the user to create a directory #34985

Conversation

VihasMakwana commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djaglowski commented Sep 4, 2024

VihasMakwana commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VihasMakwana Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

andrzej-stencel Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VihasMakwana commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VihasMakwana commented Sep 11, 2024

VihasMakwana Sep 11, 2024 •

edited

Loading

andrzej-stencel Sep 11, 2024 •

edited

Loading