Skip to content

smb: support custom ports#63924

Merged
adk3798 merged 14 commits intoceph:mainfrom
phlogistonjohn:jjm-smb-custom-ports
Jul 9, 2025
Merged

smb: support custom ports#63924
adk3798 merged 14 commits intoceph:mainfrom
phlogistonjohn:jjm-smb-custom-ports

Conversation

@phlogistonjohn
Copy link
Contributor

@phlogistonjohn phlogistonjohn commented Jun 13, 2025 •
edited
Loading

Add support for custom ports to the smb cluster resources and smb service spec.

Custom ports are meant to accomplish two things:

  1. Give administrators a way to control port binding for services on their cluster
  2. Allow smb colocation by creating multiple clusters with distinct sets of ports (to run >1 smb cluster per ceph cluster)

Updated docs have more detail, but in short a cluster can now be configured with a new optional custom_ports field like so:

resources:
-   resource_type: ceph.smb.cluster
    cluster_id: mycluster
    # ... other cluster fields ...
    custom_ports:
      smb: 4455
      smbmetrics: 9099
      ctdb: 9999

If a "service name" key is not given in custom_ports, and the feature is used, it will automatically revert to using the default port.

Previously, Windows systems did not have a good way to connect to SMB servers listtening on a port other than 445. However with recent versions of Windows using an alternative port is supported. https://github.com/MicrosoftDocs/windowsserverdocs/blob/main/WindowsServerDocs/storage/file-server/smb-ports.md

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands

@phlogistonjohn
cephadm: add custom smb port support to smb daemon
af82f57 
Add logic to help customize the smb, smbmetrics, and/or ctdb port.
In non-clustered mode the container runtime takes care of port mapping
to the host and only needs to update smb and smbmetrics.
In clustered mode the container runtime doesn't handle port mapping
and the ports the real daemons bind to matter. smbmetrics already
supports easy port configuration on the command line. smbd can be
configured to bind to a different port via the conf so that will
be later handled by the mgr module. ctdb needs a special option
in sambacc so alter the /etc/services file!

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
python-common/deployment: add custom port support to smb service spec
11d4ec2 
Add a new dictionary field `custom_ports` to the SMB service spec.
This field takes service names like `smb` or `ctdb` as string keys
and maps them to custom ports. If a key is not present in the dict
the default port for that "service" will be used.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
pybind/cephadm: update service discovery to use custom smb port
3b99fa2 
When smb is using custom ports the metrics port may not be
the default. Teach the service_discovery.py module about smb custom
ports and update a test to match.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/cephadm: update smb port management in smb.py
7122a51 
Remove the hard coded default smbmetrics port, to help break any legacy
code that might be trying to use it.
Ensure that all customized port values get passed down to the cephadm
binary.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add validation function for smb cluster custom ports
81a5081 
Add a function that will be used to validate the smb cluster's
custom port values (when provided).

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add a test case for custom port validation func
e7e13ad 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: update the cluster resource with a new custom_ports field
cec3bc6 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: update handler.py to make use of the cluster custom ports value
77b0412 
When a custom_ports field on the Cluster type is provided pass it
to the smb service spec and smb/sambacc configuration fields needed.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
cephadm: don't add --publish options to clustered instances
d4e08bf 
Clustered instances use host networking (for reasons) and nothing
benefits from having --publish options in the container cli
doing nothing and possibly being misleading (as there could be
way more ports bound than the container runtime shows).

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
doc/cephadm: document new smb service spec option custom_ports
5943770 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
doc/mgr: document new smb cluster resource option custom_ports
ac213f4 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
qa/cephadm/smb: add a test that uses custom ports
f07ae5e 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
qa/cephadm/smb: add test using custom ports to run two ctdb clusters
e6b5940 
Add a teuthology test that exercises basic colocation of two smb
clusters on the same ceph cluster by giving the 2nd cluster a
distinct set of ports.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
Copy link
Contributor Author

phlogistonjohn commented Jun 18, 2025

After a few fixes in the deploy_smb_mgr_ctdb_res_ports2c.yaml file the new teuthology tests are passing for me. Marking as Ready for review.

@phlogistonjohn phlogistonjohn marked this pull request as ready for review June 18, 2025 17:20
@phlogistonjohn phlogistonjohn requested review from a team as code owners June 18, 2025 17:20
@avanthakkar
Copy link
Contributor

avanthakkar commented Jun 19, 2025

jenkins test make check

avanthakkar
avanthakkar reviewed Jun 19, 2025
View reviewed changes
@phlogistonjohn
mgr/cephadm: add logging to service_discovery condition
17668d5 
Add a logger to service_discovery.py because a "typical" logger
was not present and use the logger to capture the hopefully
rare condition that service spec was not present for a daemon.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
Copy link
Contributor Author

phlogistonjohn commented Jun 20, 2025 •
edited
Loading

https://pulpito.ceph.com/phlogistonjohn-2025-06-20_14:07:21-orch:cephadm-wip-phlogistonjohn-testing-2025-06-18-1326-distro-default-gibba/

A run of the smb sub-suite. Relevant tests passed. One failure that seems unrelated to any test specific thing (host failed to come up after kernel upgrade). This build was based on the commit immediately before the patch adding logging.

@phlogistonjohn phlogistonjohn mentioned this pull request Jun 24, 2025
Merged
14 tasks
@phlogistonjohn
Copy link
Contributor Author

phlogistonjohn commented Jun 25, 2025

jenkins test make check arm64

@phlogistonjohn
Copy link
Contributor Author

phlogistonjohn commented Jun 25, 2025

@adk3798 please review and/or queue for regression testing in teuthology when you have a moment

anoopcs9
anoopcs9 reviewed Jun 26, 2025
View reviewed changes
Copy link
Contributor

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

In a non-clustered configuration, with smbd listening on 445 and 139 by default, we publish only 445 to the host. Whereas in clustered configuration, with host networking, both 445 and 139 are exposed. Introduction of custom ports does not alter the current behaviour but can ONLY accept a single port, although smb ports smb.conf parameter is capable of accepting more than one port.

I am not asking to make a change here to accept more than one port for 'smb' within custom_ports but rather food for thought.

spuiuk
spuiuk approved these changes Jun 26, 2025
View reviewed changes
Copy link
Member

@spuiuk spuiuk left a comment

Choose a reason for hiding this comment

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

ACK

@phlogistonjohn
Copy link
Contributor Author

phlogistonjohn commented Jun 26, 2025

@anoopcs9 thanks for pointing that out. I'd prefer to just not listen on 139 at all. AFAIK it's completely unnecessary for recent clients. Any technical objections to that?

@anthonyeleven
Copy link
Contributor

anthonyeleven commented Jun 26, 2025

Does the macOS client count as "recent"?

@anoopcs9
Copy link
Contributor

anoopcs9 commented Jun 26, 2025

@anoopcs9 thanks for pointing that out. I'd prefer to just not listen on 139 at all. AFAIK it's completely unnecessary for recent clients. Any technical objections to that?

I'm completely OK with that. May be I can look into extending the proposed change to accept only one to multiple ports for smb once we are done with the whole colocation work.

@phlogistonjohn
Copy link
Contributor Author

phlogistonjohn commented Jun 26, 2025

I'm completely OK with that.

Cool.

May be I can look into extending the proposed change to accept only one to multiple ports for smb once we are done with the whole colocation work.

I haven't verified this on a real system, but we would just need to specify smb ports = 445 when we are not customizing the smb port. Does that make sense?

@phlogistonjohn
Copy link
Contributor Author

phlogistonjohn commented Jun 26, 2025 •
edited
Loading

Does the macOS client count as "recent"?

I'd be shocked to find out that the supported versions of Mac OS (13-15 according to wikipedia) require port 139. Clients are permitted to try 139 and 445 but port 139 has been legacy for a very long time.

If you want to try and verify that it works with only 445 you can deploy a main or tentacle branch cluster without CTDB clustering. The existing container based networking only exposes 445 ... if you can connect to a share in that cluster from a mac we would know that it only needs 445. But I'm confident enough to not really need proof :-)

@anoopcs9
Copy link
Contributor

anoopcs9 commented Jun 27, 2025

May be I can look into extending the proposed change to accept only one to multiple ports for smb once we are done with the whole colocation work.

I haven't verified this on a real system, but we would just need to specify smb ports = 445 when we are not customizing the smb port. Does that make sense?

I think I know where you are going. Are you considering to remove the if cluster.is_clustered() and cluster.custom_ports: check while adding smb ports? If so, yes, smbd honours whatever is set for smb ports parameter. Additionally we could then remove the hard coded port publish for 445 from container_args().

Also the very first commit cephadm: add custom smb port support to smb daemon is not confined to just smb port. So you could drop 'smb' to mention cephadm: add custom port support to smb daemon.

@phlogistonjohn
Copy link
Contributor Author

phlogistonjohn commented Jun 27, 2025

I think I know where you are going. Are you considering to remove the if cluster.is_clustered() and cluster.custom_ports: check while adding smb ports? If so, yes, smbd honours whatever is set for smb ports parameter.

I haven't exactly written the code, but it sounds OK.

Additionally we could then remove the hard coded port publish for 445 from container_args().

But I'm not sure where you're going with this. We need to use --publish when we use container networking. Full stop. Otherwise nothing gets exposed outside the container.

Also the very first commit cephadm: add custom smb port support to smb daemon is not confined to just smb port. So you could drop 'smb' to mention cephadm: add custom port support to smb daemon.

OK, fair point. I will change it if I need to make any other substantial changes to this branch. Otherwise I don't think its misleading enough to be worth re-running make check and all the other heavy ceph tests over.

@phlogistonjohn
Copy link
Contributor Author

phlogistonjohn commented Jun 27, 2025

jenkins test make check arm64

@anoopcs9
Copy link
Contributor

anoopcs9 commented Jun 27, 2025

Additionally we could then remove the hard coded port publish for 445 from container_args().

But I'm not sure where you're going with this. We need to use --publish when we use container networking. Full stop. Otherwise nothing gets exposed outside the container.

Yes, we must publish whatever is the listening port inside the container. My thought was in the direction to replace Ports.SMB.value with self.cfg.smb_port if we could always add smb ports parameter in the configuration irrespective of the clustering mode (and customized ports). Obviously we won't be listening on 139 in that case. Actually this is what came to my mind when you previously asked about smb ports = 445.

@anoopcs9
Copy link
Contributor

anoopcs9 commented Jul 2, 2025

Additionally we could then remove the hard coded port publish for 445 from container_args().

But I'm not sure where you're going with this. We need to use --publish when we use container networking. Full stop. Otherwise nothing gets exposed outside the container.

Yes, we must publish whatever is the listening port inside the container. My thought was in the direction to replace Ports.SMB.value with self.cfg.smb_port if we could always add smb ports parameter in the configuration irrespective of the clustering mode (and customized ports). Obviously we won't be listening on 139 in that case. Actually this is what came to my mind when you previously asked about smb ports = 445.

The above suggestion was agreed (offline) to be taken in as a separate PR.

@phlogistonjohn
Copy link
Contributor Author

phlogistonjohn commented Jul 2, 2025

jenkins test make check arm64

@phlogistonjohn
Copy link
Contributor Author

phlogistonjohn commented Jul 2, 2025

@adk3798 ping. Please review and/or queue for testing and/or merge when you have a moment

@adk3798
Copy link
Contributor

adk3798 commented Jul 8, 2025

https://pulpito.ceph.com/adking-2025-06-25_15:21:10-orch:cephadm-wip-adk-testing-2025-06-24-1732-distro-default-smithi/

all SMB tests passed

@adk3798
Copy link
Contributor

adk3798 commented Jul 8, 2025

jenkins test make check arm64

@adk3798 adk3798 merged commit ef15ecb into ceph:main Jul 9, 2025
14 checks passed
@adk3798 adk3798 mentioned this pull request Jul 9, 2025
Merged
@phlogistonjohn phlogistonjohn deleted the jjm-smb-custom-ports branch July 10, 2025 14:14
@phlogistonjohn phlogistonjohn added the wip-spuiuk-tracking Sachin Prabhu - tracking label Sep 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anoopcs9 anoopcs9 anoopcs9 left review comments

@spuiuk spuiuk spuiuk approved these changes

@xhernandez xhernandez xhernandez approved these changes

@avanthakkar avanthakkar avanthakkar approved these changes

@adk3798 adk3798 adk3798 approved these changes

Assignees

No one assigned

Labels

cephadm documentation mgr pybind tests wip-adk-testing wip-phlogistonjohn-testing wip-spuiuk-tracking Sachin Prabhu - tracking

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@phlogistonjohn @avanthakkar @anthonyeleven @anoopcs9 @adk3798 @spuiuk @xhernandez