mgr/cephadm: validating service_id for MDS

[rkachach]

Fixes: https://tracker.ceph.com/issues/54184

Signed-off-by: Redouane Kachach rkachach@redhat.com

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)
  • 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

[mgfritch]

why is this needed?

[rkachach]

The validate() method checks the spec object and validate its data to detect any error.

[adk3798]

it actually ends up reaching the validate anyway even without the validate call. I commented this line out and got

[ceph: root@vm-00 /]# ceph orch apply mds 5 
Error EINVAL: Traceback (most recent call last):
  File "/usr/share/ceph/mgr/mgr_module.py", line 1701, in _handle_command
    return self.handle_command(inbuf, cmd)
  File "/usr/share/ceph/mgr/orchestrator/_interface.py", line 171, in handle_command
    return dispatch[cmd['prefix']].call(self, cmd, inbuf)
  File "/usr/share/ceph/mgr/mgr_module.py", line 433, in call
    return self.func(mgr, **kwargs)
  File "/usr/share/ceph/mgr/orchestrator/_interface.py", line 107, in <lambda>
    wrapper_copy = lambda *l_args, **l_kwargs: wrapper(*l_args, **l_kwargs)  # noqa: E731
  File "/usr/share/ceph/mgr/orchestrator/_interface.py", line 96, in wrapper
    return func(*args, **kwargs)
  File "/usr/share/ceph/mgr/orchestrator/module.py", line 1080, in _apply_mds
    return self._apply_misc([spec], dry_run, format, no_overwrite)
  File "/usr/share/ceph/mgr/orchestrator/module.py", line 1046, in _apply_misc
    raise_if_exception(completion)
  File "/usr/share/ceph/mgr/orchestrator/_interface.py", line 228, in raise_if_exception
    raise e
Exception: MDS service id cannot start with a numeric digit

which, in the cephadm mgr module actually went through

Traceback (most recent call last):
  File "/usr/share/ceph/mgr/orchestrator/_interface.py", line 125, in wrapper
    return OrchResult(f(*args, **kwargs))
  File "/usr/share/ceph/mgr/cephadm/module.py", line 2504, in apply
    results.append(self._apply(spec))
  File "/usr/share/ceph/mgr/cephadm/module.py", line 2377, in _apply
    return self._apply_service_spec(cast(ServiceSpec, spec))
  File "/usr/share/ceph/mgr/cephadm/module.py", line 2482, in _apply_service_spec
    allow_colo=self.cephadm_services[spec.service_type].allow_colo(),
  File "/usr/share/ceph/mgr/cephadm/schedule.py", line 174, in validate
    self.spec.validate()
  File "/lib/python3.6/site-packages/ceph/deployment/service_spec.py", line 684, in validate
    raise SpecValidationError('MDS service id cannot start with a numeric digit')
ceph.deployment.hostspec.SpecValidationError: MDS service id cannot start with a numeric digit

so I think we don't actually need this call

[rkachach]

we are already doing the same (validating the spec ASAP) in for the SNMPGatewaySpec:

        spec = SNMPGatewaySpec(
            snmp_version=snmp_version,
            port=port,
            credentials=credentials,
            snmp_destination=destination,
            engine_id=engine_id,
            auth_protocol=auth_protocol,
            privacy_protocol=privacy_protocol,
            placement=PlacementSpec.from_string(placement),
            unmanaged=unmanaged,
            preview_only=dry_run
        )

       spec.validate()  # force any validation exceptions to be caught correctly

In my opinion data (in general) have to be validated ASAP. In fact in the above trace we make several calls, in several classes to end up calling the validation (to detect that the spec is invalid). Something that we could have done earlier. In think both for code readability (and for analyzing errors in the future if any) it's better to do the validation ASAP so in this case I'd prefer to keep this explicit call to .validate() method.

[adk3798]

As discussed in standup, we're okay with having this check called directly, but maybe we should actually add this to other apply commands as well to have the functionality be uniform

[rkachach]

I added validate() checks for the other services

[mgfritch]

see also: #44995

[phlogistonjohn]

This is minor but do you mean "early" or "immediately" rather than "correctly" here?

[rkachach]

in this case is immediatly since the exception will be raised right away

[adk3798]

@mgfritch do you think there are any other parameters we could maybe need for mds or is this enough

[rkachach]

jenkins retest this please

[adk3798]

http://pulpito.ceph.com/adking-2022-02-17_13:37:32-orch:cephadm-wip-adk-testing-2022-02-17-0053-distro-basic-smithi/

Failures tracked by https://tracker.ceph.com/issues/54304