Skip to content

Conversation

@mergify
Copy link
Contributor

@mergify mergify bot commented Apr 14, 2025

Describe what this PR does

This commit fixes a bug in health checker that
caused shared checker to get keyed with volumeID+volumepath instead of just volumeID and the other way around
for non-shared checkers.

unit test runs:

rakshithr4@fedora:~/Workspace/github.com/ceph/ceph-csi$ /usr/local/go/bin/go test -timeout 30s -tags ceph_preview -run ^TestTwoNonSharedChecker$ githu
b.com/ceph/ceph-csi/internal/health-checker -v
=== RUN   TestTwoNonSharedChecker
=== PAUSE TestTwoNonSharedChecker
=== CONT  TestTwoNonSharedChecker
    manager_test.go:99: start the first checker
    manager_test.go:105: check health for first path, should be healthy
    manager_test.go:111: check health for second path, should error out since checker is not started
    manager_test.go:113: 
                Error Trace:    /home/rakshithr4/Workspace/github.com/ceph/ceph-csi/internal/health-checker/manager_test.go:113
                Error:          An error is expected but got nil.
                Test:           TestTwoNonSharedChecker
    manager_test.go:115: start the second checker
    manager_test.go:121: check health, should be healthy
    manager_test.go:127: stop the first checker
    manager_test.go:130: check health of second path, should still be healthy
    manager_test.go:136: stop the second checker
--- FAIL: TestTwoNonSharedChecker (0.00s)
FAIL
FAIL    github.com/ceph/ceph-csi/internal/health-checker        0.005s
FAIL
rakshithr4@fedora:~/Workspace/github.com/ceph/ceph-csi$ git diff
diff --git a/internal/health-checker/manager.go b/internal/health-checker/manager.go
index ed78ec6fa..6a08e6b3d 100644
--- a/internal/health-checker/manager.go
+++ b/internal/health-checker/manager.go
@@ -137,7 +137,8 @@ func (hcm *healthCheckManager) startStatChecker(volumeID, path string, shared bo
 // are key'd by theit volumeID+path.
 func (hcm *healthCheckManager) startChecker(cc ConditionChecker, volumeID, path string, shared bool) error {
        key := volumeID
-       if shared {
+       if !shared {
+               // if it is non-shared checker, try to use the path as well.
                key = fallbackKey(volumeID, path)
        }
 
diff --git a/internal/health-checker/manager_test.go b/internal/health-checker/manager_test.go
rakshithr4@fedora:~/Workspace/github.com/ceph/ceph-csi$ /usr/local/go/bin/go test -timeout 30s -tags ceph_preview -run ^TestTwoNonSharedChecker$ github.com/ceph/ceph-csi/internal/health-checker -v
=== RUN   TestTwoNonSharedChecker
=== PAUSE TestTwoNonSharedChecker
=== CONT  TestTwoNonSharedChecker
    manager_test.go:99: start the first checker
    manager_test.go:105: check health for first path, should be healthy
    manager_test.go:111: check health for second path, should error out since checker is not started
    manager_test.go:115: start the second checker
    manager_test.go:121: check health, should be healthy
    manager_test.go:127: stop the first checker
    manager_test.go:130: check health of second path, should still be healthy
    manager_test.go:136: stop the second checker
--- PASS: TestTwoNonSharedChecker (0.00s)
PASS
ok      github.com/ceph/ceph-csi/internal/health-checker        0.005s

Checklist:

  • Commit Message Formatting: Commit titles and messages follow
    guidelines in the developer
    guide
    .
  • Reviewed the developer guide on Submitting a Pull
    Request
  • Pending release
    notes

    updated with breaking and/or notable changes for the next major release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

  • /retest ci/centos/<job-name>: retest the <job-name> after unrelated
    failure (please report the failure too!)

This is an automatic backport of pull request #5272 done by [Mergify](https://mergify.com).

This commit fixes a bug in health checker that
caused shared checker to get keyed with volumeID+volumepath
instead of just volumeID and the other way around
for non-shared checkers.

Signed-off-by: Rakshith R <[email protected]>
(cherry picked from commit 86f2ad9)
@mergify mergify bot added ok-to-test Label to trigger E2E tests bug Something isn't working labels Apr 14, 2025
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.32

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.32

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.31

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.32

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.31

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.31

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Apr 14, 2025
@Rakshith-R Rakshith-R requested a review from a team April 15, 2025 09:21
Copy link
Member

@black-dragon74 black-dragon74 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@mergify mergify bot merged commit 90b0022 into release-v3.14 Apr 15, 2025
41 checks passed
@mergify mergify bot deleted the mergify/bp/release-v3.14/pr-5272 branch April 15, 2025 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants