Skip to content

Prevent graceful shutdown on split brain #2277

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

Jump to bottom
Merged
merged 3 commits into from
May 20, 2025
Merged

Prevent graceful shutdown on split brain #2277

merged 3 commits into from
May 20, 2025

Conversation

ryanemerson
Copy link
Contributor

Closes #2273

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented May 16, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ryanemerson ryanemerson force-pushed the 2273/prevent_graceful_shutdown_on_split_brain branch from 0ca720f to 81914bb Compare May 19, 2025 09:45
@ryanemerson ryanemerson marked this pull request as ready for review May 19, 2025 10:49
@ryanemerson ryanemerson requested a review from Crumby May 19, 2025 10:49
@ryanemerson ryanemerson force-pushed the 2273/prevent_graceful_shutdown_on_split_brain branch 2 times, most recently from 69899e3 to 2c38db6 Compare May 19, 2025 16:48
jabolina
jabolina reviewed May 19, 2025
View reviewed changes
pkg/reconcile/pipeline/infinispan/handler/manage/upgrades.go

health, err := podMeta.client.Container().HealthStatus()
if err != nil {
ctx.Requeue(fmt.Errorf("unable to retrieve cluster health status for pod '%s': %w", pod.Name, err))
Copy link
Member

Choose a reason for hiding this comment

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

In this case, health is the new endpoint that only verifies if the process is alive?

Copy link
Contributor Author

@ryanemerson ryanemerson May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's currently calling /health/status and using the cluster_health field which is determined by https://github.com/infinispan/infinispan/blob/3caa2cdf0dc191603067ab32353f072a966125a7/core/src/main/java/org/infinispan/health/impl/ClusterHealthImpl.java#L33-L41.

I think this should remain using this endpoint as this way we ensure we don't proceed with an upgrade if there is one or more DEGRADED caches.

So maybe we shouldn't deprecate container/health/status after all?

@ryanemerson
Copy link
Contributor Author

@Crumby Setting the cache to AVAILABLE explicitly seems to be working, from https://github.com/infinispan/infinispan-operator/actions/runs/15118659859/job/42495774879?pr=2277:

2025-05-19 17:14:03	INFO	upgrade/common.go:283	Setting org.infinispan.LOCKS cache to AVAILABLE for Operand Upstream=14.0.9, Downstream=<nil>, Image=quay.io/infinispan/server:14.0.9.Final, CVE=false, Deprecated=false

I realised that I was passing the wrong operand though, which is fixed in the latest commit.

@ryanemerson ryanemerson force-pushed the 2273/prevent_graceful_shutdown_on_split_brain branch from 6fa42fd to 3021317 Compare May 19, 2025 20:00
Crumby
Crumby reviewed May 20, 2025
View reviewed changes
Crumby
Crumby reviewed May 20, 2025
View reviewed changes
pkg/reconcile/pipeline/infinispan/handler/manage/upgrades.go Outdated
@@ -141,18 +142,80 @@ func GracefulShutdown(i *ispnv1.Infinispan, ctx pipeline.Context) {
return
}

var rebalanceDisabled bool
type PodMeta struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we name it ServerMeta? I think PodMeta is often used in relation to k8s Pod Metadata and I'd expect the information from those in here. May confuse us in the future

@ryanemerson ryanemerson changed the title 2273/prevent graceful shutdown on split brain Prevent graceful shutdown on split brain May 20, 2025
@ryanemerson ryanemerson mentioned this pull request May 20, 2025
Open
Crumby
Crumby reviewed May 20, 2025
View reviewed changes
pkg/reconcile/pipeline/infinispan/handler/manage/upgrades.go Outdated
// If any of the caches are not marked as HEALTHY we must prevent a GracefulShutdown to prevent
// the cluster from entering an unexpected state
if health != ispnApi.HealthStatusHealthy {
ctx.Requeue(fmt.Errorf("unable to proceed with GracefulShutdown as the cluster health is '%s'", health))
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Can we point out which caches are causing issues here to point out to user where to look?
  2. I think we should propagate the state to the Infinispan Status and generate warn events for the users to see. The user may not have access to Operator logs. (The same applies for ClusterSize status)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the code to fire a warning event when the upgrade is blocked.

Can we point out which caches are causing issues here to point out to user where to look?

That should be possible if we retrieve the individual cache status from the /rest/v2/container/health endpoint.

2. I think we should propagate the state to the Infinispan Status and generate warn events for the users to see. The user may not have access to Operator logs. (The same applies for ClusterSize status)

I believe that's beyond the scope of this PR, I have created #2282 to track this.

@Crumby
Copy link
Collaborator

Crumby commented May 20, 2025

@ryanemerson Tests are completely missing here, I wonder whether we could create two:

  • One for split-brain scenario by introducing NetworkPolicy
  • One for degraded cache. This can be harder to achieve but it was possible to introduce degraded cache to the cluster by creating a cache with invalid configuration some time ago

@ryanemerson ryanemerson force-pushed the 2273/prevent_graceful_shutdown_on_split_brain branch 2 times, most recently from 8c67f22 to b98c3b8 Compare May 20, 2025 10:44
@ryanemerson
Copy link
Contributor Author

  • One for split-brain scenario by introducing NetworkPolicy

  • One for degraded cache. This can be harder to achieve but it was possible to introduce degraded cache to the cluster by creating a cache with invalid configuration some time ago

Both of these would be good to have. The problem is that until we have #2282 there's no straightforward way to determine the Operator state in the tests as we execute the operator locally in it's own go-routine.

This can be harder to achieve but it was possible to introduce degraded cache to the cluster by creating a cache with invalid configuration some time ago

It's possible to manually set a cache to DEGRADED via the rest API: https://infinispan.org/docs/stable/titles/rest/rest.html#rest_v2_caches_set_availability

@ryanemerson ryanemerson force-pushed the 2273/prevent_graceful_shutdown_on_split_brain branch from b98c3b8 to 8f88b29 Compare May 20, 2025 11:07
@ryanemerson
Copy link
Contributor Author

Writing the last comment made me re-think the problem 🙂

We already have the Stopping condition which is meant to reflect that the status of cluster scale down. So I have made it so that we keep this as False with an appropriate error message if the cluster can't scale down due to either a split-brain or a DEGRADED cache.

I have also added a test for the latter case.

@ryanemerson ryanemerson force-pushed the 2273/prevent_graceful_shutdown_on_split_brain branch from 8f88b29 to fcc02ff Compare May 20, 2025 11:38
Crumby
Crumby reviewed May 20, 2025
View reviewed changes
Crumby
Crumby reviewed May 20, 2025
View reviewed changes
Crumby
Crumby reviewed May 20, 2025
View reviewed changes
pkg/reconcile/pipeline/infinispan/handler/manage/upgrades.go Outdated
continue
}
ctx.Requeue(fmt.Errorf("unable to create Infinispan client for cluster being upgraded: %w", err))
ctx.Requeue(fmt.Errorf("unable to create Infinispan client to determine if split-brain is present: %w", err))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the message here be more generic? As far as I understand the code we are trying to get the client to get various information. Not just split brain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update to "unable to create Infinispan client to determine if cluster is healthy: %w"

Crumby
Crumby reviewed May 20, 2025
View reviewed changes
@ryanemerson ryanemerson force-pushed the 2273/prevent_graceful_shutdown_on_split_brain branch from fcc02ff to e3eeb88 Compare May 20, 2025 13:57
@ryanemerson ryanemerson force-pushed the 2273/prevent_graceful_shutdown_on_split_brain branch from e3eeb88 to fe163b8 Compare May 20, 2025 14:02
@ryanemerson ryanemerson merged commit b8d84ca into infinispan:main May 20, 2025
10 checks passed
@ryanemerson
Copy link
Contributor Author

Thanks for reviewing @Crumby

@github-actions github-actions bot mentioned this pull request May 20, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jabolina jabolina jabolina left review comments

@Crumby Crumby Crumby approved these changes

Assignees
No one assigned
Labels
2.4.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prevent a GracefulShutdown if view is != #replicas or any cache is not in a HEALTHY state
3 participants
@ryanemerson @Crumby @jabolina