ISPN-14329 Availability of caches should be prevented until a cluster… #10479

jabolina · 2022-11-23T14:37:44Z

… is complete after "shutdown cluster"

https://issues.redhat.com/browse/ISPN-14329
https://issues.redhat.com/browse/ISPN-14414

After the cluster shutdown, the nodes must wait for the complete recovery before accepting commands. We keep the cache's registry as INITIALIZING until all nodes join back and the restoration of a stable topology. We do this if the cache has a non-shared store without purging on start.

We log and return an exception with the current members. We can't say the missing member's address for sure, as we only have the persistent UUID.

With the test we added, the log message is:

11:19:12,840 WARN  (jgroups-7,ThreeNodeGlobalStatePartialRestartTest-NodeF:[]) [o.i.t.ClusterCacheStatus] ISPN000681: Recovering cache testCache missing members, currently have [ThreeNodeGlobalStatePartialRestartTest-NodeF, ThreeNodeGlobalStatePartialRestartTest-NodeG] of a total of 3

We still need a way to send commands and force the cache to start, even if that means data loss. And handle the inconsistent behavior from reports. That's follow-up work.

But I also wanted to gather some reviews/comments on the approach here (and see how CI behaves). There is also the option to block the cache creation until the cluster is back or receives a manual intervention.

wfink · 2022-12-07T12:30:31Z

Blocking the clients is now what I would expect.

For a better user experience it would be nice to have the WARN message ISPN000681 not only at the coordinator but on each starting node.

Each client access will add an ERROR ISPN005003 with full stack trace, as this is expected I would lower it to WARN and remove the stacktrace, this would prevent from a fast growing logfile at startup if there is a huge number of clients ...

Access with the console force a ISPN012005, is it possible to return the state INITIALIZING to display the cache state for the console, and lower the message to WARN/INFO without stacktrace for the same reasons

wfink · 2022-12-07T13:52:18Z

If the is set the cache is empty after 'shutdown cluster' and started nodes are all back.
If set to false we have the previous behavior, stale entries are added back if one node goes down and up again.

jabolina · 2022-12-07T21:45:49Z

Applied the suggestions to also log the warning message on the client after the join. Another caveat here is that a node is only aware of the node when it joins, that is, the list of members does not update. This can lead to the exception message can be misleading.

I also included a small change in the REST API to handle the initializing case. This way the cache is listed as DEGRADED and the operations (I test get/put) return the exception message of missing members.

wfink · 2022-12-08T12:04:27Z

Server side messages for client access ISPN005003 are still with stacktrace, same for ISPN000682 at client side.
The cluster/cache state DEGRADED might be missleading, as well you can set the cache back to AVAILABLE or enable state-transfer (enable state-transfer will fail with IllegalState!!).
I would prefer to not being able to set state-transfer, only AVAIL and show the cluster/cache state as INIT

wfink · 2022-12-08T12:37:16Z

The WARN message ISPN000681 is now shown for all members during start 👍

core/src/main/java/org/infinispan/factories/AbstractComponentRegistry.java

core/src/main/java/org/infinispan/factories/ComponentRegistry.java

core/src/main/java/org/infinispan/topology/LocalTopologyManagerImpl.java

wburns · 2022-12-09T21:47:26Z

server/rest/src/main/java/org/infinispan/rest/resources/ContainerResource.java

@@ -328,7 +328,7 @@ private CacheInfo getCacheInfo(RestRequest request, EmbeddedCacheManager cacheMa
      if (ignoredCaches.contains(cacheName)) {
         cacheInfo.status = "IGNORED";
      } else {
-         if(cacheHealth != HealthStatus.FAILED) {
+         if(cacheHealth != HealthStatus.FAILED && cacheHealth != HealthStatus.DEGRADED) {


This looks out of place. Is this a different bug?

I ended up adding a new status. The problem here is that the cache was not running, which throws an exception.

jabolina · 2022-12-12T22:08:58Z

core/src/main/java/org/infinispan/manager/DefaultCacheManager.java

+      if (!started) return false;
+      ComponentStatus cs = cacheFuture.join().getStatus();
+      return cs == ComponentStatus.RUNNING || cs == ComponentStatus.INITIALIZING;


I'll revert this and open another JIRA to follow up, this slipped into my commit, it was supposed to be for a local test only. The problem here is that when getting the cache for the REST/CLI API it verifies if the cache is running before applying an operation. Since in our case the cache is still initializing, causes an exception, returning Unexpected error retrieving data. "ISPN012010: Cache with name 'manual' not found amongst the configured caches" to the client, which is odd, since we just saw the cache in a list.

That is fine, I just want to make sure it has its own JIRA to detail what is going on.

Created https://issues.redhat.com/browse/ISPN-14414 to handle this.

jabolina · 2022-12-12T22:13:44Z

I applied all the suggestions/changes now. An additional point of attention is that I added another health status to identify the recovering stage, meaning the console needs to be updated to reflect this, too.

I added another exception instead of using the AvailabilityException so it was easier to log a warning.

core/src/main/java/org/infinispan/topology/MissingMembersException.java

tristantarrant · 2022-12-14T11:25:25Z

LGTM

jabolina · 2022-12-14T12:00:45Z

I'll squash the commits.

wburns

Looks good to me. However, are we not missing some way for an administrator to force the cache to RUNNING state if they don't have all the members?

core/src/main/java/org/infinispan/factories/AbstractComponentRegistry.java

pruivo

LGTM

core/src/main/java/org/infinispan/factories/ComponentRegistry.java

core/src/test/java/org/infinispan/globalstate/ThreeNodeGlobalStatePartialRestartTest.java

jabolina · 2022-12-15T17:01:27Z

Applied the suggestions, thanks.

jabolina · 2022-12-16T12:09:25Z

Looks good to me. However, are we not missing some way for an administrator to force the cache to RUNNING state if they don't have all the members?

That's a next https://issues.redhat.com/browse/ISPN-14418

jabolina · 2022-12-19T21:32:21Z

I am going to add a couple of things:

The coordinator will have an up-to-date list of members when throwing an exception. Other members will have a possibly outdated list (as of right now);
If the cluster restores after the shutdown, we do not purge the store, even if purge=true.

wfink · 2022-12-21T11:59:18Z

Coordinator now sends the correct members during initializing.
purge is not happening if purge=true

Purge is working as expected if 'shudown cluster' is not used but all nodes are stopped and started in a sequence.

PR seems working fine to me

jabolina · 2023-01-06T13:30:28Z

@wfink I've integrated the keyset fixes to the branch, let me know if it is working as expected.

Also, for ignoring purge after the start, I've updated only the SIFS and SingleFile stores, am I missing something here?

wburns

@jabolina let me know what you think about these suggestions.

wburns · 2023-01-06T16:36:38Z

core/src/main/java/org/infinispan/persistence/sifs/NonBlockingSoftIndexFileStore.java

@@ -212,57 +212,47 @@ public CompletionStage<Void> start(InitializationContext ctx) {
      startIndex();
      final AtomicLong maxSeqId = new AtomicLong(0);

-      if (!configuration.purgeOnStartup()) {


TBH, I am not liking this approach to how purgeOnStartup is done. I think there is a lot of value of implementing it directly in the store, otherwise we have to start the store and then clear it, which can be very time consuming (some stores still require this, so having the clear afterwards is fine still). I am thinking it might be better instead to change the configuration provided to the store to have purgeOnStartup in certain circumstances. This way each store is still responsible for purging and can be done in a very efficient manner. For example SIFS can just delete the data and index directory and doesn't need to load them and the subsequent clear will be a noop.

wburns · 2023-01-06T16:36:41Z

core/src/main/java/org/infinispan/factories/ComponentRegistry.java

+    * Caches that do not need state transfer or are private do not need to delay the start.
+    */
+   @Override
+   protected CompletionStage<Void> delayStart() {


While this is okay... I am not liking where this code is residing. This is putting too much logic in the component registry itself. The registry should be more about registering components, not knowing inner details of them.

I wonder is it possible to move this logic to an appropriate interceptor or component? For example we can add the clear logic to the persistence manager and stable topology directly to the LocalTopologyManager/StateTransferManager. Then we would need to disconnect start method invocation from the actual interceptor initialization and leave the state as INITIALIZING until it is done. I think this would keep the logic more closely aligned with where it should be.

jabolina · 2023-01-06T19:05:35Z

@wburns I'll apply the suggestions. The only thing that worries me is if we spread the restoration requirements to different places. Right now, everything is under the component registry, although I agree that this is overloading it too much. I'll try one approach of using a new component only for that. If that is not right, I'll go by putting the logic on each individual component.

jabolina · 2023-01-09T17:53:53Z

Creating a new component dedicated only to restoration wasn't nice, so I added the logic to the LocalTopologyManager/LocalCacheStatus. For the change for persistence, I did a small change only and could keep the purge in the PersistenceManager itself, but it is still calling the clear method after the topology is restored. That is, constructing the whole store and then purging it. I'll look into that now while CI runs.

jabolina · 2023-03-09T14:51:01Z

Pushed a fix, let's see. The failures were all due to blocking calls. I changed the call from isRunning to cacheExists.

infinispanrelease · 2023-03-09T15:04:20Z

Image pushed for Jenkins build #22:

quay.io/infinispan-test/server:PR-10479

infinispanrelease · 2023-03-09T19:49:02Z

Image pushed for Jenkins build #23:

quay.io/infinispan-test/server:PR-10479

wburns · 2023-03-10T20:52:32Z

Pushed a fix, let's see. The failures were all due to blocking calls. I changed the call from isRunning to cacheExists.

Can you expand upon this? I don't think we can just change the method we are testing against here. If anything we may want do this to both methods no?

Actually I don't think we can change this at all as the cacheExists doesn't verify if the cache is actually running or not.

wburns

I think we may need to discuss the blocking issue you ran into before. The getCache method on non blocking thread MUST always be proceeded by a isRunning check to ensure it won't block. We cannot change this behavior.

core/src/main/java/org/infinispan/persistence/file/SingleFileStore.java

core/src/main/java/org/infinispan/manager/DefaultCacheManager.java

core/src/test/java/org/infinispan/globalstate/ThreeNodeGlobalStatePartialRestartTest.java

server/hotrod/src/main/java/org/infinispan/server/hotrod/BaseRequestProcessor.java

server/rest/src/main/java/org/infinispan/rest/RestRequestHandler.java

server/rest/src/main/java/org/infinispan/rest/cachemanager/RestCacheManager.java

server/rest/src/main/java/org/infinispan/rest/resources/CacheResourceV2.java

server/hotrod/src/main/java/org/infinispan/server/hotrod/logging/Log.java

server/rest/src/main/java/org/infinispan/rest/cachemanager/RestCacheManager.java

server/rest/src/main/java/org/infinispan/rest/resources/CacheResourceV2.java

wburns

Just minor things left, otherwise LGTM.

wburns · 2023-03-14T15:29:27Z

core/src/main/java/org/infinispan/reactive/publisher/impl/ClusterPublisherManagerImpl.java

@@ -987,7 +987,7 @@ public CompletionStage<Long> sizePublisher(IntSet segments, InvocationContext ct
       */
      private <E> Flowable<E> getValuesFlowable(BiFunction<InnerPublisherSubscription.InnerPublisherSubscriptionBuilder<K, I, R>, Map.Entry<Address, IntSet>, Publisher<E>> subToFlowableFunction) {
         return Flowable.defer(() -> {
-            if (!componentRegistry.getStatus().allowInvocations()) {
+            if (!componentRegistry.getStatus().allowInvocations() && !componentRegistry.getStatus().startingUp()) {


So I assume the topology exception is thrown later in this case?

Yep, once it reaches the InvocationContextInterceptor. I am unsure if this is one of the places we could fail earlier and avoid creating/sending all requests.

wburns · 2023-03-14T15:30:29Z

core/src/main/java/org/infinispan/util/logging/Log.java

+   @Message(value = "Recovering cache '%s' but there are missing members, known members %s of a total of %s", id = 689)
+   void recoverFromStateMissingMembers(String cacheName, List<Address> members, int total);
+
+   MissingMembersException recoverFromStateMissingMembers(String cacheName, List<Address> members, String total);


This needs a @message on it afaik.

This is a bit of hack, but I can't find it in the docs now :(
Since this has the same signature as the method above, it will inherit the same message and code. I did this so that the warning and exception have the same code. It is hacky because in one, the total is int, and in another is a String.

server/hotrod/src/main/java/org/infinispan/server/hotrod/BaseRequestProcessor.java

server/hotrod/src/main/java/org/infinispan/server/hotrod/logging/Log.java

jabolina · 2023-03-22T13:20:07Z

Added another commit for https://issues.redhat.com/browse/ISPN-14414. With this, we can return the caches on the state INITIALIZING to the console. Note: as we are returning a new value, the console needs to be updated to reflect this new status.

server/rest/src/main/java/org/infinispan/rest/resources/ContainerResource.java

… is complete after "shutdown cluster" After the cluster shutdown, the nodes must wait for the complete recovery before accepting commands. We keep the cache's registry as `INITIALIZING` until all nodes join back and the restoration of a stable topology. We log and return an exception with the current members. We can't say the missing member's address for sure, as we only have the persistent UUID. The coordinator throws the exception with the current member list. Created a dedicated exception to handle missing members. Avoid logging the trace. The downside is that SIFS and single-file store now creates the store from the previous file, even when purge on startup is enabled, to possibly clear all the data after. This could be improved further in following PRs.

wburns · 2023-03-22T15:50:33Z

Changes look fine, just waiting to make sure CI didn't change.

wburns · 2023-03-23T02:46:02Z

Integrated into main, thanks @jabolina !

jabolina added the Suggestions please? label Nov 23, 2022

jabolina added the Changes Suggested label Dec 7, 2022

jabolina force-pushed the ISPN-14329 branch from 5b3614b to 69a0710 Compare December 7, 2022 21:40

wburns requested changes Dec 9, 2022

View reviewed changes

wburns added Changes Required and removed Changes Suggested labels Dec 9, 2022

jabolina commented Dec 12, 2022

View reviewed changes

wfink suggested changes Dec 13, 2022

View reviewed changes

core/src/main/java/org/infinispan/topology/MissingMembersException.java Show resolved Hide resolved

jabolina force-pushed the ISPN-14329 branch from 9f50875 to b23c775 Compare December 14, 2022 12:03

wburns reviewed Dec 15, 2022

View reviewed changes

core/src/main/java/org/infinispan/factories/AbstractComponentRegistry.java Outdated Show resolved Hide resolved

pruivo reviewed Dec 15, 2022

View reviewed changes

core/src/main/java/org/infinispan/factories/ComponentRegistry.java Outdated Show resolved Hide resolved

core/src/test/java/org/infinispan/globalstate/ThreeNodeGlobalStatePartialRestartTest.java Outdated Show resolved Hide resolved

jabolina force-pushed the ISPN-14329 branch from b23c775 to 3f09bb2 Compare December 15, 2022 17:00

jabolina force-pushed the ISPN-14329 branch from 432b3b5 to 146fb1c Compare January 6, 2023 12:53

wburns reviewed Jan 6, 2023

View reviewed changes

jabolina force-pushed the ISPN-14329 branch from a438a7a to c3c3476 Compare January 11, 2023 12:41

jabolina added the Needs Rebase label Mar 9, 2023

jabolina force-pushed the ISPN-14329 branch from 178793c to c52448c Compare March 9, 2023 17:40

jabolina removed the Needs Rebase label Mar 9, 2023

wburns removed the Image Required Set this label in order for a server image to be built with the PR changes and pushed to quay.io label Mar 10, 2023

wburns requested changes Mar 10, 2023

View reviewed changes

jabolina force-pushed the ISPN-14329 branch 4 times, most recently from bda39d4 to 671783a Compare March 13, 2023 21:15

wburns reviewed Mar 13, 2023

View reviewed changes

jabolina force-pushed the ISPN-14329 branch from 671783a to 332102e Compare March 14, 2023 11:52

wburns reviewed Mar 14, 2023

View reviewed changes

jabolina force-pushed the ISPN-14329 branch from 2afd3f6 to e3d6ca9 Compare March 14, 2023 19:36

jabolina force-pushed the ISPN-14329 branch from e3d6ca9 to fe8aaf3 Compare March 22, 2023 13:18

wburns removed the Changes Required label Mar 22, 2023

wburns reviewed Mar 22, 2023

View reviewed changes

server/rest/src/main/java/org/infinispan/rest/resources/ContainerResource.java Outdated Show resolved Hide resolved

jabolina added 2 commits March 22, 2023 12:46

ISPN-14414 REST API retrieve caches in initializing state

10090f3

jabolina force-pushed the ISPN-14329 branch from fe8aaf3 to 10090f3 Compare March 22, 2023 15:46

wburns merged commit 2bd7b00 into infinispan:main Mar 23, 2023
1 check failed

jabolina deleted the ISPN-14329 branch March 23, 2023 11:25

jabolina mentioned this pull request Mar 23, 2023

[14.0.x] ISPN-14329 Availability of caches should be prevented until a cluster... #10727

Merged

jabolina mentioned this pull request Mar 30, 2023

ISPN-12224 Cluster in a confusing state after restarted from graceful… #10381

Merged

ISPN-14329 Availability of caches should be prevented until a cluster… #10479

ISPN-14329 Availability of caches should be prevented until a cluster… #10479

Conversation

jabolina commented Nov 23, 2022 • edited

wfink commented Dec 7, 2022

wfink commented Dec 7, 2022

jabolina commented Dec 7, 2022

wfink commented Dec 8, 2022 • edited

wfink commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jabolina Dec 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jabolina commented Dec 12, 2022

tristantarrant commented Dec 14, 2022

jabolina commented Dec 14, 2022 • edited

wburns left a comment

Choose a reason for hiding this comment

pruivo left a comment

Choose a reason for hiding this comment

jabolina commented Dec 15, 2022

jabolina commented Dec 16, 2022

jabolina commented Dec 19, 2022

wfink commented Dec 21, 2022 • edited

jabolina commented Jan 6, 2023

wburns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jabolina commented Jan 6, 2023

jabolina commented Jan 9, 2023

jabolina commented Mar 9, 2023

infinispanrelease commented Mar 9, 2023

infinispanrelease commented Mar 9, 2023

wburns commented Mar 10, 2023 • edited

wburns left a comment

Choose a reason for hiding this comment

wburns 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

jabolina commented Mar 22, 2023 • edited

wburns commented Mar 22, 2023

wburns commented Mar 23, 2023

jabolina commented Nov 23, 2022 •

edited

wfink commented Dec 8, 2022 •

edited

jabolina Dec 12, 2022 •

edited

jabolina commented Dec 14, 2022 •

edited

wfink commented Dec 21, 2022 •

edited

wburns commented Mar 10, 2023 •

edited

jabolina commented Mar 22, 2023 •

edited