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 write_to_file function #336

Merged
merged 6 commits into from
Oct 17, 2024

Conversation

gabrielcocenza
Copy link
Member

@gabrielcocenza gabrielcocenza commented Oct 11, 2024

Probably because of the usage of low level functions, as for example os.open and os.fdopen, it's harder to deal with buffering which can result into files with inconsistent content, which breaks the configuration and the hardware-exporter service.

After adding using high level functions and make the code simpler, the bug seems to be fixed

Closes: #305

Probably, because of a racing condition, sometimes the file was
written into inconsistent state which breaks the configuration and
the hardware-exporter service.

After adding a file lock and detaching the permission from writting,
the bug seems to be fixed

Closes: canonical#305
Copy link
Contributor

@jneo8 jneo8 left a comment

Choose a reason for hiding this comment

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

Thanks, overall LGTM. Need more information on os.chmod part.

src/service.py Outdated Show resolved Hide resolved
Copy link
Contributor

@samuelallan72 samuelallan72 left a comment

Choose a reason for hiding this comment

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

Probably, because of a racing condition,

I feel like if we have race conditions where code is trying to write to the same file at the same time, then we should fix the source, not just add file locking. It may require a refactor to only write out the config from one place. It would be nice to do some more debugging and find the source of the race condition here.

src/service.py Outdated Show resolved Hide resolved
jneo8
jneo8 previously approved these changes Oct 15, 2024
src/service.py Outdated Show resolved Hide resolved
@gabrielcocenza
Copy link
Member Author

I feel like if we have race conditions where code is trying to write to the same file at the same time, then we should fix the source, not just add file locking. It may require a refactor to only write out the config from one place. It would be nice to do some more debugging and find the source of the race condition here.

I tested again and removing the lock file, but keeping the implementation seems to solve the issue. I didn't see in the logs more than one hook triggering the write_to_file function.

I'm not sure about the root cause. My guess is that os.open is a low-level system call. Maybe when you wrap the file descriptor in os.fdopen(), you're attempting to treat a low-level file descriptor as a higher-level file object, but subtle issues can occur if the file descriptor isn't handled perfectly (e.g., file not properly synced, buffers not flushed).

I believe that keeping things simple and using more high level python functions are enough to solve the issue

samuelallan72
samuelallan72 previously approved these changes Oct 15, 2024
Copy link
Contributor

@samuelallan72 samuelallan72 left a comment

Choose a reason for hiding this comment

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

Thanks for experimenting! It's nice that we can go with something even simpler than the previous implementation and solve the bug at the same time. 👍

src/service.py Outdated Show resolved Hide resolved
Deezzir
Deezzir previously approved these changes Oct 16, 2024
Copy link
Contributor

@Deezzir Deezzir left a comment

Choose a reason for hiding this comment

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

Legit

aieri
aieri previously approved these changes Oct 16, 2024
src/service.py Outdated Show resolved Hide resolved
tests/unit/test_service.py Outdated Show resolved Hide resolved
src/service.py Outdated Show resolved Hide resolved
tests/unit/test_service.py Show resolved Hide resolved
@gabrielcocenza gabrielcocenza merged commit c4f807b into canonical:main Oct 17, 2024
10 checks passed
@gabrielcocenza gabrielcocenza deleted the write-file-SOLENG-664 branch October 17, 2024 18:22
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.

hardware exporter config not rendered correctly
5 participants