Skip to content

ZOOKEEPER-4689: Fix ACL cache recovery from fuzzy snapshots #2425

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

Open
adamyi wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ZOOKEEPER-4689: Fix ACL cache recovery from fuzzy snapshots #2425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 50 additions & 18 deletions zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,11 @@ public class DataTree {

private static final Logger LOG = LoggerFactory.getLogger(DataTree.class);

// An unknown ACL must fail closed when filtering watch events. In
// particular, null and an empty list are treated as unrestricted ACLs.
private static final List<ACL> NO_PERMISSIONS_ACL =
Collections.singletonList(new ACL(0, ZooDefs.Ids.ANYONE_ID_UNSAFE));

private final RateLogger RATE_LOGGER = new RateLogger(LOG, 15 * 60 * 1000);

/**
Expand Down Expand Up @@ -441,24 +446,27 @@ public void createNode(final String path, byte[] data, List<ACL> acl, long ephem
}
List<ACL> parentAcl;
synchronized (parent) {
parentAcl = getACL(parent);
parentAcl = getACLForWatchTrigger(parent, parentName);

// Add the ACL to ACL cache first, to avoid the ACL not being
// created race condition during fuzzy snapshot sync.
//
// This is the simplest fix, which may add ACL reference count
// again if it's already counted in the ACL map of fuzzy
// snapshot, which might also happen for deleteNode txn, but
// at least it won't cause the ACL not exist issue.
//
// Later we can audit and delete all non-referenced ACLs from
// ACL map when loading the snapshot/txns from disk, like what
// we did for the global sessions.
Long acls = aclCache.convertAcls(acl);
Long aclId = aclCache.convertAcls(acl);

DataNode existingChild = nodes.get(path);
if (existingChild != null) {
existingChild.acl = acls;
// A create txn can be replayed after its node was already
// serialized into a fuzzy snapshot. The txn carries the
// authoritative ACL value, whereas the id in the snapshot may
// be absent from the serialized ACL cache. Move the node to the
// locally interned id and release its previous reference. This
// also balances convertAcls above when both ids are equal.
synchronized (existingChild) {
Long previousAcl = existingChild.acl;
existingChild.acl = aclId;
aclCache.removeUsage(previousAcl);
if (!aclId.equals(previousAcl)) {
LOG.info("ACL reference of existing node {} changed from {} to {} while applying a create txn",
path, previousAcl, aclId);
}
}
throw new NodeExistsException();
}

Expand All @@ -476,7 +484,7 @@ public void createNode(final String path, byte[] data, List<ACL> acl, long ephem
parent.stat.setCversion(parentCVersion);
parent.stat.setPzxid(zxid);
}
DataNode child = new DataNode(data, acls, stat);
DataNode child = new DataNode(data, aclId, stat);
parent.addChild(childName);
nodes.postChange(parentName, parent);
nodeDataSize.addAndGet(getNodeSize(path, child.data));
Expand Down Expand Up @@ -562,7 +570,7 @@ public void deleteNode(String path, long zxid) throws NoNodeException {
List<ACL> acl;
nodes.remove(path);
synchronized (node) {
acl = getACL(node);
acl = getACLForWatchTrigger(node, path);
aclCache.removeUsage(node.acl);
nodeDataSize.addAndGet(-getNodeSize(path, node.data));
}
Expand All @@ -572,7 +580,7 @@ public void deleteNode(String path, long zxid) throws NoNodeException {
// separate patch.
List<ACL> parentAcl;
synchronized (parent) {
parentAcl = getACL(parent);
parentAcl = getACLForWatchTrigger(parent, parentName);
long owner = node.stat.getEphemeralOwner();
EphemeralType ephemeralType = EphemeralType.get(owner);
if (ephemeralType == EphemeralType.CONTAINER) {
Expand Down Expand Up @@ -635,7 +643,7 @@ public Stat setData(String path, byte[] data, int version, long zxid, long time)
List<ACL> acl;
byte[] lastData;
synchronized (n) {
acl = getACL(n);
acl = getACLForWatchTrigger(n, path);
lastData = n.data;
nodes.preChange(path, n);
n.data = data;
Expand Down Expand Up @@ -792,6 +800,30 @@ public List<ACL> getACL(DataNode node) {
}
}

/**
* Returns the ACL used to filter watch events for a node. A fuzzy snapshot
* can temporarily contain a node whose ACL id is absent from the serialized
* ACL cache. Transaction replay repairs that reference; until then, use a
* no-permissions ACL instead of aborting replay. During normal restore the
* watch tables are empty, so the fallback has no watch-delivery effect. If
* the state is encountered while watches are present, it fails closed
* (ZOOKEEPER-4799).
*/
private List<ACL> getACLForWatchTrigger(DataNode node, String path) {
Long aclId;
List<ACL> acls;
synchronized (node) {
aclId = node.acl;
acls = aclCache.convertLongIfCached(aclId);
}
if (acls != null) {
return acls;
}
LOG.info("ACL id {} of {} is not in the ACL cache; filtering its watch events as unreadable",
aclId, path);
return NO_PERMISSIONS_ACL;
}

public int aclCacheSize() {
return aclCache.size();
}
Expand Down
27 changes: 27 additions & 0 deletions zookeeper-server/src/main/java/org/apache/zookeeper/server/ReferenceCountedACLCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,24 @@ public synchronized List<ACL> convertLong(Long longVal) {
return acls;
}

/**
* Converts a long to a list of ACLs without throwing if it is absent from
* the cache. A node in a fuzzy snapshot can temporarily reference such an
* absent id until transaction replay repairs it (ZOOKEEPER-4689).
*
* @param longVal ACL id
* @return the corresponding ACLs, or null if the id is not cached
*/
synchronized List<ACL> convertLongIfCached(Long longVal) {
if (longVal == null) {
return null;
}
if (longVal == OPEN_UNSAFE_ACL_ID) {
return ZooDefs.Ids.OPEN_ACL_UNSAFE;
}
return longKeyMap.get(longVal);
}

private long incrementIndex() {
return ++aclIndex;
}
Expand Down Expand Up @@ -170,6 +188,15 @@ public synchronized void addUsage(Long acl) {
return;
}

if (acl > aclIndex) {
// The ACL cache is serialized before the nodes in a fuzzy snapshot.
// A node can therefore reference an id absent from the serialized
// cache. Reserve every id mentioned by the loaded tree so replay
// cannot assign it to a different ACL before repairing the node.
LOG.info("Advancing aclIndex from {} to {} based on a node reference in the snapshot", aclIndex, acl);
aclIndex = acl;
}

if (!longKeyMap.containsKey(acl)) {
LOG.info("Ignoring acl {} as it does not exist in the cache", acl);
return;
Expand Down
29 changes: 29 additions & 0 deletions zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import org.apache.zookeeper.data.Stat;
import org.apache.zookeeper.metrics.MetricsUtils;
import org.apache.zookeeper.txn.CreateTxn;
import org.apache.zookeeper.txn.DeleteTxn;
import org.apache.zookeeper.txn.TxnHeader;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
Expand Down Expand Up @@ -647,6 +648,34 @@ public void testCreateNodeFixMissingACL() throws Exception {
dt.createNode("/the_parent/the_child", new byte[0], ZooDefs.Ids.CREATOR_ALL_ACL, -1, 2, 2, 2);
}

/**
* Applying a create txn to an existing node (routine when txns overlap
* with the state restored from a fuzzy snapshot, e.g. on DIFF sync) must
* keep the ACL reference counts exact, so that unused ACLs are garbage
* collected and the cache does not grow until the next reload.
*
* See ZOOKEEPER-4689.
*/
@Test
public void testCreateTxnReplayOnExistingNodeKeepsAclRefCountExact() throws Exception {
DataTree dt = new DataTree();
int baseline = dt.aclCacheSize();

TxnHeader hdr = new TxnHeader(1, 2, 2, 2, ZooDefs.OpCode.create);
Record txn = new CreateTxn("/x", new byte[0], ZooDefs.Ids.CREATOR_ALL_ACL, false, -1);
dt.processTxn(hdr, txn);
// Duplicate apply of the same create txn hits the node-exists branch.
dt.processTxn(hdr, txn);
assertEquals(baseline + 1, dt.aclCacheSize());

TxnHeader deleteHdr = new TxnHeader(1, 2, 3, 2, ZooDefs.OpCode.delete);
Record deleteTxn = new DeleteTxn("/x");
dt.processTxn(deleteHdr, deleteTxn);
// The ACL is no longer referenced by any node, so it must have been
// garbage collected.
assertEquals(baseline, dt.aclCacheSize());
}

private DataTree buildDataTreeForTest() {
final DataTree dt = new DataTree();
assertEquals(dt.lastProcessedZxid, 0);
Expand Down
Loading