mgr/cephadm: add config section to ServiceSpec#39648
mgr/cephadm: add config section to ServiceSpec#39648sebastian-philipp merged 3 commits intoceph:masterfrom
Conversation
| self.log.warning( | ||
| f'Ignoring invalid {spec.service_name()} config option {k}' | ||
| ) | ||
| continue |
There was a problem hiding this comment.
raise and make it a HEALTH warning? I don't like silent fails, as they make it super hard to debug stuff. Like "why is this config option not working at all?"
Also, would be great to validate them in the cli handler synchronously
There was a problem hiding this comment.
very tedious to raise a health warning from here, but i added validation at apply time, so this would only happen if a spec had a config option that was removed from ceph (and i think that's pretty harmless)
There was a problem hiding this comment.
might be harmless for the cluster, but not for users. Can you add something like
self.mgr.events.for_service(spec, 'ERROR', msg)this is then displayed in https://docs.ceph.com/en/latest/cephadm/troubleshooting/#per-service-and-per-daemon-events and https://tracker.ceph.com/issues/49262
91111c5 to
f331d0c
Compare
f331d0c to
855d452
Compare
|
relates to https://tracker.ceph.com/issues/49490 |
sebastian-philipp
left a comment
There was a problem hiding this comment.
lgtm! (except for the missing events)
| self.log.warning( | ||
| f'Failed to set {spec.service_name()} option {k}: {e}' | ||
| ) |
There was a problem hiding this comment.
also
self.mgr.events.for_service(spec, 'ERROR', msg)|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
Signed-off-by: Sage Weil <sage@newdream.net>
If a service is managing a config option, prevent the user from specifying it in the spec. Signed-off-by: Sage Weil <sage@newdream.net>
Make sure config options are valid/exist. Signed-off-by: Sage Weil <sage@newdream.net>
855d452 to
deb9eae
Compare
|
one thing is missing: we're not cleaning those configs when removing the service. for that you need to hook into ceph/src/pybind/mgr/cephadm/services/cephadmservice.py Lines 291 to 293 in 80fb753 |
|
I have a slight impression that this feature is a bit too generic. I wonder if we'll get tracker issues, cause this is a bit too generic. I don't know if we should advertise this as a cool new feature just yet. To some degree this feels like #37106 which we already know is not properly supported when upgrading. |
Pass config key/value pairs through and set them in teh appropriate section.
Disallow options that are managed by cephadm itself.
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox