| amoralej | dviroel, lgtm | 09:22 |
|---|---|---|
| amoralej | sorry, i missed your message yesterday | 09:22 |
| opendevreview | Joan Gilabert proposed openstack/watcher master: Prepare to use openstacksdk instead of novaclient https://review.opendev.org/c/openstack/watcher/+/974710 | 09:40 |
| opendevreview | Joan Gilabert proposed openstack/watcher master: Complete migration from novaclient to openstacksdk https://review.opendev.org/c/openstack/watcher/+/974924 | 09:40 |
| opendevreview | Joan Gilabert proposed openstack/watcher master: Remove usage of novaclient from Watcher https://review.opendev.org/c/openstack/watcher/+/974925 | 09:40 |
| opendevreview | Joan Gilabert proposed openstack/watcher master: Remove nova_helper retries for openstacksdk params https://review.opendev.org/c/openstack/watcher/+/975498 | 09:40 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Skip change_nova_service_state actions in pre_condition phase https://review.opendev.org/c/openstack/watcher/+/977340 | 09:44 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Add get_flavor_id method to nova_helper https://review.opendev.org/c/openstack/watcher/+/977772 | 09:53 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Skip resize actions in pre_condition phase https://review.opendev.org/c/openstack/watcher/+/977773 | 09:53 |
| amoralej | I've been hitting https://bugs.launchpad.net/watcher/+bug/2142486 while testing the openstacksdk patches. I see it could be fixed in different levels, sqlalchemy models, objects, or applying some status_message_normalize method before updating it anywhere. What would be the best approach? any opinion? | 09:58 |
| sean-k-mooney | amoralej: the status message shoudl not have the fully execption traceback or a long message in general | 10:33 |
| sean-k-mooney | amoralej: if we are storign that today that a bug | 10:33 |
| sean-k-mooney | it should have only the user facing message and perhaps the name of the expction | 10:34 |
| amoralej | https://github.com/openstack/watcher/blob/18cb461373c2a7320f2c9506681fb2b01888e3c6/watcher/applier/workflow_engine/base.py#L184-L187 | 10:35 |
| amoralej | that's where it comes from | 10:35 |
| amoralej | wuoldn't that be the user facing message ? | 10:36 |
| sean-k-mooney | we are catching Excption there | 10:37 |
| sean-k-mooney | we shoudl really only be catching one of our watcher expctions | 10:37 |
| sean-k-mooney | the bug looked like it was some networing excption | 10:37 |
| amoralej | actually, yes, that was the case | 10:37 |
| sean-k-mooney | HTTPSConnectionPool(host='nova ... (152 characters truncated) ... NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7fb167c4b500>: Failed to establish a new connection: [Errno 111] ECONNREFUSED')) | 10:37 |
| amoralej | it was raising a connectivity error | 10:38 |
| sean-k-mooney | ya | 10:38 |
| sean-k-mooney | so we need to do 2 things | 10:38 |
| amoralej | i disabled nova-api for that test | 10:38 |
| sean-k-mooney | for generic exctption we need to convert them to a new excption with a fixed lenght | 10:38 |
| sean-k-mooney | for anything that inherits form our watcher base exection class it shoudl be fine | 10:38 |
| sean-k-mooney | to catch and store them | 10:38 |
| sean-k-mooney | basicly i woudl add another except clasue before this for WatcherException | 10:40 |
| sean-k-mooney | https://github.com/openstack/watcher/blob/master/watcher/common/exception.py#L56 | 10:40 |
| sean-k-mooney | and then update the current code to do `% type(e)` instead of `% str(e)` | 10:41 |
| amoralej | ack, lemme check that | 10:41 |
| amoralej | no need to cut to 254 in other places as a safety measure? | 10:42 |
| amoralej | impact is uggly as it fails to update state to FAILED, we maybe should also split the save to two different ones, one for state one for status_message ? | 10:43 |
| sean-k-mooney | well the fact that notify is modifyign db state is actully the ugly part | 10:44 |
| sean-k-mooney | it shoud jsut be a action.save() | 10:44 |
| sean-k-mooney | but to your point about truncation | 10:44 |
| sean-k-mooney | we might want ot have a helper function that will take the excption and do the extract or truncate to tyep for us | 10:45 |
| amoralej | anyway, if we would do action.save() we'd have similar problem if we do it in just one save i think | 10:45 |
| sean-k-mooney | and just use it everywhre | 10:45 |
| sean-k-mooney | amoralej: it would yes just notify in other project woudl not muteat the state in teh db | 10:46 |
| sean-k-mooney | it just bad naming | 10:46 |
| amoralej | yep, i agree that it is missleading | 10:46 |
| sean-k-mooney | https://github.com/openstack/watcher/blob/18cb461373c2a7320f2c9506681fb2b01888e3c6/watcher/applier/workflow_engine/base.py#L82-L89 | 10:46 |
| sean-k-mooney | that what it actully does it doe a get follwoed by a update and save | 10:46 |
| amoralej | wstatus_message not always comes directly from exceptions, the helper would need to be more generic, that's why i was thinking in just string truncating | 10:48 |
| sean-k-mooney | to me string tuncation is itself a bug | 10:48 |
| sean-k-mooney | we should never need to do that so if it ever truncated we woudl need to log an error and fix it when the bug is reported | 10:48 |
| sean-k-mooney | reportign a shorter message like the execption name is however a good compromies | 10:49 |
| amoralej | ok, lemme apply your proposal of splitting WatcherException + type(e) for Exception, i think that will fix the issue | 10:49 |
| sean-k-mooney | for oure excptions we can ensure our message fit withing the db field lenght | 10:49 |
| amoralej | although we may be bundling lower level exception messages in some of our internal exceptions, lemme take a look | 10:50 |
| sean-k-mooney | we do now start usign raise from e | 10:51 |
| sean-k-mooney | but that should not extend the message | 10:51 |
| sean-k-mooney | just the trace back | 10:51 |
| sean-k-mooney | i.e. we are now using excpetion chianing | 10:51 |
| sean-k-mooney | https://docs.python.org/3/tutorial/errors.html#exception-chaining | 10:52 |
| amoralej | https://github.com/openstack/watcher/blob/18cb461373c2a7320f2c9506681fb2b01888e3c6/watcher/applier/workflow_engine/default.py#L130-L137 | 10:57 |
| amoralej | raise exception.WorkflowExecutionException(error=e) | 10:57 |
| amoralej | raise exception.WorkflowExecutionException(error=e) | 10:57 |
| amoralej | wuld that impact the message? | 10:57 |
| sean-k-mooney | ya so that shoudl likely be refactored | 10:58 |
| sean-k-mooney | we shoudl do raise exception.WorkflowExecutionException(error="short error") form e | 10:59 |
| sean-k-mooney | perhaps raise exception.WorkflowExecutionException(error=type(e)) form e | 10:59 |
| sean-k-mooney | that will print the name in the string but provide the full details in teh traceback in the logs | 11:00 |
| sean-k-mooney | at least i belive htat is how that works | 11:00 |
| amoralej | i'll check it | 11:00 |
| sean-k-mooney | by the way this https://github.com/openstack/watcher/blob/18cb461373c2a7320f2c9506681fb2b01888e3c6/watcher/applier/workflow_engine/default.py#L132 is a bug | 11:01 |
| sean-k-mooney | we shoudl not raise a exepciotn class we shoudl raise a instnace of the expction | 11:01 |
| sean-k-mooney | i.e. there shoudl be () | 11:01 |
| amoralej | yep, that's a bug | 11:02 |
| opendevreview | Joan Gilabert proposed openstack/watcher master: Remove nova_helper retries for openstacksdk params https://review.opendev.org/c/openstack/watcher/+/975498 | 11:24 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Check Audit state in the applier before starting the execution https://review.opendev.org/c/openstack/watcher/+/977786 | 11:38 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Check Audit state in the decision-engine before starting the execution https://review.opendev.org/c/openstack/watcher/+/977786 | 11:55 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Add get_flavor_id method to nova_helper https://review.opendev.org/c/openstack/watcher/+/977772 | 14:20 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Skip resize actions in pre_condition phase https://review.opendev.org/c/openstack/watcher/+/977773 | 14:20 |
| opendevreview | Merged openstack/watcher master: Prepare to use openstacksdk instead of novaclient https://review.opendev.org/c/openstack/watcher/+/974710 | 14:48 |
| sean-k-mooney | amoralej: did you have time to test chandans watcher dashboard fix for the skip action | 16:09 |
| sean-k-mooney | is that ready for review? | 16:09 |
| amoralej | sean-k-mooney, let me test the latest PS | 16:27 |
| *** gmaan is now known as gmaan_afk | 16:29 | |
| amoralej | I'd say so, based on the content of latest changes | 16:31 |
| sean-k-mooney | ack ill try and get to it today so | 16:35 |
| sean-k-mooney | it would be nice to wap that up before FF | 16:35 |
| *** gmaan_afk is now known as gmaan | 17:37 | |
| dviroel | sean-k-mooney: hey, if you are around, PTAL on minor changes in https://review.opendev.org/c/openstack/watcher/+/974925 since your last +2 | 20:40 |
| dviroel | parent commit is running gate now | 20:40 |
| sean-k-mooney | sure | 20:41 |
| sean-k-mooney | dviroel: is this really a bug https://review.opendev.org/c/openstack/watcher/+/974925/comment/d848a5b5_1a9a53c9/ | 20:49 |
| sean-k-mooney | it looks valid to me | 20:50 |
| dviroel | yeah, correct, doesn't make sense | 20:55 |
| sean-k-mooney | do we want to fix that in a followup or proceed with the change | 20:56 |
| sean-k-mooney | *and proceed | 20:56 |
| sean-k-mooney | or do we want to leave joan fix it tomorrow | 20:57 |
| dviroel | yeah this is delaying other patches in the series | 20:57 |
| dviroel | may he can fix in one the the other patches | 20:57 |
| dviroel | I still can follow up this one if we W+ | 20:58 |
| sean-k-mooney | ack ill leave a comment to that effect and approve it | 20:58 |
| dviroel | ack | 20:58 |
| sean-k-mooney | ok +2w | 20:59 |
| dviroel | tks sean | 20:59 |
| sean-k-mooney | the operator job is unhappy | 21:00 |
| sean-k-mooney | any idea why? | 21:00 |
| sean-k-mooney | looks like crc failed | 21:01 |
| sean-k-mooney | waiting for ectd | 21:01 |
| sean-k-mooney | or the api server ot be read so this is unrealted | 21:01 |
| opendevreview | Merged openstack/watcher master: Complete migration from novaclient to openstacksdk https://review.opendev.org/c/openstack/watcher/+/974924 | 21:44 |
| opendevreview | Merged openstack/watcher master: Remove usage of novaclient from Watcher https://review.opendev.org/c/openstack/watcher/+/974925 | 22:33 |
Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!