Skip to content

Fix decrement of FH gauge for RW handles that try to write to existing files #716

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 25, 2024
Merged

Fix decrement of FH gauge for RW handles that try to write to existing files #716

merged 1 commit into from
Jan 25, 2024

Conversation

dannycjones
Copy link
Contributor

Description of change

In the logs, I was seeing that the gauge for unassigned file handles was not being decremented properly and increased over time with each attempt to write to a RW file handle where the file already exists.

For example:

echo 'hello-world' > /mnt/s3/existing-file.txt

2024-01-23T15:44:32.700092Z  INFO mountpoint_s3::metrics: fs.current_handles[type=unassigned]: 2
2024-01-23T15:44:37.700419Z  INFO mountpoint_s3::metrics: fs.current_handles[type=unassigned]: 2
2024-01-23T15:44:42.700741Z  INFO mountpoint_s3::metrics: fs.current_handles[type=unassigned]: 2

This change quickly fixes the issue.

In future, we may consider treating file handles more like a state machine which manages its own gauges and reduces the amount of places where we have to think about incrementing and decrementing these gauges.

Does this change impact existing behavior?

No breaking change. Fixes a counter.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

@dannycjones
Fix decrement of file handle gauge for RW handles that file on existi…
3ca1b99 
…ng files

Co-authored-by: Monthon Klongklaew <monthonk@amazon.com>
Co-authored-by: Alessandro Passaro <alexpax@amazon.co.uk>

Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com>
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 14:46 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 14:46 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 14:46 — with GitHub Actions Inactive
@dannycjones dannycjones enabled auto-merge January 25, 2024 14:46
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 14:46 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 14:46 — with GitHub Actions Inactive
dannycjones
dannycjones commented Jan 25, 2024
View reviewed changes
mountpoint-s3/src/fs.rs
Comment on lines 1171 to +1172
metrics::decrement_gauge!("fs.current_handles", 1.0, "type" => "read");
file_handle.inode.finish_reading()?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Swapped these two lines since we have a small concern that in the future, finish_reading could fail and we wouldn't decrement the gauge. It currently doesn't do anything though.

passaro
passaro approved these changes Jan 25, 2024
View reviewed changes
Copy link
Contributor

@passaro passaro left a comment

Choose a reason for hiding this comment

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

LGTM!

@dannycjones dannycjones added this pull request to the merge queue Jan 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 25, 2024
arsh
arsh approved these changes Jan 25, 2024
View reviewed changes
Copy link
Contributor

@arsh arsh left a comment

Choose a reason for hiding this comment

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

It would have been nice to see the before and after in your description of the change.

@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 16:18 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 16:18 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 16:18 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 16:18 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 16:18 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 18:57 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 18:57 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 18:57 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 18:57 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 18:57 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 18:57 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests January 25, 2024 18:57 — with GitHub Actions Inactive
@dannycjones dannycjones added this pull request to the merge queue Jan 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 25, 2024
@jamesbornholt jamesbornholt added this pull request to the merge queue Jan 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 25, 2024
@jamesbornholt jamesbornholt added this pull request to the merge queue Jan 25, 2024
Merged via the queue into awslabs:main with commit 85c98fa Jan 25, 2024
@dannycjones
Copy link
Contributor Author

It would have been nice to see the before and after in your description of the change.
#716 (review)

Makes sense, the after is no metric being emitted. Gauges aren't emitted as metrics when they have the value zero.

@dannycjones dannycjones deleted the fix-fh-count branch January 26, 2024 13:38
github-merge-queue bot pushed a commit that referenced this pull request Apr 1, 2025
@passaro
Update CRT submodules to latest releases (#1338)
c52ab15 
Update the CRT libraries to the latest releases. Notable changes:
* Update endpoints.
([awslabs/aws-c-s3#502](awslabs/aws-c-s3#502))
* Bump Default Memory Limit for Higher Target Throughput.
([awslabs/aws-c-s3#499](awslabs/aws-c-s3#499))


<details>
  <summary>Full CRT changelog:</summary>
  
```
Submodule mountpoint-s3-crt-sys/crt/aws-c-auth 01dd06ac..cd9d6afc:
  > Update docs for DefaultChain (#266)
  > Async cognito support (#267)
  > only forbid `X-Amz-S3session-Token` when signing with s3 express. (#268)
Submodule mountpoint-s3-crt-sys/crt/aws-c-cal 5d5bc553..4805a96e:
  > Fix FindCrypto behavior on win (#211)
  > Fix module export to respect ed25519 flag (#210)
  > Fix missed define in the code (#209)
Submodule mountpoint-s3-crt-sys/crt/aws-c-common 7fb0071a..8ae8f48e:
  > Simplify how inline math files are included (#1195)
  > Tests require compiler extensions (#1193)
  > CrossProcess lock -- don't unlock, just close fd (#1192)
Submodule mountpoint-s3-crt-sys/crt/aws-c-http 60c43f80..e526ac33:
  > Apple Network Framework Support (#502)
  > h1_decoder error on multiple content-length headers (#509)
  > Fix Error Handling For Connection Manager (#507)
  > HTTP/1: Support streaming requests of unknown length (#506)
Submodule mountpoint-s3-crt-sys/crt/aws-c-io 318f7e57..6c90e491:
  > Remove unused variables in aws_host_resolver (#719)
  > Grand dispatch queue (#661)
  > Fix IP address being labelled "bad" for too long (#718)
  > Add back kqueue support on iOS (#716)
Submodule mountpoint-s3-crt-sys/crt/aws-c-s3 1d0091c7..408e9c90:
  > Update endpoints (#502)
  > Newer URL for aws-lc (#500)
  > Bump Default Memory Limit for Higher Target Throughput (#499)
  > missed one file from test helper README (#498)
Submodule mountpoint-s3-crt-sys/crt/aws-checksums fb8bd0b8..66b447c0:
  > Add missing extern c to new header (#103)
  > Add init functions to support thread safe init of impls (#102)
Submodule mountpoint-s3-crt-sys/crt/aws-lc 7bca7e96..b1420f27:
  > Prepare for v1.49.1 (#2303)
  > Turn on better logging for EC2 test framework (#2298)
  > Add req to OpenSSL CLI tool (#2284)
  > Add more build options to match callback build (#2279)
  > FIPS Integrity Hash Tooling (#2296)
  > Prepare for v1.49.0 (#2297)
  > Cherrypick hardening DSA param checks from BoringSSL  (#2293)
  > Bump mysql CI to 9.2.0 (#2161)
  > AES: Add function pointer trampoline to avoid delocator issue (#2294)
  > Adding detection of out-of-bound pre-bound memory read to AES-XTS tests. (#2286)
  > Wire-up rust-openssl into GitHub CI (for the time being) (#2291)
  > Add support for more SSL BIO functions (#2273)
  > Add support for verifying PKCS7 signed attributes (#2264)
  > Reject DSA trailing garbage in EVP layer, add test cases (#2289)
  > Update patches in Ruby CI (#2233)
  > Documentation on service indicator (#2281)
  > Add the rehash utility to the openssl CLI tool (#2258)
  > Revert "Allow constructed strings in BER parsing (#2015)" (#2278)
  > Prepare AWS-LC v1.48.5 (#2274)
  > s2n-bignum build should use boringssl_prefix_symbols_asm.h (#2265)
  > ci: Nix flake and devShell (#2189)
  > GitHub CI job to verify tags are on expected branches (#2170)
  > Prepare for release v.1.48.4 (#2271)
  > Make AWS_LC_fips_failure_callback optional in builds with AWSLC_FIPS_FAILURE_CALLBACK (#2266)
  > Prepare v1.48.3 (#2269)
  > Update shard_gtest to unset environment variables once all the tests are done (#2270)
  > Minor build fixes for CMake and libssl on x86 (#2267)
  > Fix aws-lc-rs CI test (again) (#2268)
  > Add x4 batched SHAKE128 and SHAKE256 APIs (#2247)
  > Fix aws-lc-rs CI test when symbols removed (#2262)
  > Remove no-op register move from ChaCha20_ctr32_ssse3_4x (#2234)
  > Revert removal of "PEM_X509_INFO_write_bio" (#2226)
  > Use unsigned return type for BN_get_minimal_width and word size tests (#2260)
```
</details>

### Does this change impact existing behavior?

No.

### Does this change need a changelog entry? Does it require a version
change?

Yes. Updated as required.

---

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and I agree to the terms of
the [Developer Certificate of Origin
(DCO)](https://developercertificate.org/).

---------

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
mansi153 pushed a commit to mansi153/mountpoint-s3 that referenced this pull request Jul 24, 2025
@passaro @mansi153
Update CRT submodules to latest releases (awslabs#1338)
ae06df9 
Update the CRT libraries to the latest releases. Notable changes:
* Update endpoints.
([awslabs/aws-c-s3#502](awslabs/aws-c-s3#502))
* Bump Default Memory Limit for Higher Target Throughput.
([awslabs/aws-c-s3#499](awslabs/aws-c-s3#499))


<details>
  <summary>Full CRT changelog:</summary>
  
```
Submodule mountpoint-s3-crt-sys/crt/aws-c-auth 01dd06ac..cd9d6afc:
  > Update docs for DefaultChain (awslabs#266)
  > Async cognito support (awslabs#267)
  > only forbid `X-Amz-S3session-Token` when signing with s3 express. (awslabs#268)
Submodule mountpoint-s3-crt-sys/crt/aws-c-cal 5d5bc553..4805a96e:
  > Fix FindCrypto behavior on win (awslabs#211)
  > Fix module export to respect ed25519 flag (awslabs#210)
  > Fix missed define in the code (awslabs#209)
Submodule mountpoint-s3-crt-sys/crt/aws-c-common 7fb0071a..8ae8f48e:
  > Simplify how inline math files are included (awslabs#1195)
  > Tests require compiler extensions (awslabs#1193)
  > CrossProcess lock -- don't unlock, just close fd (awslabs#1192)
Submodule mountpoint-s3-crt-sys/crt/aws-c-http 60c43f80..e526ac33:
  > Apple Network Framework Support (awslabs#502)
  > h1_decoder error on multiple content-length headers (awslabs#509)
  > Fix Error Handling For Connection Manager (awslabs#507)
  > HTTP/1: Support streaming requests of unknown length (awslabs#506)
Submodule mountpoint-s3-crt-sys/crt/aws-c-io 318f7e57..6c90e491:
  > Remove unused variables in aws_host_resolver (awslabs#719)
  > Grand dispatch queue (awslabs#661)
  > Fix IP address being labelled "bad" for too long (awslabs#718)
  > Add back kqueue support on iOS (awslabs#716)
Submodule mountpoint-s3-crt-sys/crt/aws-c-s3 1d0091c7..408e9c90:
  > Update endpoints (awslabs#502)
  > Newer URL for aws-lc (awslabs#500)
  > Bump Default Memory Limit for Higher Target Throughput (awslabs#499)
  > missed one file from test helper README (awslabs#498)
Submodule mountpoint-s3-crt-sys/crt/aws-checksums fb8bd0b8..66b447c0:
  > Add missing extern c to new header (awslabs#103)
  > Add init functions to support thread safe init of impls (awslabs#102)
Submodule mountpoint-s3-crt-sys/crt/aws-lc 7bca7e96..b1420f27:
  > Prepare for v1.49.1 (#2303)
  > Turn on better logging for EC2 test framework (#2298)
  > Add req to OpenSSL CLI tool (#2284)
  > Add more build options to match callback build (#2279)
  > FIPS Integrity Hash Tooling (#2296)
  > Prepare for v1.49.0 (#2297)
  > Cherrypick hardening DSA param checks from BoringSSL  (#2293)
  > Bump mysql CI to 9.2.0 (#2161)
  > AES: Add function pointer trampoline to avoid delocator issue (#2294)
  > Adding detection of out-of-bound pre-bound memory read to AES-XTS tests. (#2286)
  > Wire-up rust-openssl into GitHub CI (for the time being) (#2291)
  > Add support for more SSL BIO functions (#2273)
  > Add support for verifying PKCS7 signed attributes (#2264)
  > Reject DSA trailing garbage in EVP layer, add test cases (#2289)
  > Update patches in Ruby CI (#2233)
  > Documentation on service indicator (#2281)
  > Add the rehash utility to the openssl CLI tool (#2258)
  > Revert "Allow constructed strings in BER parsing (#2015)" (#2278)
  > Prepare AWS-LC v1.48.5 (#2274)
  > s2n-bignum build should use boringssl_prefix_symbols_asm.h (#2265)
  > ci: Nix flake and devShell (#2189)
  > GitHub CI job to verify tags are on expected branches (#2170)
  > Prepare for release v.1.48.4 (#2271)
  > Make AWS_LC_fips_failure_callback optional in builds with AWSLC_FIPS_FAILURE_CALLBACK (#2266)
  > Prepare v1.48.3 (#2269)
  > Update shard_gtest to unset environment variables once all the tests are done (#2270)
  > Minor build fixes for CMake and libssl on x86 (#2267)
  > Fix aws-lc-rs CI test (again) (#2268)
  > Add x4 batched SHAKE128 and SHAKE256 APIs (#2247)
  > Fix aws-lc-rs CI test when symbols removed (#2262)
  > Remove no-op register move from ChaCha20_ctr32_ssse3_4x (#2234)
  > Revert removal of "PEM_X509_INFO_write_bio" (#2226)
  > Use unsigned return type for BN_get_minimal_width and word size tests (#2260)
```
</details>

### Does this change impact existing behavior?

No.

### Does this change need a changelog entry? Does it require a version
change?

Yes. Updated as required.

---

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and I agree to the terms of
the [Developer Certificate of Origin
(DCO)](https://developercertificate.org/).

---------

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@passaro passaro passaro approved these changes

@monthonk monthonk Awaiting requested review from monthonk

+1 more reviewer

@arsh arsh arsh approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dannycjones @arsh @passaro @jamesbornholt