Skip to content

Conversation

@Rakshith-R
Copy link
Contributor

@Rakshith-R Rakshith-R 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!)

@Rakshith-R Rakshith-R requested review from a team and nixpanic April 14, 2025 11:02
@Rakshith-R Rakshith-R added bug Something isn't working component/util Utility functions shared between CephFS and RBD labels Apr 14, 2025
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]>
@nixpanic nixpanic requested a review from a team April 14, 2025 12:51
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Apr 14, 2025

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Apr 14, 2025

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 86f2ad9

@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Apr 14, 2025
@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/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-helm/k8s-1.31

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Apr 14, 2025
@Madhu-1 Madhu-1 added the backport-to-release-v3.14 Label to backport from devel to release-v3.14 branch label Apr 14, 2025
@mergify mergify bot merged commit 86f2ad9 into ceph:devel Apr 14, 2025
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-to-release-v3.14 Label to backport from devel to release-v3.14 branch bug Something isn't working component/util Utility functions shared between CephFS and RBD

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants