Include last-committed data in publication by DaveCTurner · Pull Request #92259 · elastic/elasticsearch

DaveCTurner · 2022-12-09T16:58:44Z

The cluster coordination consistency layer relies on a couple of fields within Metadata which record the last committed values on each node. In contrast, the rest of the cluster state can only be changed at accept time.

In the past we would copy these fields over from the master on every publication, but since #90101 we don't copy anything at all if the Metadata is unchanged on the master. However, the master computes the diff against the last committed state whereas the receiving nodes apply the diff to the last accepted state, and this means if the master sends a no-op Metadata diff then the receiving node will revert its last-committed values to the ones included in the state it last accepted.

With this commit we include the last-committed values alongside the cluster state diff so that they are always copied properly.

Closes #90158

DaveCTurner · 2022-12-09T17:00:03Z

Alternative to #92258 that doesn't involve changing the safety algorithm. Not as bad as I thought it would be.

Repeating here: I think this bug is more of a liveness concern than a safety one: it means that an election may incorrectly wait to collect votes from the previous configuration as well as the current one.

~~NB no tests yet, I was to see this run through CI first.~~ (edit: now has tests)

The cluster coordination consistency layer relies on a couple of fields within `Metadata` which record the last _committed_ values on each node. In contrast, the rest of the cluster state can only be changed at _accept_ time. In the past we would copy these fields over from the master on every publication, but since elastic#90101 we don't copy anything at all if the `Metadata` is unchanged on the master. However, the master computes the diff against the last _committed_ state whereas the receiving nodes apply the diff to the last _accepted_ state, and this means if the master sends a no-op `Metadata` diff then the receiving node will revert its last-committed values to the ones included in the state it last accepted. With this commit we include the last-committed values alongside the cluster state diff so that they are always copied properly.

elasticsearchmachine · 2022-12-11T20:38:15Z

Hi @DaveCTurner, I've created a changelog YAML for you.

elasticsearchmachine · 2022-12-11T20:38:20Z

Pinging @elastic/es-distributed (Team:Distributed)

DaveCTurner · 2022-12-11T20:43:33Z

We should backport this to 8.6, but I'm unsure if it'll land in 8.6.0 or 8.6.1. It's not serious enough to call it a blocker for 8.6.0 IMO. However the 8.6.1 version constant doesn't exist yet so I am going to have to set the BwC version to 8.6.0 while backporting, and then we may have to adjust this when bumping versions after the 8.6.0 release.

cc @henningandersen who may have a different opinion about calling this a blocker for 8.6.0.
cc @kingherc who I believe will be doing the 8.6.0 version bump.

elasticsearchmachine · 2022-12-11T20:50:01Z

Hi @DaveCTurner, I've updated the changelog YAML for you.

henningandersen

Left a few comments, probably to be considered questions.

henningandersen · 2022-12-12T08:37:57Z

server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationState.java

+            final var hasClusterUuid = lastAcceptedState.metadata().clusterUUID().equals(Metadata.UNKNOWN_CLUSTER_UUID) == false;
+            assert hasClusterUuid : "received cluster state with empty cluster uuid: " + lastAcceptedState;
+
+            if (hasClusterUuid && lastAcceptedState.metadata().clusterUUIDCommitted() == false) {


Feels odd to protect the logging by something we assert on above?

Suggested change

if (hasClusterUuid && lastAcceptedState.metadata().clusterUUIDCommitted() == false) {

if (lastAcceptedState.metadata().clusterUUIDCommitted() == false) {

Ehh I just kept the same logic as before but I don't think I've ever seen this assertion trip. I'd be ok to drop it.

henningandersen · 2022-12-12T08:41:09Z

server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationState.java

-                setLastAcceptedState(ClusterState.builder(lastAcceptedState).metadata(metadataBuilder).build());
+
+            final var adjustedMetadata = lastAcceptedState.metadata()
+                .withLastCommittedValues(hasClusterUuid, lastAcceptedState.getLastAcceptedConfiguration());


Should we pass in hasClusterUuid || lastAcceptedState.metadata().clusterUUIDCommitted() here? I mean, we are outside assertions already, so could also pass in true. But assuming assertions have some case we are trying to protect against, passing in the original committed value seems safer and also in line with original code.

henningandersen · 2022-12-12T08:49:38Z

server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationState.java

         * marked as committed.
         */
        default void markLastAcceptedStateAsCommitted() {
-            final ClusterState lastAcceptedState = getLastAcceptedState();


I think the code is cleaner, but I failed to spot the important difference, is it just the logging you wanted out of this?

I don't think this does anything different, there's just no need to use a Metadata.Builder here any more.

Thanks. In that case, let us revert this and move this to a follow-up, where we can also assume that assertions are held (i.e., use true instead of hasClusterUuid later in the code).

kingherc · 2022-12-12T09:10:31Z

server/src/main/java/org/elasticsearch/cluster/coordination/PublicationTransportHandler.java

        TransportRequestOptions.Type.STATE
    );

+    public static final Version INCLUDES_LAST_COMMITTED_DATA_VERSION = Version.V_8_7_0;


@DaveCTurner should this be 8.6.0 since it's intended to be backported?

I'll adjust that while backporting. If I set this to 8.6.0 now then the BwC test suite would fail on this PR, but it's important for those tests to pass because they tell us whether there are any other more subtle BwC problems here.

Joining the dots here:

I opened a PR at Include last-committed data in publication #92277 which backports this to 8.6 with a fixed BwC version, and waited for it to pass CI.

I merged 296eacd which disables the BwC tests in main.

I merged the backport PR.

I opened another PR at Fixup BWC after backport of #92259 #92280 to fix up the BwC version in main and reinstate the BwC tests, which will be merged once it passes CI.

DaveCTurner · 2022-12-12T09:44:32Z

I reverted the tidy-up to CoordinationState in 768a864 because it's not necessary for the backport. I'll follow-up with that change in a separate PR

henningandersen

LGTM.

...r/src/test/java/org/elasticsearch/cluster/coordination/PublicationTransportHandlerTests.java

…cation

The cluster coordination consistency layer relies on a couple of fields within `Metadata` which record the last _committed_ values on each node. In contrast, the rest of the cluster state can only be changed at _accept_ time. In the past we would copy these fields over from the master on every publication, but since elastic#90101 we don't copy anything at all if the `Metadata` is unchanged on the master. However, the master computes the diff against the last _committed_ state whereas the receiving nodes apply the diff to the last _accepted_ state, and this means if the master sends a no-op `Metadata` diff then the receiving node will revert its last-committed values to the ones included in the state it last accepted. With this commit we include the last-committed values alongside the cluster state diff so that they are always copied properly. Closes elastic#90158 Backport of elastic#92259 to 8.6

The cluster coordination consistency layer relies on a couple of fields within `Metadata` which record the last _committed_ values on each node. In contrast, the rest of the cluster state can only be changed at _accept_ time. In the past we would copy these fields over from the master on every publication, but since #90101 we don't copy anything at all if the `Metadata` is unchanged on the master. However, the master computes the diff against the last _committed_ state whereas the receiving nodes apply the diff to the last _accepted_ state, and this means if the master sends a no-op `Metadata` diff then the receiving node will revert its last-committed values to the ones included in the state it last accepted. With this commit we include the last-committed values alongside the cluster state diff so that they are always copied properly. Closes #90158 Backport of #92259 to 8.6

Adjusts the BWC version for the wire protocol and re-enables the BWC tests.

Extracted from elastic#92259

Extracted from #92259

DaveCTurner added >bug WIP :Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. v8.7.0 labels Dec 9, 2022

DaveCTurner force-pushed the 2022-12-09-include-committed-fields-in-publication branch from 173d526 to ad1574f Compare December 11, 2022 19:50

DaveCTurner added the v8.6.0 label Dec 11, 2022

DaveCTurner mentioned this pull request Dec 11, 2022

Preserve last-committed data at accept time #92258

Closed

DaveCTurner removed the WIP label Dec 11, 2022

DaveCTurner marked this pull request as ready for review December 11, 2022 20:37

DaveCTurner requested review from henningandersen and original-brownbear December 11, 2022 20:37

Update docs/changelog/92259.yaml

226591b

elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Dec 11, 2022

Update docs/changelog/92259.yaml

c95a33e

DaveCTurner mentioned this pull request Dec 11, 2022

[CI] CoordinatorTests testUnhealthyLeaderIsReplaced failing #90158

Closed

henningandersen reviewed Dec 12, 2022

View reviewed changes

kingherc reviewed Dec 12, 2022

View reviewed changes

Revert tidy-up for safer backporting

768a864

DaveCTurner requested a review from henningandersen December 12, 2022 09:46

henningandersen approved these changes Dec 12, 2022

View reviewed changes

...r/src/test/java/org/elasticsearch/cluster/coordination/PublicationTransportHandlerTests.java Outdated Show resolved Hide resolved

DaveCTurner added 2 commits December 12, 2022 11:07

Merge branch 'main' into 2022-12-09-include-committed-fields-in-publi…

88c729d

…cation

Verify we really sent a diff

b910c87

DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 12, 2022

elasticsearchmachine merged commit fb756a9 into elastic:main Dec 12, 2022

DaveCTurner added the backport pending label Dec 12, 2022

DaveCTurner deleted the 2022-12-09-include-committed-fields-in-publication branch December 12, 2022 11:56

DaveCTurner mentioned this pull request Dec 12, 2022

Include last-committed data in publication #92277

Merged

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Dec 12, 2022

Fixup BWC after backport of elastic#92259

70c6fc2

Adjusts the BWC version for the wire protocol and re-enables the BWC tests.

elasticsearchmachine pushed a commit that referenced this pull request Dec 12, 2022

Fixup BWC after backport of #92259 (#92280)

bdf634f

Adjusts the BWC version for the wire protocol and re-enables the BWC tests.

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Dec 12, 2022

Tidy up CoordinationState

9b8c8b3

Extracted from elastic#92259

DaveCTurner mentioned this pull request Dec 12, 2022

Tidy up CoordinationState #92294

Merged

DaveCTurner added a commit that referenced this pull request Dec 13, 2022

Tidy up CoordinationState (#92294)

93bc07e

Extracted from #92259

original-brownbear mentioned this pull request Jan 3, 2023

[CI] FullRollingRestartIT testFullRollingRestart failing #91857

Closed

	if (hasClusterUuid && lastAcceptedState.metadata().clusterUUIDCommitted() == false) {
	if (lastAcceptedState.metadata().clusterUUIDCommitted() == false) {

Conversation

DaveCTurner commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaveCTurner commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Dec 11, 2022

Uh oh!

elasticsearchmachine commented Dec 11, 2022

Uh oh!

DaveCTurner commented Dec 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Dec 11, 2022

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Dec 12, 2022

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DaveCTurner commented Dec 9, 2022 •

edited

Loading

DaveCTurner commented Dec 9, 2022 •

edited

Loading

DaveCTurner commented Dec 11, 2022 •

edited

Loading