mgr/devicehealth: fix _get_device_metrics ValueError#41946
mgr/devicehealth: fix _get_device_metrics ValueError#41946tchaikov merged 1 commit intoceph:masterfrom
Conversation
| isample = None | ||
| imin_sample = None | ||
| try: | ||
| if sample: |
There was a problem hiding this comment.
The issue here seems to be that sample is ''. I would say that's not a valid input and the diskprediction module should be fixed:
ceph/src/pybind/mgr/diskprediction_local/module.py
Lines 129 to 130 in 87ab4d7
Or we could change _t2epoch to accept the empty string and return 0.
|
retest this please |
This appears to have broken with abd35d4 The SQL OR doesn't work because in the case that sample is passed, _t2epoch(min_sample) is 0 and the 0 <= time portion of the expression is always true. Fixes: https://tracker.ceph.com/issues/51294 Signed-off-by: Sage Weil <sage@newdream.net>
|
|
||
| def _t2epoch(self, t: Optional[str]) -> int: | ||
| if t is None: | ||
| if not t: |
There was a problem hiding this comment.
With this change I don't think the SQL changes are necesary, right?
There was a problem hiding this comment.
I think they are, or else you end up with (time = ? OR ? <= time) -> (time = 123 OR 0 <= time) which is everything instead of just 123.
There was a problem hiding this comment.
Hm, I don't know how this worked in my testing when I made this change. Anyway, that makes sense to me.
|
|
||
| def _t2epoch(self, t: Optional[str]) -> int: | ||
| if t is None: | ||
| if not t: |
There was a problem hiding this comment.
Hm, I don't know how this worked in my testing when I made this change. Anyway, that makes sense to me.
This appears to have broken with abd35d4
Fixes: https://tracker.ceph.com/issues/51294
Signed-off-by: Sage Weil sage@newdream.net