| opendevreview | JamesLin proposed openstack/watcher-specs master: fix: Update the launchpad blueprint link of host-maintenance-strategy-disable-migration spec https://review.opendev.org/c/openstack/watcher-specs/+/952729 | 01:13 |
|---|---|---|
| opendevreview | Victoria Martinez de la Cruz proposed openstack/grian-ui master: [WIP] Volume metrics admin dashboard https://review.opendev.org/c/openstack/grian-ui/+/952750 | 10:32 |
| sean-k-mooney | gmaan: amoralej fun faction all existign action pre and post condtions are litrualy nops | 11:23 |
| sean-k-mooney | we should change that but that makes the effect on the skipped stte simpelr. | 11:24 |
| sean-k-mooney | we shoudl not remove the content form the spec | 11:24 |
| sean-k-mooney | as we want to defien the behavior moving forward | 11:24 |
| sean-k-mooney | but ya thats a thing. | 11:24 |
| amoralej | actually, i thought of that part of the skipped while checking existing pre_conditions in actions | 11:25 |
| amoralej | which are empty, but i think it's good to implement them once we have skipped in place | 11:25 |
| amoralej | as you said, to move forward | 11:26 |
| sean-k-mooney | yep, at a minitum we shoudl be assertign the resouce we are goign to perfrom an action on exist and is in a vaild sate | 11:26 |
| amoralej | yes | 11:26 |
| sean-k-mooney | i was reviewign the new stop action for jameslin's spec https://review.opendev.org/c/openstack/watcher/+/952538/4/watcher/applier/actions/stop.py | 11:27 |
| sean-k-mooney | and they actully impelemted pre/post condtion checks | 11:27 |
| sean-k-mooney | althoguh we will need to modify https://github.com/openstack/watcher/blob/master/watcher/applier/workflow_engine/default.py#L163 to actully mark the action as failed if the post condition failed | 11:29 |
| amoralej | that's good | 11:29 |
| amoralej | although we may move to skipped once we have it. i.e. instance does not exist or it is already stopped | 11:30 |
| sean-k-mooney | skipped woudl be correct for preconditions failure not for post condtion failures | 11:30 |
| sean-k-mooney | if the post_condtions fail then the action failed | 11:31 |
| amoralej | yes, i meant pre, i was reading that | 11:31 |
| sean-k-mooney | right | 11:31 |
| amoralej | agree, post failures -> failed | 11:31 |
| sean-k-mooney | so when we have skipped then we need to make sure that that works proeprly | 11:31 |
| sean-k-mooney | you shoudl be able to handel that generically here https://github.com/openstack/watcher/blob/master/watcher/applier/workflow_engine/default.py#L139-L144 | 11:33 |
| sean-k-mooney | just wrappign the cal to pre_conditosn in a try except | 11:33 |
| sean-k-mooney | and marking the action as skiped if it raises | 11:33 |
| amoralej | yes, that was my plan | 11:35 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Add debug message to report calculated metric for workload_balance https://review.opendev.org/c/openstack/watcher/+/951639 | 12:12 |
| opendevreview | David proposed openstack/watcher-tempest-plugin master: Add tests for workload balance with real data https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/950335 | 13:24 |
| opendevreview | Joan Gilabert proposed openstack/watcher master: Support zone migration audit without compute_nodes https://review.opendev.org/c/openstack/watcher/+/950665 | 13:31 |
| opendevreview | Merged openstack/watcher-specs master: fix: Update the launchpad blueprint link of host-maintenance-strategy-disable-migration spec https://review.opendev.org/c/openstack/watcher-specs/+/952729 | 14:44 |
| amoralej | sean-k-mooney, wrt scope, after checking with jgilaber the current behavior of scope in the audits is that the model which is presented to every audit execution in the decision-engine is scoped according to the scope definition, https://github.com/openstack/watcher/blob/31879d26f4889c6367d85ff279f369913c672df7/watcher/decision_engine/strategy/strategies/base.py#L289-L291 | 15:04 |
| amoralej | so scope should be the "top level filter" for the audit | 15:05 |
| amoralej | even in zone_migration it has more priority that the list of src_node in compute_nodes parameter | 15:05 |
| amoralej | jgilaber ^ is this correct? | 15:05 |
| amoralej | well, there is a bug, but that's a different story :) | 15:05 |
| jgilaber | yes, the model that is exposed to the strategy takes into account the scope properly | 15:14 |
| jgilaber | but the input for the audit is not modified, so as amoralej says in the zone migration if you ask for a migration that is out of the scope the audit will crash | 15:15 |
| sean-k-mooney | amoralej: ok but the zone migration only move vms on the comptue nodes that are in the input parmaters | 15:21 |
| sean-k-mooney | amoralej: so if you only use scope it wont work | 15:21 |
| amoralej | yes | 15:21 |
| sean-k-mooney | well it might but it wont move anythign | 15:21 |
| amoralej | it won't move anything if compute_nodes is empty, regardless of the scope | 15:22 |
| sean-k-mooney | we would need something like a "compute_nodes_form_scope" value in the input_parmaters | 15:22 |
| amoralej | ok, now i understand your point, i didn't get it before, so you want to infer input_parameter.compute_nodes from the scope | 15:23 |
| amoralej | that's an rfe :) | 15:23 |
| sean-k-mooney | right if we wanted to defien a zone via a host aggreate and pass that host aggrate via the scope | 15:23 |
| sean-k-mooney | we would need to infer the computes form taht scope | 15:24 |
| sean-k-mooney | for it ot be useful | 15:24 |
| sean-k-mooney | yes it defintly is | 15:24 |
| sean-k-mooney | for some stragies that just operate on teh whole model | 15:25 |
| sean-k-mooney | you dont need the infernce | 15:25 |
| sean-k-mooney | but for zone migration you do | 15:25 |
| amoralej | yep | 15:25 |
| sean-k-mooney | it looks like host_matintance adn zone_migration are outlier | 15:28 |
| sean-k-mooney | the other stargies do not use input_parmaters to contol whcih resouce to modify | 15:29 |
| sean-k-mooney | for zone migration the solution might be to deprecate compute_nodes and make ti optional in a newer microverion | 15:29 |
| sean-k-mooney | and instead contole it via scope | 15:30 |
| amoralej | but then we'd need a way to specify dst_node if we want to maintain current behavior | 15:30 |
| amoralej | compute_nodes can be made of src_node, dst_node pairs | 15:31 |
| sean-k-mooney | well i guess you would not need to fuly deprecate it jsut he soruce nod aspect | 15:31 |
| sean-k-mooney | i guess curently its also quite limited in that you cant have multiple src_nodes | 15:31 |
| amoralej | it's 1:1, yes | 15:32 |
| sean-k-mooney | which is really somethign we woudl want to have for a "zone based" stratgey | 15:32 |
| sean-k-mooney | realisticly i think in the medium term we may end up decidng to create new stragies to replace this | 15:32 |
| sean-k-mooney | you can do the vm part with workload stablisation | 15:33 |
| sean-k-mooney | and the volume part to some degree with https://docs.openstack.org/watcher/latest/strategies/storage_capacity_balance.html | 15:33 |
| sean-k-mooney | but we may decised that its better to create a clean impletation without the legacy | 15:34 |
| sean-k-mooney | that will replcae the existing one | 15:34 |
| amoralej | yes, mid-term i think it's worthy to rethink what we want to do with this goal and strategy | 15:35 |
| sean-k-mooney | the only thing you cant do with the other two today | 15:35 |
| sean-k-mooney | is move the vms and the volumes in one action plan | 15:35 |
| amoralej | and moving vms from a predefined set of hosts | 15:36 |
| amoralej | host_maintenance can do only one | 15:36 |
| amoralej | and workload_stabilization would not move all the vms | 15:37 |
| amoralej | i mean, at least we enhance it for that use case | 15:37 |
| amoralej | sean-k-mooney, wrt unit tests to assert debug messages, should i mock LoggerAdapter.debug ? | 15:37 |
| amoralej | what's the best way to do it? | 15:37 |
| sean-k-mooney | did the job report. if the line has coverage now then the fixture that jgilaber proted form nova tests teh formating | 15:38 |
| sean-k-mooney | how we woudl normally test this is mock the LOG varible in teh module under test and assert it called with teh correct paramter or assert teh expected string is in teh output of the log fixture | 15:39 |
| sean-k-mooney | either works | 15:39 |
| jgilaber | the problem with the fixture is that with the current config it will not capture debug logs in the output | 15:40 |
| amoralej | we are not including debug messages in the log fixtures, iiuc | 15:40 |
| jgilaber | it executes them, but with a NullHandler | 15:40 |
| sean-k-mooney | jgilaber: debug logs requrie you to set an envionemt variable | 15:40 |
| amoralej | so mocking LOG seems the way to go | 15:40 |
| sean-k-mooney | OS_DEBUG | 15:40 |
| sean-k-mooney | and we shoudl be setting that in the ci jobs | 15:40 |
| jgilaber | yes, ok then that is missing | 15:40 |
| sean-k-mooney | jgilaber: actully i think we may not to save space i cant recall | 15:41 |
| sean-k-mooney | amoralej: but ya mockign log.debug is more convetnional | 15:41 |
| jgilaber | I guessed that it was disabled by default because it would be quite verbose | 15:41 |
| sean-k-mooney | i know we had issues in teh past with hitting the max message size of subunit | 15:41 |
| sean-k-mooney | when we had debug enabeld in nova | 15:42 |
| sean-k-mooney | that was eventually fixed in stestr | 15:42 |
| sean-k-mooney | but it was a proablme in the past | 15:42 |
| sean-k-mooney | amoralej: so if we look at the coverage report | 15:43 |
| sean-k-mooney | https://cba07d0052e893dee9e9-23ff2657fe87c0d4ee3109cc6b8bff44.ssl.cf2.rackcdn.com/openstack/d872cc5544854bd28ecf23f9c9e342de/cover/z_65cb95e032a919a1_workload_balance_py.html#t201 | 15:43 |
| sean-k-mooney | we have unit test coverage for the debug slog you added at least there | 15:43 |
| sean-k-mooney | we have no unit test coverage for instance_ram_usage... | 15:44 |
| sean-k-mooney | amoralej: that lack of coverage was actully a problem before your change | 15:44 |
| sean-k-mooney | so you do not need to fix it in this one althoguh i wont object if you do | 15:44 |
| sean-k-mooney | but we should add that to the list of tech debt | 15:45 |
| sean-k-mooney | i dont think we need 100% unit test coverage | 15:45 |
| sean-k-mooney | but we shoudl try to impvoe our current gaps over time | 15:45 |
| amoralej | got it | 15:46 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Add debug message to report calculated metric for workload_balance https://review.opendev.org/c/openstack/watcher/+/951639 | 17:15 |
| -opendevstatus- NOTICE: Zuul jobs reporting POST_FAILURE were due to an incident with one of our cloud providers; this provider has been temporarily disabled and changes can be rechecked | 22:38 | |
Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!