Skip to content

ZOOKEEPER-4846: Failure to reload database due to missing ACL - #2183

Closed
ztzg wants to merge 4 commits into
apache:masterfrom
ztzg:ZOOKEEPER-4846-load-database-missing-acl
Closed

ZOOKEEPER-4846: Failure to reload database due to missing ACL#2183
ztzg wants to merge 4 commits into
apache:masterfrom
ztzg:ZOOKEEPER-4846-load-database-missing-acl

Conversation

@ztzg

@ztzg ztzg commented Aug 28, 2024

Copy link
Copy Markdown
Contributor

ZooKeeper snapshots are fuzzy, as the server does not stop processing requests while ACLs and nodes are being streamed to disk.

ACLs, notably, are streamed first, as a mapping between the full serialized ACL and an "ACL ID" referenced by the node.

Consequently, a snapshot can very well contain ACL IDs which do not exist in the mapping. Prior to ZOOKEEPER-4799, such situations would produce harmless (if annoying) "Ignoring acl XYZ as it does not exist in the cache" INFO entries in the server logs.

With ZOOKEEPER-4799, we started "eagerly" fetching the referenced ACLs in DataTree operations such as createNode, deleteNode, etc.—as opposed to just fetching them from request processors.

This can result in fatal errors during the fastForwardFromEdits phase of restoring a database, when transactions are processed on top of an inconsistent data tree—preventing the server from starting.

The errors are thrown in this code path:

// ReferenceCountedACLCache.java:90
List<ACL> acls = longKeyMap.get(longVal);
if (acls == null) {
    LOG.error("ERROR: ACL not available for long {}", longVal);
    throw new RuntimeException("Failed to fetch acls for " + longVal);
}

Here is a scenario leading to such a failure:

  • An existing node /foo, sporting an unique ACL, is deleted. This is recorded in transaction log $SNAP-1; said ACL is also deallocated;
  • Snapshot $SNAP is started;
  • The ACL map is serialized to $SNAP;
  • A new node /foo sporting the same unique ACL is created in a portion of the data tree which still has to be serialized;
  • Node /foo is serialized to $SNAP—but its ACL isn't;
  • The server is restarted;
  • The DataTree is initialized from $SNAP, including node /foo with a dangling ACL reference;
  • Transaction log $SNAP-1 is being replayed, leading to a deleteNode("/foo");
  • getACL(node) panics.

@anmolnar

anmolnar commented Feb 5, 2025

Copy link
Copy Markdown
Contributor

hi @ztzg , are u still working on this patch?

@ztzg

ztzg commented Feb 6, 2025

Copy link
Copy Markdown
Contributor Author

Hi @anmolnar,

hi @ztzg , are u still working on this patch?

We have been using it for a while without adverse effects. I am not working on it, but some decisions in the patch are perhaps a bit controversial. I will "undraft" if it helps getting more reviews & comments :)

@ztzg
ztzg marked this pull request as ready for review February 6, 2025 10:18

@cnauroth cnauroth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just confirming my own understanding, it seems the current patch avoids the failure but does not preserve the ACL that was originally set on /foo. The ACL is removed.

@anmolnar

anmolnar commented Feb 6, 2025

Copy link
Copy Markdown
Contributor

@anmolnar

anmolnar commented Feb 7, 2025

Copy link
Copy Markdown
Contributor

Apols @ztzg , I quickly created the alternate patch to get feedback as soon as possible: #2222
Don't wanna take the credit, so we should commit as yours eventually once we have agreement.

@anmolnar

Copy link
Copy Markdown
Contributor

Superseded by #2222

@anmolnar anmolnar closed this Feb 11, 2025
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.

3 participants