Skip to content

rbd: Register FenceController only once #5248

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 1 commit into from
Apr 1, 2025
Merged

rbd: Register FenceController only once #5248

merged 1 commit into from
Apr 1, 2025

Conversation

@monoamin
Copy link
Contributor

@monoamin monoamin commented Mar 31, 2025 •
edited
Loading

Running on Docker Swarm currently requires serving both the nodeserver and controllerserver over the same socket. This leads to errors like

FATAL: [core] grpc: Server.RegisterService found duplicate service registration for "fence.FenceController""

...since FenceController is registered once per server type.

The commit proposes a simple fix by registering FenceController only once when at least one of IsControllerServer or IsNodeServer is true.

Updates: #3769

@mergify mergify bot added the component/rbd Issues related to RBD label Mar 31, 2025
@monoamin
Copy link
Contributor Author

monoamin commented Apr 1, 2025

I compiled the patched version and tested it on a non-production Swarm. The plugin now starts up as expected, and the mentioned error does not appear.:

"I0401 05:50:50.020832 1 server.go:114] listening for CSI-Addons requests on address: &net.UnixAddr{Name:"/tmp/csi-addons.sock", Net:"unix"}"
"I0401 05:50:50.020902 1 server.go:125] Listening for connections on address: &net.UnixAddr{Name:"/run/docker/plugins/cephcsi.sock", Net:"unix"}"

nixpanic
nixpanic previously approved these changes Apr 1, 2025
View reviewed changes
Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me.

@nixpanic nixpanic requested a review from a team April 1, 2025 06:59
@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

Maybe the same thing is needed for CephFS too? I guess you'll find out later 👍

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

Hi @monoamin ,

There seem to be two issues with the commit:

  1. likely a tabs/spaces issue, run go fmt internal/rbd/driver/driver.go
  2. the commit body's lines must not be longer than 80 characters

Please amend the existing commit, and force-push it to your branch to update this PR.

Thanks for providing this fix, I hope it gets you get closer running Ceph-CSI on Docker Swarm 👏

@ppignet ppignet force-pushed the grpc-aio-k8sless branch from 0b956d5 to b8f3740 Compare April 1, 2025 07:34
@mergify mergify bot dismissed nixpanic’s stale review April 1, 2025 07:35

Pull request has been modified.

@monoamin
Copy link
Contributor Author

monoamin commented Apr 1, 2025 •
edited
Loading

@nixpanic Thanks, I updated it accordingly, please let me know if there are any further complications.

From what I gathered, the same patch is not needed for cephfs. The code seems to only register one FenceController, specifically within the setupCSIAddonsServer function.

Rakshith-R
Rakshith-R approved these changes Apr 1, 2025
View reviewed changes
Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

Thanks !

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

@Mergifyio rebase

@monoamin @nixpanic
rbd: Register FenceController only once
700eee1 
Running cephcsi in docker swarm currently requires serving both
the nodeserver and controllerserver over the same socket.
This leads to errors like

> FATAL: [core] grpc: Server.RegisterService found duplicate
> service registration for \"fence.FenceController\""

...since `FenceController` is registererd once per server type.

Commit proposes simple fix by registering `FenceController` only once
when at least one of `IsControllerServer` or `IsNodeServer` is `true`.

Signed-off-by: monoamin <[email protected]>
@mergify
Copy link
Contributor

mergify bot commented Apr 1, 2025

rebase

✅ Branch has been successfully rebased

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

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

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

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

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

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

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

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

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

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

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

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

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

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

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

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

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

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

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

/test ci/centos/upgrade-tests

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

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

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

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

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

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

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

/retest ci/centos/k8s-e2e-external-storage/1.30
/retest ci/centos/mini-e2e/k8s-1.31

Failed while deploying minikube.

@nixpanic
Copy link
Member

nixpanic commented Apr 1, 2025

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

Ceph MON failure of some kind (logs)

@mergify mergify bot merged commit 71decb8 into ceph:devel Apr 1, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nixpanic nixpanic nixpanic approved these changes

@Rakshith-R Rakshith-R Rakshith-R approved these changes

Assignees

No one assigned

Labels

component/rbd Issues related to RBD

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@monoamin @nixpanic @Rakshith-R