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

kvs: remove fence support #6592

Merged
merged 5 commits into from
Jan 31, 2025

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Jan 30, 2025

Built upon the cleanups in #6591

Per discussion in #6112, remove KVS fence support.

  • it was only used by the shell's pmi "native" mode
  • has DoS possibilities
  • fixing the DoS possibilities proved annoying / troublesome
  • a better future implementation (if ever needed) should be a separate module / service that can handle DoS and support something far better / smarter

Note that with this removal some refactoring of the KVS can happen, but I will leave that for a follow up PR. Making this simply the removal and nothing else.

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

(reviewing the parts that are not in #6591)
Nice, this'll help make the code more maintainable.

chu11 added 5 commits January 30, 2025 17:18
Problem: In the near future the KVS fence implementation will be
removed from the KVS.  The only remaining user is the shell PMI
plugin and its "native" KVS option.

Solution: Remove all support for the native KVS in the shell pmi
plugin.  Remove associated documentation and tests.
Problem: In the near future the KVS fence implementation will be
removed.  So all KVS fence related tests will no longer be relevant.

Solution: Remove all KVS fence related tests.
Problem: In the near future the KVS fence implementation will be
removed.  The flux_kvs_fence() function will no longer be applicable.

Solution: Remove flux_kvs_fence().
Problem: The flux_kvs_fence(3) function has been removed but
is still referenced in documentation.

Remove all references to flux_kvs_fence(3).
Problem: The KVS fence implementation is virtually unused.  It also
has the potential for denial-of-service attacks.  Its implementation
is also quite "one off" within the KVS, limiting some ability to refactor
the code.  For all these reasons, it should be removed.  If fence-like
behavior is ever needed again in the future, it should be implemented
as a stand alone module/service on top of the KVS.

Solution: Remove all fence support in the KVS.

Fixes flux-framework#6587
@chu11 chu11 force-pushed the issue6587_kvs_fence_remove branch from 044238c to 5318a8d Compare January 31, 2025 01:19
@mergify mergify bot merged commit ace8d8a into flux-framework:master Jan 31, 2025
35 checks passed
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.44%. Comparing base (88092cd) to head (5318a8d).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6592      +/-   ##
==========================================
- Coverage   79.46%   79.44%   -0.02%     
==========================================
  Files         531      531              
  Lines       88433    88202     -231     
==========================================
- Hits        70275    70075     -200     
+ Misses      18158    18127      -31     
Files with missing lines Coverage Δ
src/common/libkvs/kvs_commit.c 73.23% <100.00%> (+5.56%) ⬆️
src/modules/kvs/kvs.c 72.35% <ø> (-0.08%) ⬇️
src/shell/pmi/pmi.c 83.80% <100.00%> (+2.20%) ⬆️

... and 11 files with indirect coverage changes

@chu11 chu11 deleted the issue6587_kvs_fence_remove branch February 1, 2025 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants