cephadm: implement smb daemon core deployment #54817

phlogistonjohn · 2023-12-06T20:30:23Z

Depends on #54722

Add an incomplete but largely viable SMB/Samba container daemon form
implementation to cephadm. Currently difficult to use as there's no mgr support - but I have tested it by hand rolling some JSON to feed to the ceph _orch deploy command.

See #55068 for some architectural description

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
- Docs to be updated later
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

github-actions · 2023-12-09T21:49:06Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

github-actions · 2024-01-05T02:04:10Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

phlogistonjohn · 2024-01-05T19:22:17Z

jenkins test make check

adk3798

This is still in draft, so not sure if you intended it to be reviewed, but I've done a pass over anyway. Didn't see anything that stuck out as an issue.

adk3798 · 2024-01-09T12:58:57Z

src/cephadm/cephadmlib/container_types.py

+            del args[idx]
+            del args[idx]


why is it we need to do this twice when the arg has no value?

Consider a list representing arguments like so:
a1 = ['--foo', '--speed=88', '--location=home', '--enable-flux']

Let's say we want to remove the --speed argument. If so we need to remove it's value too. idx first gets set to the position of an arg starting with --speed if speed is followed by the = OR if we do not expect the argument to have a value (has_value is false). We only need to remove the element at position idx

Thus a1 becomes:

idx = 1 a1 = ['--foo', '--speed=88', '--location=home', '--enable-flux'] del a1[idx] # idx is now ['--foo', '--speed=88', '--location=home', '--enable-flux']

Now consider a2:
a2 = ['--foo', '--speed', '88', '--location=home', '--enable-flux']
In this case we actually want to remove --speed and 88 because 88 is the argument to speed but found in a seperate list value.

So therefore:

idx = 2 a2 = ['--foo', '--speed', '88', '--location=home', '--enable-flux'] del a2[idx] del a2[idx] # a2 is now ['--foo', '--speed', '--enable-flux']

I will try to add some comments on this to make it clearer.

It would probably be a lot clearer if I change it to use slices instead. So I'll look into that too.

I think I see what you mean. I was only thinking in terms of args using an "=" or that didn't have values at all. I was thinking how you would differentiate between something like --boolean-flag-with-no-value --other-flag and --flag-with-value <value>, but I guess the fact that you're replacing a specific flag that MUST have an = if it has a value (at least the replacement version of it, which is coming from us so can be controlled) then you know beforehand whether the flag will have a value and the issue goes away.

adk3798 · 2024-01-09T13:01:37Z

src/cephadm/cephadmlib/container_types.py

+    name: str,
+    ns: Iterable[Namespace],
+) -> None:
+    """Update the args list to contain options that enable conatiner namespace


NIT conatiner -> container

Fixed. (I finally remembered to this this one)

src/cephadm/cephadmlib/daemons/smb.py

adk3798 · 2024-01-09T14:30:02Z

src/cephadm/cephadmlib/daemons/smb.py

+        # XXX: is this needed? if so, can this be simplified
+        if isinstance(ctx.container_engine, Podman):
+            ctx.container_engine.update_mounts(ctx, volume_mounts)


this seems to just be setting the /etc/hosts mount that we started doing to avoid name resolution issues caused by podman inserting its own stuff there. It definitely caused issues for things like iscsi and grafana, but not sure for smb.

Exactly, plus I don't know if it needs to be replicated for the various sidecars or not.

src/cephadm/cephadmlib/daemons/smb.py

adk3798 · 2024-01-09T14:40:54Z

src/cephadm/cephadmlib/daemons/smb.py

+            custom_dns=custom_dns,
+            domain_member=Features.DOMAIN.value in instance_features,
+            clustered=Features.CLUSTERED.value in instance_features,
+            samba_debug_level=6,


are there plans to make this configurable later? Seems the only place this gets set to an actual value that I can find is here where it's set to 6. I don't see it picking this up from the provided json or anything.

Yes. This needs to be changed at some point.

github-actions · 2024-01-31T18:49:31Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Update the type annotations to allow passing pathlib.Path objects to the makedirs function. All the calls makedirs uses already can accept Path objects. This causes mypy to accept calling makedirs with a Path argument and Paths are nice. Signed-off-by: John Mulligan <jmulligan@redhat.com>

The prepare_data_dir method is a general way for classes to prepare the data dir (e.g. `/var/lib/ceph/$FSID/$DAEMON_TYPE.$DAEMON_ID`) before containers will use it. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Instead of always climbing through an "if ladder" based on daemon type variables we will have the option of using the common method provided by container daemon form classes. This will initially be used by the smb daemon. I don't have the energy to refactor all the existing stuff at the moment. Signed-off-by: John Mulligan <jmulligan@redhat.com>

In the future, some sidecar containers will need to share namespaces with the primary container (or each other). Make it easy to set this up by creating a enable_shared_namespaces function and Namespace enum. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Signed-off-by: John Mulligan <jmulligan@redhat.com>

Add an incomplete but largely viable SMB/Samba container daemon form implementation to cephadm. Currently unused but it lays out some of the basics needed to create smb sharing using samba containers under cephadm orchestration. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Enable the use of the SMB container daemon form class by importing, and thus registering, it. Note that the only way to invoke this feature is by hand rolling some JSON to feed to the `ceph _orch deploy` command. Connecting this with the cephadm mgr module is left as a future task. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Signed-off-by: John Mulligan <jmulligan@redhat.com>

The not-a-real-fqdn hostname that the containers got were causing performance issues joining AD (and running testjoin and winbind). Define a virtual hostname that can be passed in from the service or automatically derived from the system's hostname. Signed-off-by: John Mulligan <jmulligan@redhat.com>

phlogistonjohn · 2024-02-12T20:20:55Z

@adk3798 and I discussed this PR today and since the current feedback is largely addressed and that it is fully a subset of #55068 - and that PR has docs and will gain teuthology tests soonish we don't need to keep this separate PR open.

github-actions bot added cephadm needs-rebase labels Dec 6, 2023

phlogistonjohn force-pushed the jjm-cephadm-smb-core branch from 9a1d897 to 8e41330 Compare December 13, 2023 16:11

github-actions bot removed the needs-rebase label Dec 13, 2023

phlogistonjohn force-pushed the jjm-cephadm-smb-core branch from 8e41330 to 308aa81 Compare December 15, 2023 14:58

github-actions bot added the needs-rebase label Jan 5, 2024

phlogistonjohn force-pushed the jjm-cephadm-smb-core branch from 308aa81 to 2550696 Compare January 5, 2024 16:00

github-actions bot removed the needs-rebase label Jan 5, 2024

phlogistonjohn mentioned this pull request Jan 5, 2024

cephadm: add a new SMB service to the mgr module #55068

Merged

15 tasks

phlogistonjohn changed the title ~~[WIP] cephadm: implement smb daemon core deployment~~ cephadm: implement smb daemon core deployment Jan 5, 2024

adk3798 reviewed Jan 9, 2024

View reviewed changes

phlogistonjohn force-pushed the jjm-cephadm-smb-core branch from 2550696 to ffeef73 Compare January 29, 2024 19:14

github-actions bot added the needs-rebase label Jan 31, 2024

phlogistonjohn force-pushed the jjm-cephadm-smb-core branch from ffeef73 to 8caddaf Compare February 5, 2024 18:19

github-actions bot removed the needs-rebase label Feb 5, 2024

phlogistonjohn added 3 commits February 12, 2024 13:52

cephadm: add a prepare_data_dir method to container daemon form

8f17617

The prepare_data_dir method is a general way for classes to prepare the data dir (e.g. `/var/lib/ceph/$FSID/$DAEMON_TYPE.$DAEMON_ID`) before containers will use it. Signed-off-by: John Mulligan <jmulligan@redhat.com>

phlogistonjohn force-pushed the jjm-cephadm-smb-core branch from 8caddaf to 030a5af Compare February 12, 2024 18:55

phlogistonjohn added 6 commits February 12, 2024 14:10

cephadm: add a default constant value for samba server container image

6f9b8e4

Signed-off-by: John Mulligan <jmulligan@redhat.com>

cephadm: add a basic deployment test for an smb daemon

a3e353b

Signed-off-by: John Mulligan <jmulligan@redhat.com>

phlogistonjohn force-pushed the jjm-cephadm-smb-core branch from 030a5af to c924491 Compare February 12, 2024 19:10

phlogistonjohn closed this Feb 12, 2024

phlogistonjohn deleted the jjm-cephadm-smb-core branch March 25, 2024 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cephadm: implement smb daemon core deployment #54817

cephadm: implement smb daemon core deployment #54817

phlogistonjohn commented Dec 6, 2023 •

edited

github-actions bot commented Dec 9, 2023

github-actions bot commented Jan 5, 2024

phlogistonjohn commented Jan 5, 2024

adk3798 left a comment

adk3798 Jan 9, 2024

phlogistonjohn Jan 9, 2024

phlogistonjohn Jan 9, 2024

adk3798 Jan 9, 2024

adk3798 Jan 9, 2024

phlogistonjohn Feb 12, 2024

adk3798 Jan 9, 2024

phlogistonjohn Jan 9, 2024

adk3798 Jan 9, 2024

phlogistonjohn Jan 9, 2024

github-actions bot commented Jan 31, 2024

phlogistonjohn commented Feb 12, 2024

cephadm: implement smb daemon core deployment #54817

cephadm: implement smb daemon core deployment #54817

Conversation

phlogistonjohn commented Dec 6, 2023 • edited

Checklist

github-actions bot commented Dec 9, 2023

github-actions bot commented Jan 5, 2024

phlogistonjohn commented Jan 5, 2024

adk3798 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 31, 2024

phlogistonjohn commented Feb 12, 2024

phlogistonjohn commented Dec 6, 2023 •

edited