K8SPSMDB-1466 improve MCS error handling to prevent operator crashes #2044
+44
−4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hors
requested review from
egegunes,
gkech,
nmarukovich and
pooknull
as code owners
egegunes
requested changes
Sep 22, 2025
Comment on lines
38
to
41
| // MCS is optional functionality - if discovery fails for any reason, | ||
| // mark it as unavailable and continue without crashing the operator | ||
| available = false | ||
| return nil |
Contributor
There was a problem hiding this comment.
i think we need to log the error and inform users that MCS is not available
egegunes
previously approved these changes
Nov 17, 2025
gkech
reviewed
Nov 17, 2025
pkg/mcs/register_test.go
Outdated
| ) | ||
|
|
||
| func TestIsAvailable(t *testing.T) { | ||
| // Test IsAvailable function |
Contributor
There was a problem hiding this comment.
These comments are not necessary
pkg/mcs/register_test.go
Outdated
Comment on lines
10
to
11
| if !IsAvailable() { | ||
| t.Error("Expected IsAvailable to return true when available is true") |
Contributor
There was a problem hiding this comment.
Why aren't we using assert.True(t, IsAvailable()) and instead we use if statements for assertions?
pkg/mcs/register_test.go
Outdated
| } | ||
|
|
||
| func TestServiceExport(t *testing.T) { | ||
| // Test ServiceExport creation |
pkg/mcs/register_test.go
Outdated
Comment on lines
40
to
51
| if se.Name != name { | ||
| t.Errorf("Expected ServiceExport name to be %s, got: %s", name, se.Name) | ||
| } | ||
|
|
||
| if se.Namespace != namespace { | ||
| t.Errorf("Expected ServiceExport namespace to be %s, got: %s", namespace, se.Namespace) | ||
| } | ||
|
|
||
| if se.Labels["app"] != "test" { | ||
| t.Errorf("Expected ServiceExport label 'app' to be 'test', got: %s", se.Labels["app"]) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
All these could be assertions
Comment on lines
53
to
60
| func TestServiceExportList(t *testing.T) { | ||
| // Test ServiceExportList creation | ||
| seList := ServiceExportList() | ||
|
|
||
| if seList.Kind != "ServiceExportList" { | ||
| t.Errorf("Expected ServiceExportList Kind to be 'ServiceExportList', got: %s", seList.Kind) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Same approach with the other tests, we can drop comments and use assertions
pkg/mcs/register_test.go
Outdated
Comment on lines
20
to
28
| func TestMCSSchemeGroupVersion(t *testing.T) { | ||
| // Test that MCSSchemeGroupVersion is initialized correctly | ||
| if MCSSchemeGroupVersion.Group != "" { | ||
| t.Errorf("Expected MCSSchemeGroupVersion.Group to be empty initially, got: %s", MCSSchemeGroupVersion.Group) | ||
| } | ||
| if MCSSchemeGroupVersion.Version != "" { | ||
| t.Errorf("Expected MCSSchemeGroupVersion.Version to be empty initially, got: %s", MCSSchemeGroupVersion.Version) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Not sure what we are testing here. Since we are not calling a function or something to affect the values, we are essentially asserting that they are empty. Is that what we want?
Collaborator
commit: 6293f90 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CHANGE DESCRIPTION
Description
This PR fixes the issue where the Percona Server MongoDB Operator would crash when encountering errors during Multi-Cluster Services (MCS) discovery, specifically the "stale GroupVersion discovery" error.
Problem
The operator was crashing with the following error:
This occurred in the MCS registration process when
dc.ServerPreferredResources()failed, causing the entire operator pod to crash and restart.Solution
Register()function inpkg/mcs/register.goto handle any discovery error gracefullyNew Unit Tests in
pkg/mcs/register_test.goTestIsAvailable(): Tests MCS availability statusTestMCSSchemeGroupVersion(): Tests scheme group version initializationTestServiceExport(): Tests ServiceExport object creationTestServiceExportList(): Tests ServiceExportList object creationBenefits
Testing
Type of Change
Checklist
Related Issues
Fixes the operator crash issue when MCS discovery fails with "stale GroupVersion discovery" error.
Additional Notes
This fix ensures that MCS (Multi-Cluster Services) is treated as an optional feature. When MCS discovery fails for any reason, the operator gracefully marks MCS as unavailable and continues with normal MongoDB cluster operations. This makes the operator more resilient and prevents unnecessary pod restarts.