python-common: fix ServiceSpec validation#35493
python-common: fix ServiceSpec validation#35493sebastian-philipp merged 3 commits intoceph:masterfrom
Conversation
the ServiceSpec needs to be validated during `orch apply`, but not during `orch daemon add` Signed-off-by: Michael Fritch <mfritch@suse.com>
Signed-off-by: Michael Fritch <mfritch@suse.com>
the service_id needs to be validated during `orch apply`, but not during `orch daemon add` Signed-off-by: Michael Fritch <mfritch@suse.com>
| if self.service_type in ['mds', 'rgw', 'nfs', 'iscsi'] and not self.service_id: | ||
| raise ServiceSpecValidationError('Cannot add Service: id required') | ||
|
|
There was a problem hiding this comment.
Is there a chance that there are existing specs in the store that violate this validation?
There was a problem hiding this comment.
nice, this is what I meant to do as well.
Is there a chance that there are existing specs in the store that violate this validation?
They probably is. Is the validate method being run on already existing specs though? If not, this would only raise for new or exported(and re-imported) specs.
There was a problem hiding this comment.
nice, this is what I meant to do as well.
Is there a chance that there are existing specs in the store that violate this validation?
They probably is. Is the
validatemethod being run on already existing specs though?
right: __init__ doesn't call validate():
ceph/src/python-common/ceph/deployment/service_spec.py
Lines 399 to 411 in 3a757a4
and https://github.com/ceph/ceph/blob/master/src/pybind/mgr/cephadm/inventory.py doesn't call .validate() either.
should we add it? If yes, how do we handle specs that fail the validation?
If not, this would only raise for new or exported(and re-imported) specs.
There was a problem hiding this comment.
should we add it? If yes, how do we handle specs that fail the validation?
we probably only should do that if we have a clear path for communicating these kind of issues. some way of indicating that an already applied spec is not valid anymore..
otoh, we probably shouldn't invalidate existing specs to begin with.. maybe adding an internal tracking id for already existing specs isn't the worst move.
There was a problem hiding this comment.
it actually is called on existing specs here:
https://github.com/ceph/ceph/blob/master/src/pybind/mgr/cephadm/schedule.py#L67
There was a problem hiding this comment.
which is not as bad as in load() . I think #35456 plus some HEALTH_WARN integration would be enough here.
There was a problem hiding this comment.
Is there a chance that there are existing specs in the store that violate this validation?
If so, they would have likely broke the orch ls etc commands...
which is not as bad as in
load(). I think #35456 plus some HEALTH_WARN integration would be enough here.
This actually makes a lot of sense in the general case where the cluster state could change and we might want to (re)validate before another placement...
There was a problem hiding this comment.
so I think we're good, as long as we provide a way for users to remove that service after they did an upgrade.
|
QA run failed due to #35474 (comment) . not 100% sure if this PR is not related. |
|
@callithea is this |
|
jenkins test dashboard backend |
Fixes various issues around validation of a ServiceSpec during
orch apply.Signed-off-by: Michael Fritch mfritch@suse.com
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 dashboard backendjenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox