| opendevreview | chandan kumar proposed openstack/watcher master: Drop code from Host maintenance strategy migrating instance to disabled hosts https://review.opendev.org/c/openstack/watcher/+/948967 | 03:55 |
|---|---|---|
| opendevreview | chandan kumar proposed openstack/watcher stable/2025.1: Drop nova command reference from the code https://review.opendev.org/c/openstack/watcher/+/949693 | 03:59 |
| opendevreview | chandan kumar proposed openstack/watcher stable/2024.2: Drop nova command reference from the code https://review.opendev.org/c/openstack/watcher/+/949694 | 04:01 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Return HTTP code 400 when creating an audit with wrong parameters https://review.opendev.org/c/openstack/watcher/+/949703 | 07:48 |
| opendevreview | David proposed openstack/watcher-tempest-plugin master: Refactor of wait_for_instances_in_model function to wait until all instances are in the correct compute node on the model https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/949657 | 08:41 |
| opendevreview | David proposed openstack/watcher-tempest-plugin master: Refactor of wait_for_instances_in_model function to wait until all instances are in the correct compute node on the model https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/949657 | 09:18 |
| jgilaber | sean-k-mooney: some more context on my question from yesterday. It was about the bug https://bugs.launchpad.net/watcher/+bug/2110149 | 10:09 |
| jgilaber | this debug call https://github.com/openstack/watcher/blob/77e7e4ef7b5b0cd29e27b3529fdac4fdc2287d40/watcher/common/nova_helper.py#L732 is wrong and there are a few similar ones in cinder-helper | 10:09 |
| jgilaber | but because the unit test have the logging disabled, this was not noticed | 10:10 |
| jgilaber | in https://review.opendev.org/c/openstack/watcher/+/949187 I managed to make the error visible, but it feels really hacky, and I guessed there must be a better way | 10:10 |
| sean-k-mooney | jgilaber: so i have not looked at watches based test yet but oslo.test has some standard logging fixture whice we normally use | 10:16 |
| sean-k-mooney | so logging should not be disable dby default in the test but the logs should be captured by the test fixture | 10:17 |
| sean-k-mooney | the question is is watcher using that and using it properly | 10:17 |
| jgilaber | this is what you mean? https://docs.openstack.org/oslotest/latest/reference/api/oslotest.log.html#oslotest.log.ConfigureLogging | 10:21 |
| jgilaber | the only reference to oslotest that I see in watcher is the use BaseTestCase https://github.com/openstack/watcher/blob/59607f616a0a7c8e38f488922ec3c27dffe692e7/watcher/tests/base.py#L43 | 10:23 |
| sean-k-mooney | yes so nova has its own standard logign fixture that predates that to make sure that logging is properly cofnigured in the test case https://github.com/openstack/nova/blob/master/nova/tests/fixtures/nova.py#L126-L199 | 10:24 |
| sean-k-mooney | so nova disables the base capture log fixture https://github.com/openstack/nova/blob/master/nova/test.py#L187-L188 | 10:25 |
| sean-k-mooney | but normlaly we would use that to ensure that logging is configure properly in the tests | 10:25 |
| jgilaber | ok, that looks promising, I'll give it a shot, thanks! | 10:26 |
| sean-k-mooney | you proably could copy paste the nova StandardLogging fixture to get a workable baseline | 10:29 |
| jgilaber | yep, I'll use that as a start | 10:30 |
| opendevreview | David proposed openstack/watcher-tempest-plugin master: Add tests for workload_balance with injected data https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/949722 | 10:36 |
| sean-k-mooney | jgilaber: by the way we also often just do this https://github.com/openstack/nova/blob/eb5a4a7f3b0b8c3967c42a537cca766144eff25b/nova/tests/unit/virt/libvirt/volume/test_mount.py#L571-L581 | 11:03 |
| sean-k-mooney | in openstack we have the pattern of creatign a module level logger. | 11:04 |
| sean-k-mooney | that means you can simply mock that in a test and then do asserts if you want to assert specific log output | 11:04 |
| sean-k-mooney | we use the fixture for generic log assertions and capture | 11:05 |
| sean-k-mooney | both patthern have there place but just wanted you to be aware fo that pattern as well | 11:05 |
| jgilaber | thanks, that might be useful in the future | 11:06 |
| sean-k-mooney | its useful if you creating a repoducer test and you want to assert forexample that an specific excption message is logged | 11:07 |
| sean-k-mooney | i.e. if your replciating a failure form a bug report | 11:07 |
| sean-k-mooney | jgilaber: https://github.com/openstack/nova/blob/eb5a4a7f3b0b8c3967c42a537cca766144eff25b/nova/tests/unit/virt/libvirt/cpu/test_core.py#L56-L60 | 11:12 |
| sean-k-mooney | when your usign the logging fixture you access the loggers output by self.stdlog.logger.output | 11:12 |
| jgilaber | yep, that's something I wanted to do | 11:13 |
| sean-k-mooney | im just calling htat out because i dotn know if self.assertlogs will work with that fixture | 11:13 |
| jgilaber | looks much cleaner, I'll try to get it working this afternoon | 11:13 |
| sean-k-mooney | it may be our loging fixtures predate that | 11:13 |
| sean-k-mooney | i belive how this works internally is we are configuring pythons loggin infra to log to an StringIO stream | 11:14 |
| sean-k-mooney | so assertLogs has the potential to work but i dont use that normlaly | 11:15 |
| opendevreview | sean mooney proposed openstack/watcher-dashboard master: add pyproject.toml to support pip 23.1 https://review.opendev.org/c/openstack/watcher-dashboard/+/949737 | 12:48 |
| opendevreview | sean mooney proposed openstack/watcher-tempest-plugin master: add pyproject.toml to support pip 23.1 https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/949738 | 12:51 |
| opendevreview | sean mooney proposed openstack/python-watcherclient master: add pyproject.toml to support pip 23.1 https://review.opendev.org/c/openstack/python-watcherclient/+/949739 | 12:51 |
| opendevreview | David proposed openstack/watcher-tempest-plugin master: Add tests for workload_balance with injected data https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/949722 | 13:05 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Return HTTP code 400 when creating an audit with wrong parameters https://review.opendev.org/c/openstack/watcher/+/949703 | 15:05 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Add a unit test to check the error when creating an audit with wrong parameters https://review.opendev.org/c/openstack/watcher/+/949769 | 15:05 |
| opendevreview | David proposed openstack/watcher-tempest-plugin master: Add tests for workload_balance with injected data https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/949722 | 15:10 |
| opendevreview | David proposed openstack/watcher-tempest-plugin master: Add metrics for RAM to gnocchi and prometheus injected data https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/949773 | 15:22 |
| opendevreview | David proposed openstack/watcher-tempest-plugin master: Add metrics for RAM to gnocchi and prometheus injected data Required by futures tests of strategies that use memory as parameter for the audits https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/949773 | 15:23 |
| opendevreview | David proposed openstack/watcher-tempest-plugin master: Add tests for workload_balance with injected data https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/949722 | 15:43 |
| jgilaber | sean-k-mooney: the logging stuff turned out to be a deeper rabbit hole that I expected | 16:39 |
| jgilaber | I added the nova fixture but that did not solve the problem, the logging was still disabled | 16:39 |
| sean-k-mooney | :) | 16:39 |
| jgilaber | I finally managed to track what is disabling | 16:39 |
| sean-k-mooney | oh cool | 16:39 |
| jgilaber | it's this https://github.com/openstack/watcher/blob/59607f616a0a7c8e38f488922ec3c27dffe692e7/watcher/db/sqlalchemy/alembic/env.py#L26 | 16:40 |
| sean-k-mooney | what | 16:40 |
| jgilaber | called from https://github.com/openstack/watcher/blob/master/watcher/tests/db/base.py#L56 | 16:40 |
| sean-k-mooney | how | 16:40 |
| jgilaber | the setup for the db tests calls alembic stamp | 16:40 |
| jgilaber | which loads the env.py | 16:40 |
| sean-k-mooney | rieght btu that is defintly not intentional | 16:40 |
| jgilaber | yes, that is a side effect | 16:40 |
| sean-k-mooney | ok that tracks we whoudl liketly isolate the effect on the config | 16:41 |
| jgilaber | apparently allembic generates a ini file on the fly whichs configures logging as well | 16:41 |
| sean-k-mooney | yep it does | 16:42 |
| jgilaber | and that for some reason disables the watcher logging | 16:42 |
| sean-k-mooney | so this is runnign as a sideefect fo the import | 16:42 |
| sean-k-mooney | which is not whoe we shoudl be doign that | 16:42 |
| jgilaber | I just tried uncommenting the env.py and we get 7 unit test failures during logging (3 of those are new yay!) | 16:42 |
| sean-k-mooney | im just looking at how we handel this in other service like placement/nova | 16:43 |
| sean-k-mooney | we can likely takek the same approch in watcher ot the alembic migrations | 16:44 |
| sean-k-mooney | so they use a db fixture also but the logic is a bit different | 16:46 |
| jgilaber | nova avoids configuring the logging in alembic https://github.com/openstack/nova/blob/eb5a4a7f3b0b8c3967c42a537cca766144eff25b/nova/db/migration.py#L50 | 16:46 |
| jgilaber | with a condition in the env.py https://github.com/openstack/nova/blob/eb5a4a7f3b0b8c3967c42a537cca766144eff25b/nova/db/api/migrations/env.py#L27C27-L27C43 | 16:46 |
| sean-k-mooney | ya that an option | 16:46 |
| sean-k-mooney | placement also does not seam to configure the logging | 16:48 |
| sean-k-mooney | we can take the same apprcoh either by following placements lead https://github.com/openstack/placement/blob/master/placement/db/sqlalchemy/alembic/env.py and just not invokeing that on import | 16:52 |
| sean-k-mooney | or by explcitly disabling it like nova does | 16:52 |
| sean-k-mooney | jgilaber: im leaning toworads just deleteing https://github.com/openstack/watcher/blob/master/watcher/db/sqlalchemy/alembic/env.py#L24-L26 honestly | 16:53 |
| sean-k-mooney | jgilaber: this unfortuently dates form the inital import of the git repo so we do not have any context in git blame | 16:55 |
| jgilaber | looking at the comment in nova code https://github.com/openstack/nova/blob/eb5a4a7f3b0b8c3967c42a537cca766144eff25b/nova/db/migration.py#L49C7-L49C50 | 16:56 |
| jgilaber | it seems that this logging is only important when running alembic from cli | 16:56 |
| jgilaber | not sure if this would affect also running watcher-db-manage? | 16:57 |
| jgilaber | there is also a simple solution of adding 'disable_existing_loggers=False' to the fileConfig call in env.py | 16:57 |
| sean-k-mooney | it would but all loging is ment to be confired via oslo in that case | 16:57 |
| jgilaber | that leaves the existing logging alone | 16:57 |
| sean-k-mooney | i think this is legacy code form the inital import that should have been updated hwen oslo.log was intoduced | 16:58 |
| jgilaber | ack, in that case I'll remove it | 16:58 |
| jgilaber | thanks! | 16:58 |
| opendevreview | JaromÃr Wysoglad proposed openstack/watcher-specs master: Support multitenancy with Prometheus datasource https://review.opendev.org/c/openstack/watcher-specs/+/949804 | 20:56 |
Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!