refactor(master): allow ACL ops in KV backends

[lgsilva3087]

Thread FilesystemOperationContext through all ACL methods (deleteAcl, setAcl, getAcl, applySetAcl, applySetRichAcl) in both IFilesystemNodeOperations and IFilesystemOperations, removing the internal transaction creation that was hardcoded inside each FilesystemOperationsBase method body.

Callers (matoclserv, restore) now own the transaction lifecycle, enabling KV backends to participate in ACL operations as part of a single coordinated transaction.

Introduce a protected virtual getAclForAccess() method to abstract ACL retrieval inside access(). The default in-memory implementation reads from aclStorage; KV backends can override it to materialise the ACL from the key-value store.

Pass FilesystemOperationContext through access() at all call sites so the new virtual hook receives the context it needs. A caller-supplied RichACL scratch buffer is provided so temporary ACLs can be returned without extra allocation.

The in-memory base implementation is not affected by the changes.

Also document all affected interface declarations according to their implementations.

Related to: LS-70

[gemini-code-assist]

Summary of Changes

Hello @lgsilva3087, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the Access Control List (ACL) management within the master service to improve its compatibility and integration with Key-Value (KV) backends. The core change involves centralizing transaction management by passing a FilesystemOperationContext through all ACL-related methods, thereby allowing external callers to control the transaction lifecycle. This architectural shift facilitates more robust and consistent ACL operations, especially for systems utilizing KV storage, without altering the behavior of the in-memory base implementation.

Highlights

• ACL Operations Refactored for KV Backends: All ACL-related methods (deleteAcl, setAcl, getAcl, applySetAcl, applySetRichAcl) in both IFilesystemNodeOperations and IFilesystemOperations now accept a FilesystemOperationContext. This change removes hardcoded internal transaction creation, enabling KV backends to participate in ACL operations within a single, coordinated transaction.
• Caller-Owned Transaction Lifecycle: Components like matoclserv and restore are now responsible for managing the transaction lifecycle for ACL operations, ensuring that these operations are part of a unified transaction context.
• Introduced Virtual getAclForAccess() Method: A new protected virtual method, getAclForAccess(), has been added to FilesystemNodeOperationsBase. This abstracts ACL retrieval within the access() method, allowing KV backends to override it and materialize ACLs directly from the key-value store.
• access() Method Enhancements: The access() method now passes FilesystemOperationContext and utilizes a caller-supplied RichACL scratch buffer. This allows for temporary ACLs to be returned without additional memory allocations, improving efficiency.
• Updated Interface Documentation: All affected interface declarations have been thoroughly documented with detailed Doxygen comments, clarifying their purpose, parameters, and return values in light of these changes.

Changelog

• src/master/filesystem_node.cc
  • Modified deleteAcl, getAcl, and setAcl methods to accept FilesystemOperationContext.
  • Introduced the getAclForAccess virtual method.
  • Updated the access method to use FilesystemOperationContext and the new getAclForAccess method.
  • Adjusted getNodeForOperation to pass fsOpContext to the access method.
• src/master/filesystem_node.h
  • Updated declarations for setAcl, getAcl, and deleteAcl to include FilesystemOperationContext.
  • Added the declaration for the getAclForAccess virtual method.
  • Provided detailed Doxygen comments for ACL-related method declarations.
• src/master/filesystem_node_operations_interface.h
  • Updated interface declarations for setAcl, getAcl, and deleteAcl to incorporate FilesystemOperationContext.
  • Added comprehensive Doxygen documentation for all ACL operation interfaces.
• src/master/filesystem_operations.cc
  • Modified setAttr and openCheck to pass fsOpContext to nodeOperations_->access.
  • Refactored deleteAcl, setAcl, getAcl, applySetAcl, and applySetRichAcl to remove internal transaction creation and accept FilesystemOperationContext.
  • Ensured proper transaction commit logic for write operations within deleteAcl, setAcl, applySetAcl, and applySetRichAcl.
• src/master/filesystem_operations.h
  • Updated declarations for deleteAcl, setAcl, getAcl, applySetAcl, and applySetRichAcl to include FilesystemOperationContext.
  • Added detailed Doxygen comments for these method declarations.
• src/master/kv_common_keys.h
  • Updated the comment for kACLsKeyPrefix to describe its format and purpose for efficient range queries.
• src/master/matoclserv.cc
  • Modified matoclserv_fuse_deleteacl, matoclserv_fuse_getacl, and matoclserv_fuse_setacl to create and manage FilesystemOperationContext and commit transactions for write operations.
  • Updated matoclserv_fuse_getacl to pass fsOpContext to idToNode.
• src/master/recursive_remove_task.cc
  • Updated retrieveNodes to pass fsOpContext to nodeOperations_->access.
• src/master/restore.cc
  • Modified do_deleteacl, do_setacl, and do_setrichacl to create and manage FilesystemOperationContext and commit transactions for write operations during the restore process.

Activity

• The author lgsilva3087 has refactored ACL operations to pass FilesystemOperationContext through relevant methods, enabling external transaction management.
• Internal transaction creation logic has been removed from FilesystemOperationsBase methods and moved to callers like matoclserv and restore.
• A new virtual method getAclForAccess was introduced to abstract ACL retrieval for KV backends.
• The access method was updated to accept FilesystemOperationContext and use a scratch buffer for ACLs.
• Extensive documentation has been added to interface declarations to reflect the changes.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request is a well-executed refactoring that allows ACL operations to work with transactional backends like KV stores. It achieves this by threading FilesystemOperationContext through the call stack, moving transaction lifecycle management to the callers (matoclserv, restore), which is a solid design improvement. The introduction of getAclForAccess provides a clean abstraction for ACL retrieval. Additionally, the documentation for the affected interfaces has been significantly improved. The changes are consistent and well-implemented. I have found one minor documentation issue to address.

[Copilot]

Pull request overview

This PR refactors ACL operations in the master filesystem to enable KV backend participation by threading FilesystemOperationContext through all ACL methods. Previously, ACL operations created transactions internally, preventing KV backends from coordinating ACL updates with other metadata changes in a single transaction.

Changes:

• Thread FilesystemOperationContext through all ACL methods (deleteAcl, setAcl, getAcl, applySetAcl, applySetRichAcl) in both IFilesystemNodeOperations and IFilesystemOperations interfaces
• Move transaction lifecycle management from internal method bodies to callers (matoclserv, restore), enabling coordinated transactions for KV backends
• Introduce protected virtual getAclForAccess() method to abstract ACL retrieval in access(), allowing KV backends to override and materialize ACLs from key-value storage

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/master/restore.cc	Add transaction creation and commit logic to ACL restore functions (do_deleteacl, do_setacl, do_setrichacl); improved unused parameter comment formatting
src/master/recursive_remove_task.cc	Pass fsOpContext to access() method for consistency with new signature
src/master/matoclserv.cc	Add transaction creation and commit logic to ACL client-facing operations (deleteacl, getacl, setacl); pass fsOpContext to idToNode call
src/master/kv_common_keys.h	Document ACL key prefix format for future KV backend implementation
src/master/filesystem_operations_interface.h	Add comprehensive documentation for all ACL methods; add fsOpContext parameter to all ACL method signatures
src/master/filesystem_operations.h	Update ACL method signatures with fsOpContext parameter; add documentation references
src/master/filesystem_operations.cc	Remove internal transaction creation from ACL methods; pass fsOpContext to node operations; remove unreachable code after return statements
src/master/filesystem_node_operations_interface.h	Add comprehensive documentation for ACL operations; add fsOpContext and access() parameters
src/master/filesystem_node.h	Update method signatures with fsOpContext; add protected getAclForAccess() virtual method; minor documentation reference fix needed
src/master/filesystem_node.cc	Mark fsOpContext as [[maybe_unused]] in base implementation; implement getAclForAccess() using in-memory aclStorage; pass fsOpContext through access() to support KV backend overrides

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

[rolysr]

LGTM. Please check my suggestions.

[dmga44]

LGTM, please check my minor suggestion.

[ralcolea]

Excellent work @lgsilva3087!! 👍 💪 🚀
I really liked the new documentation!! 🔥