Merge pull request #51863 from phlogistonjohn/jjm-cephadm-argument-spec

cephadm: add a new ArgumentSpec type for advanced argument handling

Reviewed-by: Adam King <adking@redhat.com>

doc/cephadm/services/index.rst

@@ -541,13 +541,60 @@ a spec like
 
 which would cause each mon daemon to be deployed with `--cpus=2`.
 
+There are two ways to express arguments in the ``extra_container_args`` list.
+To start, an item in the list can be a string. When passing an argument
+as a string and the string contains spaces, Cephadm will automatically split it
+into multiple arguments. For example, ``--cpus 2`` would become ``["--cpus",
+"2"]`` when processed. Example:
+
+.. code-block:: yaml
+
+    service_type: mon
+    service_name: mon
+    placement:
+      hosts:
+        - host1
+        - host2
+        - host3
+    extra_container_args:
+      - "--cpus 2"
+
+As an alternative, an item in the list can be an object (mapping) containing
+the required key "argument" and an optional key "split". The value associated
+with the ``argument`` key must be a single string. The value associated with
+the ``split`` key is a boolean value. The ``split`` key explicitly controls if
+spaces in the argument value cause the value to be split into multiple
+arguments. If ``split`` is true then Cephadm will automatically split the value
+into multiple arguments.  If ``split`` is false then spaces in the value will
+be retained in the argument.  The default, when ``split`` is not provided, is
+false. Examples:
+
+.. code-block:: yaml
+
+    service_type: mon
+    service_name: mon
+    placement:
+      hosts:
+        - tiebreaker
+    extra_container_args:
+      # No spaces, always treated as a single argument
+      - argument: "--timout=3000"
+      # Splitting explicitly disabled, one single argument
+      - argument: "--annotation=com.example.name=my favorite mon"
+        split: false
+      # Splitting explicitly enabled, will become two arguments
+      - argument: "--cpuset-cpus 1-3,7-11"
+        split: true
+      # Splitting implicitly disabled, one single argument
+      - argument: "--annotation=com.example.note=a simple example"
+
 Mounting Files with Extra Container Arguments
 ---------------------------------------------
 
 A common use case for extra container arguments is to mount additional
-files within the container. However, some intuitive formats for doing
-so can cause deployment to fail (see https://tracker.ceph.com/issues/57338).
-The recommended syntax for mounting a file with extra container arguments is:
+files within the container. Older versions of Ceph did not support spaces
+in arguments and therefore the examples below apply to the widest range
+of Ceph versions.
 
 .. code-block:: yaml
 
@@ -587,6 +634,55 @@ the node-exporter service , one could apply a service spec like
   extra_entrypoint_args:
     - "--collector.textfile.directory=/var/lib/node_exporter/textfile_collector2"
 
+There are two ways to express arguments in the ``extra_entrypoint_args`` list.
+To start, an item in the list can be a string. When passing an argument as a
+string and the string contains spaces, cephadm will automatically split it into
+multiple arguments. For example, ``--debug_ms 10`` would become
+``["--debug_ms", "10"]`` when processed. Example:
+
+.. code-block:: yaml
+
+    service_type: mon
+    service_name: mon
+    placement:
+      hosts:
+        - host1
+        - host2
+        - host3
+    extra_entrypoint_args:
+      - "--debug_ms 2"
+
+As an alternative, an item in the list can be an object (mapping) containing
+the required key "argument" and an optional key "split". The value associated
+with the ``argument`` key must be a single string. The value associated with
+the ``split`` key is a boolean value. The ``split`` key explicitly controls if
+spaces in the argument value cause the value to be split into multiple
+arguments. If ``split`` is true then cephadm will automatically split the value
+into multiple arguments.  If ``split`` is false then spaces in the value will
+be retained in the argument.  The default, when ``split`` is not provided, is
+false. Examples:
+
+.. code-block:: yaml
+
+    # An theoretical data migration service
+    service_type: pretend
+    service_name: imagine1
+    placement:
+      hosts:
+        - host1
+    extra_entrypoint_args:
+      # No spaces, always treated as a single argument
+      - argument: "--timout=30m"
+      # Splitting explicitly disabled, one single argument
+      - argument: "--import=/mnt/usb/My Documents"
+        split: false
+      # Splitting explicitly enabled, will become two arguments
+      - argument: "--tag documents"
+        split: true
+      # Splitting implicitly disabled, one single argument
+      - argument: "--title=Imported Documents"
+
+
 Custom Config Files
 ===================
 

src/pybind/mgr/cephadm/exchange.py

@@ -87,8 +87,8 @@ class DeployMeta:
         deployed_by: Optional[List[str]] = None,
         rank: Optional[int] = None,
         rank_generation: Optional[int] = None,
-        extra_container_args: Optional[List[str]] = None,
-        extra_entrypoint_args: Optional[List[str]] = None,
+        extra_container_args: Optional[List[Union[str, Dict[str, Any]]]] = None,
+        extra_entrypoint_args: Optional[List[Union[str, Dict[str, Any]]]] = None,
     ):
         self.data = dict(init_data or {})
         # set fields

src/pybind/mgr/cephadm/module.py

@@ -869,6 +869,9 @@ class CephadmOrchestrator(orchestrator.Orchestrator, MgrModule,
                 ssh_config_fname))
 
     def _process_ls_output(self, host: str, ls: List[Dict[str, Any]]) -> None:
+        def _as_datetime(value: Optional[str]) -> Optional[datetime.datetime]:
+            return str_to_datetime(value) if value is not None else None
+
         dm = {}
         for d in ls:
             if not d['style'].startswith('cephadm'):
@@ -877,51 +880,55 @@ class CephadmOrchestrator(orchestrator.Orchestrator, MgrModule,
                 continue
             if '.' not in d['name']:
                 continue
-            sd = orchestrator.DaemonDescription()
-            sd.last_refresh = datetime_now()
-            for k in ['created', 'started', 'last_configured', 'last_deployed']:
-                v = d.get(k, None)
-                if v:
-                    setattr(sd, k, str_to_datetime(d[k]))
-            sd.daemon_type = d['name'].split('.')[0]
-            if sd.daemon_type not in orchestrator.KNOWN_DAEMON_TYPES:
-                logger.warning(f"Found unknown daemon type {sd.daemon_type} on host {host}")
+            daemon_type = d['name'].split('.')[0]
+            if daemon_type not in orchestrator.KNOWN_DAEMON_TYPES:
+                logger.warning(f"Found unknown daemon type {daemon_type} on host {host}")
                 continue
 
-            sd.daemon_id = '.'.join(d['name'].split('.')[1:])
-            sd.hostname = host
-            sd.container_id = d.get('container_id')
-            if sd.container_id:
+            container_id = d.get('container_id')
+            if container_id:
                 # shorten the hash
-                sd.container_id = sd.container_id[0:12]
-            sd.container_image_name = d.get('container_image_name')
-            sd.container_image_id = d.get('container_image_id')
-            sd.container_image_digests = d.get('container_image_digests')
-            sd.memory_usage = d.get('memory_usage')
-            sd.memory_request = d.get('memory_request')
-            sd.memory_limit = d.get('memory_limit')
-            sd.cpu_percentage = d.get('cpu_percentage')
-            sd._service_name = d.get('service_name')
-            sd.deployed_by = d.get('deployed_by')
-            sd.version = d.get('version')
-            sd.ports = d.get('ports')
-            sd.ip = d.get('ip')
-            sd.rank = int(d['rank']) if d.get('rank') is not None else None
-            sd.rank_generation = int(d['rank_generation']) if d.get(
+                container_id = container_id[0:12]
+            rank = int(d['rank']) if d.get('rank') is not None else None
+            rank_generation = int(d['rank_generation']) if d.get(
                 'rank_generation') is not None else None
-            sd.extra_container_args = d.get('extra_container_args')
-            sd.extra_entrypoint_args = d.get('extra_entrypoint_args')
+            status, status_desc = None, 'unknown'
             if 'state' in d:
-                sd.status_desc = d['state']
-                sd.status = {
+                status_desc = d['state']
+                status = {
                     'running': DaemonDescriptionStatus.running,
                     'stopped': DaemonDescriptionStatus.stopped,
                     'error': DaemonDescriptionStatus.error,
                     'unknown': DaemonDescriptionStatus.error,
                 }[d['state']]
-            else:
-                sd.status_desc = 'unknown'
-                sd.status = None
+            sd = orchestrator.DaemonDescription(
+                daemon_type=daemon_type,
+                daemon_id='.'.join(d['name'].split('.')[1:]),
+                hostname=host,
+                container_id=container_id,
+                container_image_id=d.get('container_image_id'),
+                container_image_name=d.get('container_image_name'),
+                container_image_digests=d.get('container_image_digests'),
+                version=d.get('version'),
+                status=status,
+                status_desc=status_desc,
+                created=_as_datetime(d.get('created')),
+                started=_as_datetime(d.get('started')),
+                last_configured=_as_datetime(d.get('last_configured')),
+                last_deployed=_as_datetime(d.get('last_deployed')),
+                memory_usage=d.get('memory_usage'),
+                memory_request=d.get('memory_request'),
+                memory_limit=d.get('memory_limit'),
+                cpu_percentage=d.get('cpu_percentage'),
+                service_name=d.get('service_name'),
+                ports=d.get('ports'),
+                ip=d.get('ip'),
+                deployed_by=d.get('deployed_by'),
+                rank=rank,
+                rank_generation=rank_generation,
+                extra_container_args=d.get('extra_container_args'),
+                extra_entrypoint_args=d.get('extra_entrypoint_args'),
+            )
             dm[sd.name()] = sd
         self.log.debug('Refreshed host %s daemons (%d)' % (host, len(dm)))
         self.cache.update_host_daemons(host, dm)

src/pybind/mgr/cephadm/serve.py

@@ -10,7 +10,14 @@ from typing import TYPE_CHECKING, Optional, List, cast, Dict, Any, Union, Tuple,
 
 from ceph.deployment import inventory
 from ceph.deployment.drive_group import DriveGroupSpec
-from ceph.deployment.service_spec import ServiceSpec, CustomContainerSpec, PlacementSpec, RGWSpec
+from ceph.deployment.service_spec import (
+    ArgumentList,
+    ArgumentSpec,
+    CustomContainerSpec,
+    PlacementSpec,
+    RGWSpec,
+    ServiceSpec,
+)
 from ceph.utils import datetime_now
 
 import orchestrator
@@ -1284,8 +1291,12 @@ class CephadmServe:
                             deployed_by=self.mgr.get_active_mgr_digests(),
                             rank=daemon_spec.rank,
                             rank_generation=daemon_spec.rank_generation,
-                            extra_container_args=extra_container_args,
-                            extra_entrypoint_args=extra_entrypoint_args,
+                            extra_container_args=ArgumentSpec.map_json(
+                                extra_container_args,
+                            ),
+                            extra_entrypoint_args=ArgumentSpec.map_json(
+                                extra_entrypoint_args,
+                            ),
                         ),
                         config_blobs=daemon_spec.final_config,
                     ).dump_json_str(),
@@ -1333,19 +1344,21 @@ class CephadmServe:
                     self.mgr.cephadm_services[servict_type].post_remove(dd, is_failed_deploy=True)
                 raise
 
-    def _setup_extra_deployment_args(self, daemon_spec: CephadmDaemonDeploySpec, params: Dict[str, Any]) -> Tuple[CephadmDaemonDeploySpec, Optional[List[str]], Optional[List[str]]]:
+    def _setup_extra_deployment_args(
+        self,
+        daemon_spec: CephadmDaemonDeploySpec,
+        params: Dict[str, Any],
+    ) -> Tuple[CephadmDaemonDeploySpec, Optional[ArgumentList], Optional[ArgumentList]]:
         # this function is for handling any potential user specified
         # (in the service spec) extra runtime or entrypoint args for a daemon
         # we are going to deploy. Effectively just adds a set of extra args to
         # pass to the cephadm binary to indicate the daemon being deployed
         # needs extra runtime/entrypoint args. Returns the modified daemon spec
         # as well as what args were added (as those are included in unit.meta file)
-        def _to_args(lst: List[str]) -> List[str]:
+        def _to_args(lst: ArgumentList) -> List[str]:
             out: List[str] = []
-            for value in lst:
-                for arg in value.split(' '):
-                    if arg:
-                        out.append(arg)
+            for argspec in lst:
+                out.extend(argspec.to_args())
             return out
 
         try:

src/pybind/mgr/cephadm/services/cephadmservice.py

@@ -10,7 +10,14 @@ from typing import TYPE_CHECKING, List, Callable, TypeVar, \
 
 from mgr_module import HandleCommandResult, MonCommandFailed
 
-from ceph.deployment.service_spec import ServiceSpec, RGWSpec, CephExporterSpec, MONSpec
+from ceph.deployment.service_spec import (
+    ArgumentList,
+    CephExporterSpec,
+    GeneralArgList,
+    MONSpec,
+    RGWSpec,
+    ServiceSpec,
+)
 from ceph.deployment.utils import is_ipv6, unwrap_ipv6
 from mgr_util import build_url, merge_dicts
 from orchestrator import OrchestratorError, DaemonDescription, DaemonDescriptionStatus
@@ -61,8 +68,8 @@ class CephadmDaemonDeploySpec:
                  ports: Optional[List[int]] = None,
                  rank: Optional[int] = None,
                  rank_generation: Optional[int] = None,
-                 extra_container_args: Optional[List[str]] = None,
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[ArgumentList] = None,
+                 extra_entrypoint_args: Optional[ArgumentList] = None,
                  ):
         """
         A data struction to encapsulate `cephadm deploy ...
@@ -103,6 +110,15 @@ class CephadmDaemonDeploySpec:
         self.extra_container_args = extra_container_args
         self.extra_entrypoint_args = extra_entrypoint_args
 
+    def __setattr__(self, name: str, value: Any) -> None:
+        if value is not None and name in ('extra_container_args', 'extra_entrypoint_args'):
+            for v in value:
+                tname = str(type(v))
+                if 'ArgumentSpec' not in tname:
+                    raise TypeError(f"{name} is not all ArgumentSpec values: {v!r}(is {type(v)} in {value!r}")
+
+        super().__setattr__(name, value)
+
     def name(self) -> str:
         return '%s.%s' % (self.daemon_type, self.daemon_id)
 
@@ -146,8 +162,8 @@ class CephadmDaemonDeploySpec:
             ports=self.ports,
             rank=self.rank,
             rank_generation=self.rank_generation,
-            extra_container_args=self.extra_container_args,
-            extra_entrypoint_args=self.extra_entrypoint_args,
+            extra_container_args=cast(GeneralArgList, self.extra_container_args),
+            extra_entrypoint_args=cast(GeneralArgList, self.extra_entrypoint_args),
         )
 
     @property

src/pybind/mgr/cephadm/tests/test_cephadm.py

@@ -16,9 +16,17 @@ try:
 except ImportError:
     pass
 
-from ceph.deployment.service_spec import ServiceSpec, PlacementSpec, RGWSpec, \
-    NFSServiceSpec, IscsiServiceSpec, HostPlacementSpec, CustomContainerSpec, MDSSpec, \
-    CustomConfig
+from ceph.deployment.service_spec import (
+    CustomConfig,
+    CustomContainerSpec,
+    HostPlacementSpec,
+    IscsiServiceSpec,
+    MDSSpec,
+    NFSServiceSpec,
+    PlacementSpec,
+    RGWSpec,
+    ServiceSpec,
+)
 from ceph.deployment.drive_selection.selector import DriveSelection
 from ceph.deployment.inventory import Devices, Device
 from ceph.utils import datetime_to_str, datetime_now

src/pybind/mgr/orchestrator/_interface.py

@@ -30,8 +30,19 @@ except ImportError:
 import yaml
 
 from ceph.deployment import inventory
-from ceph.deployment.service_spec import ServiceSpec, NFSServiceSpec, RGWSpec, \
-    IscsiServiceSpec, IngressSpec, SNMPGatewaySpec, MDSSpec, TunedProfileSpec
+from ceph.deployment.service_spec import (
+    ArgumentList,
+    ArgumentSpec,
+    GeneralArgList,
+    IngressSpec,
+    IscsiServiceSpec,
+    MDSSpec,
+    NFSServiceSpec,
+    RGWSpec,
+    SNMPGatewaySpec,
+    ServiceSpec,
+    TunedProfileSpec,
+)
 from ceph.deployment.drive_group import DriveGroupSpec
 from ceph.deployment.hostspec import HostSpec, SpecValidationError
 from ceph.utils import datetime_to_str, str_to_datetime
@@ -932,8 +943,8 @@ class DaemonDescription(object):
                  deployed_by: Optional[List[str]] = None,
                  rank: Optional[int] = None,
                  rank_generation: Optional[int] = None,
-                 extra_container_args: Optional[List[str]] = None,
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
+                 extra_entrypoint_args: Optional[GeneralArgList] = None,
                  ) -> None:
 
         #: Host is at the same granularity as InventoryHost
@@ -998,8 +1009,23 @@ class DaemonDescription(object):
 
         self.is_active = is_active
 
-        self.extra_container_args = extra_container_args
-        self.extra_entrypoint_args = extra_entrypoint_args
+        self.extra_container_args: Optional[ArgumentList] = None
+        self.extra_entrypoint_args: Optional[ArgumentList] = None
+        if extra_container_args:
+            self.extra_container_args = ArgumentSpec.from_general_args(
+                extra_container_args)
+        if extra_entrypoint_args:
+            self.extra_entrypoint_args = ArgumentSpec.from_general_args(
+                extra_entrypoint_args)
+
+    def __setattr__(self, name: str, value: Any) -> None:
+        if value is not None and name in ('extra_container_args', 'extra_entrypoint_args'):
+            for v in value:
+                tname = str(type(v))
+                if 'ArgumentSpec' not in tname:
+                    raise TypeError(f"{name} is not all ArgumentSpec values: {v!r}(is {type(v)} in {value!r}")
+
+        super().__setattr__(name, value)
 
     @property
     def status(self) -> Optional[DaemonDescriptionStatus]:

src/python-common/ceph/deployment/drive_group.py

@@ -2,7 +2,12 @@ import enum
 import yaml
 
 from ceph.deployment.inventory import Device
-from ceph.deployment.service_spec import ServiceSpec, PlacementSpec, CustomConfig
+from ceph.deployment.service_spec import (
+    CustomConfig,
+    GeneralArgList,
+    PlacementSpec,
+    ServiceSpec,
+)
 from ceph.deployment.hostspec import SpecValidationError
 
 try:
@@ -190,8 +195,8 @@ class DriveGroupSpec(ServiceSpec):
                  unmanaged=False,  # type: bool
                  filter_logic='AND',  # type: str
                  preview_only=False,  # type: bool
-                 extra_container_args=None,  # type: Optional[List[str]]
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
+                 extra_entrypoint_args: Optional[GeneralArgList] = None,
                  data_allocate_fraction=None,  # type: Optional[float]
                  method=None,  # type: Optional[OSDMethod]
                  config=None,  # type: Optional[Dict[str, str]]

src/python-common/ceph/deployment/service_spec.py

@@ -486,6 +486,132 @@ def service_spec_allow_invalid_from_json() -> Iterator[None]:
     _service_spec_from_json_validate = True
 
 
+class ArgumentSpec:
+    """The ArgumentSpec type represents an argument that can be
+    passed to an underyling subsystem, like a container engine or
+    another command line tool.
+
+    The ArgumentSpec aims to be backwards compatible with the previous
+    form of argument, a single string. The string was always assumed
+    to be indentended to be split on spaces. For example:
+    `--cpus 8` becomes `["--cpus", "8"]`. This type is converted from
+    either a string or an json/yaml object. In the object form you
+    can choose if the string part should be split so an argument like
+    `--migrate-from=//192.168.5.22/My Documents` can be expressed.
+    """
+    _fields = ['argument', 'split']
+
+    class OriginalType(enum.Enum):
+        OBJECT = 0
+        STRING = 1
+
+    def __init__(
+        self,
+        argument: str,
+        split: bool = False,
+        *,
+        origin: OriginalType = OriginalType.OBJECT,
+    ) -> None:
+        self.argument = argument
+        self.split = bool(split)
+        # origin helps with round-tripping between inputs that
+        # are simple strings or objects (dicts)
+        self._origin = origin
+        self.validate()
+
+    def to_json(self) -> Union[str, Dict[str, Any]]:
+        """Return a json-safe represenation of the ArgumentSpec."""
+        if self._origin == self.OriginalType.STRING:
+            return self.argument
+        return {
+            'argument': self.argument,
+            'split': self.split,
+        }
+
+    def to_args(self) -> List[str]:
+        """Convert this ArgumentSpec into a list of arguments suitable for
+        adding to an argv-style command line.
+        """
+        if not self.split:
+            return [self.argument]
+        return [part for part in self.argument.split(" ") if part]
+
+    def __eq__(self, other: Any) -> bool:
+        if isinstance(other, ArgumentSpec):
+            return (
+                self.argument == other.argument
+                and self.split == other.split
+            )
+        if isinstance(other, object):
+            # This is a workaround for silly ceph mgr object/type identity
+            # mismatches due to multiple python interpreters in use.
+            try:
+                argument = getattr(other, 'argument')
+                split = getattr(other, 'split')
+                return (self.argument == argument and self.split == split)
+            except AttributeError:
+                pass
+        return NotImplemented
+
+    def __repr__(self) -> str:
+        return f'ArgumentSpec({self.argument!r}, {self.split!r})'
+
+    def validate(self) -> None:
+        if not isinstance(self.argument, str):
+            raise SpecValidationError(
+                    f'ArgumentSpec argument must be a string. Got {type(self.argument)}')
+        if not isinstance(self.split, bool):
+            raise SpecValidationError(
+                    f'ArgumentSpec split must be a boolean. Got {type(self.split)}')
+
+    @classmethod
+    def from_json(cls, data: Union[str, Dict[str, Any]]) -> "ArgumentSpec":
+        """Convert a json-object (dict) to an ArgumentSpec."""
+        if isinstance(data, str):
+            return cls(data, split=True, origin=cls.OriginalType.STRING)
+        if 'argument' not in data:
+            raise SpecValidationError(f'ArgumentSpec must have an "argument" field')
+        for k in data.keys():
+            if k not in cls._fields:
+                raise SpecValidationError(f'ArgumentSpec got an unknown field {k!r}')
+        return cls(**data)
+
+    @staticmethod
+    def map_json(
+        values: Optional["ArgumentList"]
+    ) -> Optional[List[Union[str, Dict[str, Any]]]]:
+        """Given a list of ArgumentSpec objects return a json-safe
+        representation.of them."""
+        if values is None:
+            return None
+        return [v.to_json() for v in values]
+
+    @classmethod
+    def from_general_args(cls, data: "GeneralArgList") -> "ArgumentList":
+        """Convert a list of strs, dicts, or existing ArgumentSpec objects
+        to a list of only ArgumentSpec objects.
+        """
+        out: ArgumentList = []
+        for item in data:
+            if isinstance(item, (str, dict)):
+                out.append(cls.from_json(item))
+            elif isinstance(item, cls):
+                out.append(item)
+            elif hasattr(item, 'to_json'):
+                # This is a workaround for silly ceph mgr object/type identity
+                # mismatches due to multiple python interpreters in use.
+                # It should be safe because we already have to be able to
+                # round-trip between json/yaml.
+                out.append(cls.from_json(item.to_json()))
+            else:
+                raise SpecValidationError(f"Unknown type for argument: {type(item)}")
+        return out
+
+
+ArgumentList = List[ArgumentSpec]
+GeneralArgList = List[Union[str, Dict[str, Any], "ArgumentSpec"]]
+
+
 class ServiceSpec(object):
     """
     Details of service creation.
@@ -560,8 +686,8 @@ class ServiceSpec(object):
                  unmanaged: bool = False,
                  preview_only: bool = False,
                  networks: Optional[List[str]] = None,
-                 extra_container_args: Optional[List[str]] = None,
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
+                 extra_entrypoint_args: Optional[GeneralArgList] = None,
                  custom_configs: Optional[List[CustomConfig]] = None,
                  ):
 
@@ -601,10 +727,27 @@ class ServiceSpec(object):
         if config:
             self.config = {k.replace(' ', '_'): v for k, v in config.items()}
 
-        self.extra_container_args: Optional[List[str]] = extra_container_args
-        self.extra_entrypoint_args: Optional[List[str]] = extra_entrypoint_args
+        self.extra_container_args: Optional[ArgumentList] = None
+        self.extra_entrypoint_args: Optional[ArgumentList] = None
+        if extra_container_args:
+            self.extra_container_args = ArgumentSpec.from_general_args(
+                extra_container_args)
+        if extra_entrypoint_args:
+            self.extra_entrypoint_args = ArgumentSpec.from_general_args(
+                extra_entrypoint_args)
         self.custom_configs: Optional[List[CustomConfig]] = custom_configs
 
+    def __setattr__(self, name: str, value: Any) -> None:
+        if value is not None and name in ('extra_container_args', 'extra_entrypoint_args'):
+            for v in value:
+                tname = str(type(v))
+                if 'ArgumentSpec' not in tname:
+                    raise TypeError(
+                        f"{name} is not all ArgumentSpec values:"
+                        f" {v!r}(is {type(v)} in {value!r}")
+
+        super().__setattr__(name, value)
+
     @classmethod
     @handle_type_error
     def from_json(cls: Type[ServiceSpecT], json_spec: Dict) -> ServiceSpecT:
@@ -730,9 +873,13 @@ class ServiceSpec(object):
         if self.networks:
             ret['networks'] = self.networks
         if self.extra_container_args:
-            ret['extra_container_args'] = self.extra_container_args
+            ret['extra_container_args'] = ArgumentSpec.map_json(
+                self.extra_container_args
+            )
         if self.extra_entrypoint_args:
-            ret['extra_entrypoint_args'] = self.extra_entrypoint_args
+            ret['extra_entrypoint_args'] = ArgumentSpec.map_json(
+                self.extra_entrypoint_args
+            )
         if self.custom_configs:
             ret['custom_configs'] = [c.to_json() for c in self.custom_configs]
 
@@ -812,8 +959,8 @@ class NFSServiceSpec(ServiceSpec):
                  port: Optional[int] = None,
                  virtual_ip: Optional[str] = None,
                  enable_haproxy_protocol: bool = False,
-                 extra_container_args: Optional[List[str]] = None,
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
+                 extra_entrypoint_args: Optional[GeneralArgList] = None,
                  custom_configs: Optional[List[CustomConfig]] = None,
                  ):
         assert service_type == 'nfs'
@@ -884,8 +1031,8 @@ class RGWSpec(ServiceSpec):
                  config: Optional[Dict[str, str]] = None,
                  networks: Optional[List[str]] = None,
                  subcluster: Optional[str] = None,  # legacy, only for from_json on upgrade
-                 extra_container_args: Optional[List[str]] = None,
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
+                 extra_entrypoint_args: Optional[GeneralArgList] = None,
                  custom_configs: Optional[List[CustomConfig]] = None,
                  rgw_realm_token: Optional[str] = None,
                  update_endpoints: Optional[bool] = False,
@@ -978,8 +1125,8 @@ class IscsiServiceSpec(ServiceSpec):
                  preview_only: bool = False,
                  config: Optional[Dict[str, str]] = None,
                  networks: Optional[List[str]] = None,
-                 extra_container_args: Optional[List[str]] = None,
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
+                 extra_entrypoint_args: Optional[GeneralArgList] = None,
                  custom_configs: Optional[List[CustomConfig]] = None,
                  ):
         assert service_type == 'iscsi'
@@ -1057,8 +1204,8 @@ class IngressSpec(ServiceSpec):
                  ssl: bool = False,
                  keepalive_only: bool = False,
                  enable_haproxy_protocol: bool = False,
-                 extra_container_args: Optional[List[str]] = None,
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
+                 extra_entrypoint_args: Optional[GeneralArgList] = None,
                  custom_configs: Optional[List[CustomConfig]] = None,
                  ):
         assert service_type == 'ingress'
@@ -1135,11 +1282,12 @@ class CustomContainerSpec(ServiceSpec):
                  preview_only: bool = False,
                  image: Optional[str] = None,
                  entrypoint: Optional[str] = None,
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_entrypoint_args: Optional[GeneralArgList] = None,
                  uid: Optional[int] = None,
                  gid: Optional[int] = None,
                  volume_mounts: Optional[Dict[str, str]] = {},
-                 args: Optional[List[str]] = [],  # args for the container runtime, not entrypoint
+                 # args are for the container runtime, not entrypoint
+                 args: Optional[GeneralArgList] = [],
                  envs: Optional[List[str]] = [],
                  privileged: Optional[bool] = False,
                  bind_mounts: Optional[List[List[str]]] = None,
@@ -1202,8 +1350,8 @@ class MonitoringSpec(ServiceSpec):
                  unmanaged: bool = False,
                  preview_only: bool = False,
                  port: Optional[int] = None,
-                 extra_container_args: Optional[List[str]] = None,
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
+                 extra_entrypoint_args: Optional[GeneralArgList] = None,
                  custom_configs: Optional[List[CustomConfig]] = None,
                  ):
         assert service_type in ['grafana', 'node-exporter', 'prometheus', 'alertmanager',
@@ -1250,8 +1398,8 @@ class AlertManagerSpec(MonitoringSpec):
                  networks: Optional[List[str]] = None,
                  port: Optional[int] = None,
                  secure: bool = False,
-                 extra_container_args: Optional[List[str]] = None,
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
+                 extra_entrypoint_args: Optional[GeneralArgList] = None,
                  custom_configs: Optional[List[CustomConfig]] = None,
                  ):
         assert service_type == 'alertmanager'
@@ -1306,8 +1454,8 @@ class GrafanaSpec(MonitoringSpec):
                  protocol: Optional[str] = 'https',
                  initial_admin_password: Optional[str] = None,
                  anonymous_access: Optional[bool] = True,
-                 extra_container_args: Optional[List[str]] = None,
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
+                 extra_entrypoint_args: Optional[GeneralArgList] = None,
                  custom_configs: Optional[List[CustomConfig]] = None,
                  ):
         assert service_type == 'grafana'
@@ -1350,8 +1498,8 @@ class PrometheusSpec(MonitoringSpec):
                  port: Optional[int] = None,
                  retention_time: Optional[str] = None,
                  retention_size: Optional[str] = None,
-                 extra_container_args: Optional[List[str]] = None,
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
+                 extra_entrypoint_args: Optional[GeneralArgList] = None,
                  custom_configs: Optional[List[CustomConfig]] = None,
                  ):
         assert service_type == 'prometheus'
@@ -1424,8 +1572,8 @@ class SNMPGatewaySpec(ServiceSpec):
                  unmanaged: bool = False,
                  preview_only: bool = False,
                  port: Optional[int] = None,
-                 extra_container_args: Optional[List[str]] = None,
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
+                 extra_entrypoint_args: Optional[GeneralArgList] = None,
                  custom_configs: Optional[List[CustomConfig]] = None,
                  ):
         assert service_type == 'snmp-gateway'
@@ -1547,8 +1695,8 @@ class MDSSpec(ServiceSpec):
                  config: Optional[Dict[str, str]] = None,
                  unmanaged: bool = False,
                  preview_only: bool = False,
-                 extra_container_args: Optional[List[str]] = None,
-                 extra_entrypoint_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
+                 extra_entrypoint_args: Optional[GeneralArgList] = None,
                  custom_configs: Optional[List[CustomConfig]] = None,
                  ):
         assert service_type == 'mds'
@@ -1581,7 +1729,7 @@ class MONSpec(ServiceSpec):
                  unmanaged: bool = False,
                  preview_only: bool = False,
                  networks: Optional[List[str]] = None,
-                 extra_container_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
                  custom_configs: Optional[List[CustomConfig]] = None,
                  crush_locations: Optional[Dict[str, List[str]]] = None,
                  ):
@@ -1741,7 +1889,7 @@ class CephExporterSpec(ServiceSpec):
                  placement: Optional[PlacementSpec] = None,
                  unmanaged: bool = False,
                  preview_only: bool = False,
-                 extra_container_args: Optional[List[str]] = None,
+                 extra_container_args: Optional[GeneralArgList] = None,
                  ):
         assert service_type == 'ceph-exporter'
 

src/python-common/ceph/tests/test_service_spec.py

@@ -6,9 +6,19 @@ import yaml
 
 import pytest
 
-from ceph.deployment.service_spec import HostPlacementSpec, PlacementSpec, \
-    ServiceSpec, RGWSpec, NFSServiceSpec, IscsiServiceSpec, AlertManagerSpec, \
-    CustomContainerSpec, GrafanaSpec, PrometheusSpec
+from ceph.deployment.service_spec import (
+    AlertManagerSpec,
+    ArgumentSpec,
+    CustomContainerSpec,
+    GrafanaSpec,
+    HostPlacementSpec,
+    IscsiServiceSpec,
+    NFSServiceSpec,
+    PlacementSpec,
+    PrometheusSpec,
+    RGWSpec,
+    ServiceSpec,
+)
 from ceph.deployment.drive_group import DriveGroupSpec
 from ceph.deployment.hostspec import SpecValidationError
 
@@ -964,3 +974,296 @@ def test_service_spec_validation_error(y, error_match):
     with pytest.raises(SpecValidationError) as err:
         specObj = ServiceSpec.from_json(data)
     assert err.match(error_match)
+
+
+@pytest.mark.parametrize("y, ec_args, ee_args, ec_final_args, ee_final_args", [
+    pytest.param("""
+service_type: container
+service_id: hello-world
+service_name: container.hello-world
+spec:
+  args:
+  - --foo
+  bind_mounts:
+  - - type=bind
+    - source=lib/modules
+    - destination=/lib/modules
+    - ro=true
+  dirs:
+  - foo
+  - bar
+  entrypoint: /usr/bin/bash
+  envs:
+  - FOO=0815
+  files:
+    bar.conf:
+    - foo
+    - bar
+    foo.conf: 'foo
+
+      bar'
+  gid: 2000
+  image: docker.io/library/hello-world:latest
+  ports:
+  - 8080
+  - 8443
+  uid: 1000
+  volume_mounts:
+    foo: /foo
+""",
+    None,
+    None,
+    None,
+    None,
+    id="no_extra_args"),
+    pytest.param("""
+service_type: container
+service_id: hello-world
+service_name: container.hello-world
+spec:
+  args:
+  - --foo
+  extra_entrypoint_args:
+  - "--lasers=blue"
+  - "--enable-confetti"
+  bind_mounts:
+  - - type=bind
+    - source=lib/modules
+    - destination=/lib/modules
+    - ro=true
+  dirs:
+  - foo
+  - bar
+  entrypoint: /usr/bin/bash
+  envs:
+  - FOO=0815
+  files:
+    bar.conf:
+    - foo
+    - bar
+    foo.conf: 'foo
+
+      bar'
+  gid: 2000
+  image: docker.io/library/hello-world:latest
+  ports:
+  - 8080
+  - 8443
+  uid: 1000
+  volume_mounts:
+    foo: /foo
+""",
+    None,
+    ["--lasers=blue", "--enable-confetti"],
+    None,
+    ["--lasers=blue", "--enable-confetti"],
+    id="only_extra_entrypoint_args_spec"),
+    pytest.param("""
+service_type: container
+service_id: hello-world
+service_name: container.hello-world
+spec:
+  args:
+  - --foo
+  bind_mounts:
+  - - type=bind
+    - source=lib/modules
+    - destination=/lib/modules
+    - ro=true
+  dirs:
+  - foo
+  - bar
+  entrypoint: /usr/bin/bash
+  envs:
+  - FOO=0815
+  files:
+    bar.conf:
+    - foo
+    - bar
+    foo.conf: 'foo
+
+      bar'
+  gid: 2000
+  image: docker.io/library/hello-world:latest
+  ports:
+  - 8080
+  - 8443
+  uid: 1000
+  volume_mounts:
+    foo: /foo
+extra_entrypoint_args:
+- "--lasers blue"
+- "--enable-confetti"
+""",
+    None,
+    ["--lasers blue", "--enable-confetti"],
+    None,
+    ["--lasers", "blue", "--enable-confetti"],
+    id="only_extra_entrypoint_args_toplevel"),
+    pytest.param("""
+service_type: nfs
+service_id: mynfs
+service_name: nfs.mynfs
+spec:
+  port: 1234
+  extra_entrypoint_args:
+  - "--lasers=blue"
+  - "--title=Custom NFS Options"
+  extra_container_args:
+  - "--cap-add=CAP_NET_BIND_SERVICE"
+  - "--oom-score-adj=12"
+""",
+    ["--cap-add=CAP_NET_BIND_SERVICE", "--oom-score-adj=12"],
+    ["--lasers=blue", "--title=Custom NFS Options"],
+    ["--cap-add=CAP_NET_BIND_SERVICE", "--oom-score-adj=12"],
+    ["--lasers=blue", "--title=Custom", "NFS", "Options"],
+    id="both_kinds_nfs"),
+    pytest.param("""
+service_type: container
+service_id: hello-world
+service_name: container.hello-world
+spec:
+  args:
+  - --foo
+  bind_mounts:
+  - - type=bind
+    - source=lib/modules
+    - destination=/lib/modules
+    - ro=true
+  dirs:
+  - foo
+  - bar
+  entrypoint: /usr/bin/bash
+  envs:
+  - FOO=0815
+  files:
+    bar.conf:
+    - foo
+    - bar
+    foo.conf: 'foo
+
+      bar'
+  gid: 2000
+  image: docker.io/library/hello-world:latest
+  ports:
+  - 8080
+  - 8443
+  uid: 1000
+  volume_mounts:
+    foo: /foo
+extra_entrypoint_args:
+- argument: "--lasers=blue"
+  split: true
+- argument: "--enable-confetti"
+""",
+    None,
+    [
+        {"argument": "--lasers=blue", "split": True},
+        {"argument": "--enable-confetti", "split": False},
+    ],
+    None,
+    [
+        "--lasers=blue",
+        "--enable-confetti",
+    ],
+    id="only_extra_entrypoint_args_obj_toplevel"),
+    pytest.param("""
+service_type: container
+service_id: hello-world
+service_name: container.hello-world
+spec:
+  args:
+  - --foo
+  bind_mounts:
+  - - type=bind
+    - source=lib/modules
+    - destination=/lib/modules
+    - ro=true
+  dirs:
+  - foo
+  - bar
+  entrypoint: /usr/bin/bash
+  envs:
+  - FOO=0815
+  files:
+    bar.conf:
+    - foo
+    - bar
+    foo.conf: 'foo
+
+      bar'
+  gid: 2000
+  image: docker.io/library/hello-world:latest
+  ports:
+  - 8080
+  - 8443
+  uid: 1000
+  volume_mounts:
+    foo: /foo
+  extra_entrypoint_args:
+  - argument: "--lasers=blue"
+    split: true
+  - argument: "--enable-confetti"
+""",
+    None,
+    [
+        {"argument": "--lasers=blue", "split": True},
+        {"argument": "--enable-confetti", "split": False},
+    ],
+    None,
+    [
+        "--lasers=blue",
+        "--enable-confetti",
+    ],
+    id="only_extra_entrypoint_args_obj_indented"),
+    pytest.param("""
+service_type: nfs
+service_id: mynfs
+service_name: nfs.mynfs
+spec:
+  port: 1234
+extra_entrypoint_args:
+- argument: "--lasers=blue"
+- argument: "--title=Custom NFS Options"
+extra_container_args:
+- argument: "--cap-add=CAP_NET_BIND_SERVICE"
+- argument: "--oom-score-adj=12"
+""",
+    [
+        {"argument": "--cap-add=CAP_NET_BIND_SERVICE", "split": False},
+        {"argument": "--oom-score-adj=12", "split": False},
+    ],
+    [
+        {"argument": "--lasers=blue", "split": False},
+        {"argument": "--title=Custom NFS Options", "split": False},
+    ],
+    [
+        "--cap-add=CAP_NET_BIND_SERVICE",
+        "--oom-score-adj=12",
+    ],
+    [
+        "--lasers=blue",
+        "--title=Custom NFS Options",
+    ],
+    id="both_kinds_obj_nfs"),
+])
+def test_extra_args_handling(y, ec_args, ee_args, ec_final_args, ee_final_args):
+    data = yaml.safe_load(y)
+    spec_obj = ServiceSpec.from_json(data)
+
+    assert ArgumentSpec.map_json(spec_obj.extra_container_args) == ec_args
+    assert ArgumentSpec.map_json(spec_obj.extra_entrypoint_args) == ee_args
+    if ec_final_args is None:
+        assert spec_obj.extra_container_args is None
+    else:
+        ec_res = []
+        for args in spec_obj.extra_container_args:
+            ec_res.extend(args.to_args())
+        assert ec_res == ec_final_args
+    if ee_final_args is None:
+        assert spec_obj.extra_entrypoint_args is None
+    else:
+        ee_res = []
+        for args in spec_obj.extra_entrypoint_args:
+            ee_res.extend(args.to_args())
+        assert ee_res == ee_final_args