ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp… #1581

srdo · 2021-01-18T15:06:41Z

…ty is true

snapshot.trust.empty is an escape hatch for users upgrading from 3.4.x to later Zookeeper
versions, allowing nodes to start with a non-empty transaction log but no snapshot.

The intent is for this setting to be enabled for a short while during the upgrade,
and then disabled again, as the check it disables is a safety feature.

Prior to this PR, a node would only write a snapshot locally if it became leader,
or if it had fallen so far behind the leader that the leader sent a SNAP message instead
of a DIFF. This made the upgrade process inconvenient, as not all nodes would create
a snapshot when snapshot.trust.empty was true, meaning that the safety check could
not be flipped back on.

This PR makes follower nodes write a local snapshot when they receive NEWLEADER,
if they have no local snapshot and snapshot.trust.empty is true.

srdo · 2021-01-18T15:07:28Z

Additional context here https://mail-archives.apache.org/mod_mbox/zookeeper-dev/202101.mbox/%3cpony-db78e81c2a8c7e7aa50468ebe248816470b8ee2f-8a8224758cb92c542b444f6af8884c82154cdaf2@dev.zookeeper.apache.org%3e

srdo · 2021-01-18T21:49:26Z

I think these test failures are on master as well, see e.g. https://github.com/apache/zookeeper/runs/1717919885. Seems to be tests trying to bind in-use ports.

eolivelli

well done.
@ztzg please consider adding this to 3.7.
it will help a lot users in getting rid of old EOL 3.4 zookeeper clusters

eolivelli · 2021-01-19T10:19:25Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java

@@ -530,6 +530,10 @@ public void takeSnapshot(boolean syncSnap) {
        ServerMetrics.getMetrics().SNAPSHOT_TIME.add(elapsed);
    }

+    public boolean shouldForceWriteInitialSnapshotDuring34Migration() {


there is no need to add 'During34Migration', we can just leave a javadoc

Ok, will fix. How about shouldForceWriteInitialSnapshotAfterLeaderElection?

eolivelli · 2021-01-19T10:20:30Z

zookeeper-server/src/test/java/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java

+                    List<File> snapshots = txnLogFactory.findNRecentSnapshots(10);
+                    assertTrue(snapshots.size() > 0, "We have a snapshot to corrupt");
+                    for (File file : snapshots) {
+                        Files.deleteIfExists(file.toPath());


we are sure that the file exists, we can just use 'delete', correct ?

Yes, will fix

srdo · 2021-01-27T14:30:10Z

@eolivelli I addressed your comments (Sorry for pinging, I'm not sure Github sends messages when I push updates)

eolivelli

LGTM
@ztzg @hanm PTAL

ztzg

LGTM, but I have a small question/doubt regarding one section of the test. What do you think?

ztzg · 2021-02-17T16:41:27Z

zookeeper-server/src/test/java/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java

+            String connString = qu.getConnectionStringForServer(1);
+            try (ZooKeeper zk = new ZooKeeper(connString, CONNECTION_TIMEOUT, this)) {
+                for (int i = 0; i < N_TRANSACTIONS; i++) {
+                    zk.create("/node-" + i, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+                }
+            }


This section creates transactions, ensuring a reasonable xzid, but might not result in a snapshot, as it doesn't lower the snapCount using SyncRequestProcessor.setSnapCount. (The other tests do, and might run before. Might be a good idea to be explicit if the goal of running N_TRANSACTIONS is to create one?)

This does not mean that the test is broken: a snapshot is present, but it is there because of the initialize file left behind by ClientBase.createTmpDir.

The goal here is not to make the nodes write a snapshot due to hitting the snapcount. I just want to put some data in Zookeeper, since if Zookeeper is empty, it will create a snapshot file on boot, even without this change.

The behavior this PR changes is that if Zookeeper is booted with the trust empty property set to true, and the cluster is not empty, then the follower nodes will write a snapshot file if they don't already have one.

The issue with the code as it is without this change is that if you have a 3.4 cluster with low traffic and you then enable ZOOKEEPER_SNAPSHOT_TRUST_EMPTY and upgrade to 3.5+, then it may take a very long time for follower nodes to write snapshots, since they only do it if they become leader, or if Zookeeper is empty, or if the snapcount is hit. That makes ZOOKEEPER_SNAPSHOT_TRUST_EMPTY pretty inconvenient, since you can't remove that property again until all nodes have at least one snapshot.

I have added a few lines to the test to assert that the data created here is still there once the followers come up, that seems like a healthy thing to check.

I guess I could use another number than N_TRANSACTIONS if it is misleading?

The goal here is not to make the nodes write a snapshot due to hitting the snapcount. I just want to put some data in Zookeeper, since if Zookeeper is empty, it will create a snapshot file on boot, even without this change.

Okay.

(I understand the rest of the PR, and don't have any objections. I was just wondering if the N_TRANSACTIONS was because you were trying to hit snapCount, like other tests in that file seem to be doing.)

Thank you for the clarification!

Just to summarize what this test is intended to do:

It is supposed to set up a quorum with some data in it. It then deletes the snapshots on all follower nodes. This simulates what a low-traffic cluster being upgraded from 3.4 might look like. The cluster is restarted with ZOOKEEPER_SNAPSHOT_TRUST_EMPTY=true, and the test verifies that all nodes get a snapshot written. This will allow the operator to remove ZOOKEEPER_SNAPSHOT_TRUST_EMPTY again and reboot the cluster shortly after, easing migration.

Right; we are on the same page :)

Would you mind rebasing this on top of the latest master, so that CI picks the new configuration and re-runs the test suite with a higher chance of success?

Sure. Rebased and squashed. FWIW I ran the two failing tests locally, and they passed, so am guessing it's just flakiness.

Also just a note on rebasing on top of master, since the behavior surprised me, and I thought I'd share: GH Actions' checkout action does not check out the PR branch, it instead creates a merge commit between the PR branch and the target branch, which it then runs the rest of the CI workflow on. So likely the previous run did run with the config from master.

ztzg · 2021-02-17T16:58:46Z

@ztzg please consider adding this to 3.7.

Okay, noted.

srdo · 2021-02-18T20:38:46Z

The failing test is a different one this time than last time. I think it's just flaky.

…ty is true snapshot.trust.empty is an escape hatch for users upgrading from 3.4.x to later Zookeeper versions, allowing nodes to start with a non-empty transaction log but no snapshot. The intent is for this setting to be enabled for a short while during the upgrade, and then disabled again, as the check it disables is a safety feature. Prior to this PR, a node would only write a snapshot locally if it became leader, or if it had fallen so far behind the leader that the leader sent a SNAP message instead of a DIFF. This made the upgrade process inconvenient, as not all nodes would create a snapshot when snapshot.trust.empty was true, meaning that the safety check could not be flipped back on. This PR makes follower nodes write a local snapshot when they receive NEWLEADER, if they have no local snapshot and snapshot.trust.empty is true.

srdo · 2021-02-25T17:36:36Z

Turns out it was just a flaky test. I think this should be ready to go in now. I'm not sure what your backporting policy is like, but it would be nice to get on as many branches as possible :)

…ty is true snapshot.trust.empty is an escape hatch for users upgrading from 3.4.x to later Zookeeper versions, allowing nodes to start with a non-empty transaction log but no snapshot. The intent is for this setting to be enabled for a short while during the upgrade, and then disabled again, as the check it disables is a safety feature. Prior to this PR, a node would only write a snapshot locally if it became leader, or if it had fallen so far behind the leader that the leader sent a SNAP message instead of a DIFF. This made the upgrade process inconvenient, as not all nodes would create a snapshot when snapshot.trust.empty was true, meaning that the safety check could not be flipped back on. This PR makes follower nodes write a local snapshot when they receive NEWLEADER, if they have no local snapshot and snapshot.trust.empty is true. Author: Stig Rohde Døssing <stig@humio.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Damien Diederen <ddiederen@apache.org> Closes #1581 from srdo/zookeeper-3781 (cherry picked from commit 1214d3b) Signed-off-by: Damien Diederen <ddiederen@apache.org>

ztzg · 2021-03-06T19:44:25Z

Thank you, @srdo! This is now in master, branch-3.7, and branch-3.7.0.

srdo · 2021-03-06T19:46:08Z

Sounds good, thank you.

…ty is true snapshot.trust.empty is an escape hatch for users upgrading from 3.4.x to later Zookeeper versions, allowing nodes to start with a non-empty transaction log but no snapshot. The intent is for this setting to be enabled for a short while during the upgrade, and then disabled again, as the check it disables is a safety feature. Prior to this PR, a node would only write a snapshot locally if it became leader, or if it had fallen so far behind the leader that the leader sent a SNAP message instead of a DIFF. This made the upgrade process inconvenient, as not all nodes would create a snapshot when snapshot.trust.empty was true, meaning that the safety check could not be flipped back on. This PR makes follower nodes write a local snapshot when they receive NEWLEADER, if they have no local snapshot and snapshot.trust.empty is true. Author: Stig Rohde Døssing <stig@humio.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Damien Diederen <ddiederen@apache.org> Closes #1581 from srdo/zookeeper-3781 (cherry picked from commit 1214d3b) Signed-off-by: Damien Diederen <ddiederen@apache.org> (cherry picked from commit 679cc2b)

…ty is true snapshot.trust.empty is an escape hatch for users upgrading from 3.4.x to later Zookeeper versions, allowing nodes to start with a non-empty transaction log but no snapshot. The intent is for this setting to be enabled for a short while during the upgrade, and then disabled again, as the check it disables is a safety feature. Prior to this PR, a node would only write a snapshot locally if it became leader, or if it had fallen so far behind the leader that the leader sent a SNAP message instead of a DIFF. This made the upgrade process inconvenient, as not all nodes would create a snapshot when snapshot.trust.empty was true, meaning that the safety check could not be flipped back on. This PR makes follower nodes write a local snapshot when they receive NEWLEADER, if they have no local snapshot and snapshot.trust.empty is true. Author: Stig Rohde Døssing <stig@humio.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Damien Diederen <ddiederen@apache.org> Closes apache#1581 from srdo/zookeeper-3781 (cherry picked from commit 1214d3b) Signed-off-by: Damien Diederen <ddiederen@apache.org> (cherry picked from commit 9722488)

…ty is true snapshot.trust.empty is an escape hatch for users upgrading from 3.4.x to later Zookeeper versions, allowing nodes to start with a non-empty transaction log but no snapshot. The intent is for this setting to be enabled for a short while during the upgrade, and then disabled again, as the check it disables is a safety feature. Prior to this PR, a node would only write a snapshot locally if it became leader, or if it had fallen so far behind the leader that the leader sent a SNAP message instead of a DIFF. This made the upgrade process inconvenient, as not all nodes would create a snapshot when snapshot.trust.empty was true, meaning that the safety check could not be flipped back on. This PR makes follower nodes write a local snapshot when they receive NEWLEADER, if they have no local snapshot and snapshot.trust.empty is true. Author: Stig Rohde Døssing <stig@humio.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Damien Diederen <ddiederen@apache.org> Closes apache#1581 from srdo/zookeeper-3781

…ty is true (#111) snapshot.trust.empty is an escape hatch for users upgrading from 3.4.x to later Zookeeper versions, allowing nodes to start with a non-empty transaction log but no snapshot. The intent is for this setting to be enabled for a short while during the upgrade, and then disabled again, as the check it disables is a safety feature. Prior to this PR, a node would only write a snapshot locally if it became leader, or if it had fallen so far behind the leader that the leader sent a SNAP message instead of a DIFF. This made the upgrade process inconvenient, as not all nodes would create a snapshot when snapshot.trust.empty was true, meaning that the safety check could not be flipped back on. This PR makes follower nodes write a local snapshot when they receive NEWLEADER, if they have no local snapshot and snapshot.trust.empty is true. Author: Stig Rohde Døssing <stig@humio.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Damien Diederen <ddiederen@apache.org> Closes apache#1581 from srdo/zookeeper-3781 Co-authored-by: Stig Rohde Døssing <stig@humio.com>

…ty is true snapshot.trust.empty is an escape hatch for users upgrading from 3.4.x to later Zookeeper versions, allowing nodes to start with a non-empty transaction log but no snapshot. The intent is for this setting to be enabled for a short while during the upgrade, and then disabled again, as the check it disables is a safety feature. Prior to this PR, a node would only write a snapshot locally if it became leader, or if it had fallen so far behind the leader that the leader sent a SNAP message instead of a DIFF. This made the upgrade process inconvenient, as not all nodes would create a snapshot when snapshot.trust.empty was true, meaning that the safety check could not be flipped back on. This PR makes follower nodes write a local snapshot when they receive NEWLEADER, if they have no local snapshot and snapshot.trust.empty is true. Author: Stig Rohde Døssing <stig@humio.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Damien Diederen <ddiederen@apache.org> Closes apache#1581 from srdo/zookeeper-3781 (cherry picked from commit 1214d3b) Signed-off-by: Damien Diederen <ddiederen@apache.org> (cherry picked from commit 9722488)

srdo force-pushed the zookeeper-3781 branch from b54c45c to 1a0e172 Compare January 18, 2021 16:07

eolivelli requested changes Jan 19, 2021

View reviewed changes

eolivelli approved these changes Jan 27, 2021

View reviewed changes

ztzg approved these changes Feb 17, 2021

View reviewed changes

srdo force-pushed the zookeeper-3781 branch from 9ef3dff to 676c973 Compare February 17, 2021 19:41

srdo force-pushed the zookeeper-3781 branch from 676c973 to 4d3f8b6 Compare February 23, 2021 12:28

ztzg closed this in 1214d3b Mar 6, 2021

symat mentioned this pull request May 16, 2022

ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp… #1876

Closed

anuragmadnawat1 mentioned this pull request Nov 2, 2022

ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.empty is true anuragmadnawat1/zookeeper#111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp… #1581

ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp… #1581

srdo commented Jan 18, 2021

srdo commented Jan 18, 2021

srdo commented Jan 18, 2021

eolivelli left a comment

eolivelli Jan 19, 2021

srdo Jan 19, 2021

eolivelli Jan 19, 2021

srdo Jan 19, 2021

srdo commented Jan 27, 2021

eolivelli left a comment

ztzg left a comment

ztzg Feb 17, 2021

srdo Feb 17, 2021 •

edited

Loading

srdo Feb 17, 2021

ztzg Feb 17, 2021

srdo Feb 17, 2021

ztzg Feb 17, 2021

srdo Feb 17, 2021

srdo Feb 17, 2021

ztzg commented Feb 17, 2021

srdo commented Feb 18, 2021

srdo commented Feb 25, 2021

ztzg commented Mar 6, 2021

srdo commented Mar 6, 2021

ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp… #1581

ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp… #1581

Conversation

srdo commented Jan 18, 2021

srdo commented Jan 18, 2021

srdo commented Jan 18, 2021

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdo commented Jan 27, 2021

eolivelli left a comment

Choose a reason for hiding this comment

ztzg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdo Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ztzg commented Feb 17, 2021

srdo commented Feb 18, 2021

srdo commented Feb 25, 2021

ztzg commented Mar 6, 2021

srdo commented Mar 6, 2021

srdo Feb 17, 2021 •

edited

Loading