)]}'
{"watcher/applier/actions/volume_migration.py":[{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"fb0665922af732f12507e04f3d5075025e850ead","unresolved":false,"context_lines":[{"line_number":225,"context_line":"        \"\"\""},{"line_number":226,"context_line":"        try:"},{"line_number":227,"context_line":"            volume \u003d self.cinder_util.get_volume(self.volume_id)"},{"line_number":228,"context_line":"        except cinder_exception.NotFound:"},{"line_number":229,"context_line":"            raise exception.ActionSkipped("},{"line_number":230,"context_line":"                _(\"Volume %s not found\") % self.volume_id"},{"line_number":231,"context_line":"            )"}],"source_content_type":"text/x-python","patch_set":2,"id":"97295b78_bbe82134","line":228,"updated":"2026-06-25 11:41:14.000000000","message":"pre_condition catches cinder_exception.NotFound from get_volume, but get_volume is now decorated with @handle_cinder_error which converts NotFound into StorageResourceNotFound. In production the except branch is dead code: a missing volume propagates unhandled.\n\n**Severity**: HIGH | **Confidence**: 0.9\n\n**Risk**: When a volume is not found, the volume migration action no longer skips gracefully; it fails the action pipeline with an unexpected StorageResourceNotFound rather than ActionSkipped, changing documented runtime behavior.\n\n**Priority**: Before merge\n**Why This Matters**: The existing test (test_pre_condition_volume_not_found) passes only because it mocks get_volume to raise the raw cinder exception, bypassing the decorator. In real deployments the decorator always translates NotFound, so the graceful skip path is never taken.\n\n**Recommendation**:\nCatch exception.StorageResourceNotFound (and optionally keep cinder_exception.NotFound as a fallback). For example: except (exception.StorageResourceNotFound, cinder_exception.NotFound): and add a non-mocked test that exercises the decorator path.","commit_id":"4723f99f3a9f6774f6db175f83e938f04ffabe69"}],"watcher/common/cinder_helper.py":[{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"04f42407042544cbc6838a059ca59e9ad3d90ebf","unresolved":false,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"def handle_cinder_error(resource_type, id_arg_index\u003d1):"},{"line_number":31,"context_line":"    \"\"\"Decorator to handle exceptions from novaclient."},{"line_number":32,"context_line":""},{"line_number":33,"context_line":"    This decorator catches novaclient exceptions and handles them as follows:"},{"line_number":34,"context_line":"    - NotFound exceptions: logs a debug message and raises"}],"source_content_type":"text/x-python","patch_set":1,"id":"29dad517_7564d1a0","line":31,"updated":"2026-06-25 06:59:52.000000000","message":"The handle_cinder_error decorator docstring and log messages are copy-pasted from NovaHelper without updating the service name. The docstring says \u0027novaclient exceptions\u0027, \u0027ComputeResourceNotFound\u0027, and \u0027NovaClientError\u0027, and line 61 logs \u0027Nova client error\u0027 for Cinder errors.\n\n**Severity**: WARNING | **Confidence**: 1.0\n\n**Impact**: Incorrect docstring and log messages will mislead operators debugging Cinder issues. Searching logs for \u0027Nova client error\u0027 during a Cinder failure will produce confusing results, and the decorator documentation is factually wrong.\n\n**Suggestion**:\nUpdate the docstring to reference cinderclient exceptions, StorageResourceNotFound, and CinderClientError. Change the LOG.error on line 61 from \u0027Nova client error: %s\u0027 to \u0027Cinder client error: %s\u0027.","commit_id":"bcb15e0cd4849841745eff3b802622ce33794602"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"04f42407042544cbc6838a059ca59e9ad3d90ebf","unresolved":false,"context_lines":[{"line_number":44,"context_line":"    :returns: Decorator function"},{"line_number":45,"context_line":"    \"\"\""},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"    def decorator(func):"},{"line_number":48,"context_line":"        @functools.wraps(func)"},{"line_number":49,"context_line":"        def wrapper(*args, **kwargs):"},{"line_number":50,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"67fd509f_b1d75506","line":47,"updated":"2026-06-25 06:59:52.000000000","message":"There are no unit tests for the handle_cinder_error decorator\u0027s exception-translation behavior itself. No test verifies that cinder_exception.NotFound is converted to StorageResourceNotFound or that cinder_exception.ClientException is converted to CinderClientError.\n\n**Severity**: WARNING | **Confidence**: 0.9\n\n**Impact**: The decorator\u0027s core contract (translate NotFound to StorageResourceNotFound, translate ClientException to CinderClientError) is untested. As shown by the _can_get_volume regression, the lack of integration-level tests for the decorator allows behavioral bugs to slip through.\n\n**Suggestion**:\nAdd tests that set cinder API mocks to raise cinder_exception.NotFound and cinder_exception.ClientException, then assert the correct Watcher exceptions are raised from the decorated methods (get_volume_list, get_volume, etc.) without mocking get_volume itself.","commit_id":"bcb15e0cd4849841745eff3b802622ce33794602"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"04f42407042544cbc6838a059ca59e9ad3d90ebf","unresolved":false,"context_lines":[{"line_number":134,"context_line":"            value \u003d capabilities.get(field)"},{"line_number":135,"context_line":"            if value \u003d\u003d \"unknown\" or value is None:"},{"line_number":136,"context_line":"                # NOTE (jgilaber) according to the cinder V3 api does"},{"line_number":137,"context_line":"                # https://docs.openstack.org/api-ref/block-storage/v3/index.html#services-os-services"},{"line_number":138,"context_line":"                # the capacity values can be the string \u0027unknown\u0027"},{"line_number":139,"context_line":"                capacity_values[field] \u003d None"},{"line_number":140,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":1,"id":"c447d8af_496227e1","line":137,"updated":"2026-06-25 06:59:52.000000000","message":"The StoragePool.from_cinderclient comment on lines 136-137 references the Cinder services API doc URL (index.html#services-os-services) when describing capacity values for pools, which is the wrong API endpoint.\n\n**Severity**: SUGGESTION | **Confidence**: 0.8\n\n**Benefit**: Accurate documentation helps future developers understand why \u0027unknown\u0027 capacity values are handled specially.\n\n**Recommendation**:\nUpdate the URL to point to the scheduler stats or pools API endpoint which is where pool capacity values originate.","commit_id":"bcb15e0cd4849841745eff3b802622ce33794602"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"04f42407042544cbc6838a059ca59e9ad3d90ebf","unresolved":false,"context_lines":[{"line_number":171,"context_line":"    metadata: dict"},{"line_number":172,"context_line":"    bootable: str"},{"line_number":173,"context_line":"    created_at: str"},{"line_number":174,"context_line":"    tenant_id: str"},{"line_number":175,"context_line":"    host: str | None"},{"line_number":176,"context_line":"    migration_status: str | None"},{"line_number":177,"context_line":"    name_id: str | None"}],"source_content_type":"text/x-python","patch_set":1,"id":"331891ce_253a2b6e","line":174,"updated":"2026-06-25 06:59:52.000000000","message":"The Volume dataclass declares tenant_id as \u0027str\u0027 (non-Optional) but from_cinderclient assigns vol_dict.get(\u0027os-vol-tenant-attr:tenant_id\u0027) which can return None. The test test_with_none_optional_fields explicitly passes tenant_id\u003dNone, confirming None is a valid value that contradicts the type hint.\n\n**Severity**: WARNING | **Confidence**: 0.8\n\n**Impact**: Code that relies on the type hint str for tenant_id may not handle None, leading to AttributeError or incorrect behavior when a volume has no tenant assignment. This is a type-safety gap in a change whose stated goal is to provide type safety.\n\n**Suggestion**:\nChange the tenant_id field declaration from \u0027tenant_id: str\u0027 to \u0027tenant_id: str | None\u0027 to match the actual runtime values and the existing host/migration_status/name_id/snapshot_id Optional fields.","commit_id":"bcb15e0cd4849841745eff3b802622ce33794602"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"04f42407042544cbc6838a059ca59e9ad3d90ebf","unresolved":false,"context_lines":[{"line_number":226,"context_line":""},{"line_number":227,"context_line":""},{"line_number":228,"context_line":"@dc.dataclass(frozen\u003dTrue)"},{"line_number":229,"context_line":"class VolumeSnapshot:"},{"line_number":230,"context_line":"    \"\"\"Pure dataclass for Cinder volume snapshot data."},{"line_number":231,"context_line":""},{"line_number":232,"context_line":"    Extracted from cinderclient Snapshot object with all attributes"}],"source_content_type":"text/x-python","patch_set":1,"id":"dfb8c9c6_681e5f55","line":229,"updated":"2026-06-25 06:59:52.000000000","message":"VolumeSnapshot only wraps a single field (volume_id). If other snapshot attributes are needed later by consumer code, they will require another change. Consider whether id, status, or size should be included now to avoid a follow-up.\n\n**Severity**: SUGGESTION | **Confidence**: 0.6\n\n**Benefit**: Forward-looking field coverage reduces future churn if snapshot attributes are needed by strategies.\n\n**Recommendation**:\nThis is acceptable for now since only volume_id is consumed. Document that the wrapper is intentionally minimal, or add commonly-used fields like id and status if they are cheap to include.","commit_id":"bcb15e0cd4849841745eff3b802622ce33794602"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"04f42407042544cbc6838a059ca59e9ad3d90ebf","unresolved":false,"context_lines":[{"line_number":362,"context_line":"            volume \u003d self.get_volume(volume_id)"},{"line_number":363,"context_line":"            if not volume:"},{"line_number":364,"context_line":"                raise Exception"},{"line_number":365,"context_line":"        except cinder_exception.NotFound:"},{"line_number":366,"context_line":"            return False"},{"line_number":367,"context_line":"        else:"},{"line_number":368,"context_line":"            return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"7ea0fd75_ba2c856b","line":365,"updated":"2026-06-25 06:59:52.000000000","message":"The @handle_cinder_error decorator on get_volume converts NotFound to StorageResourceNotFound, but _can_get_volume (line 365) catches cinder_exception.NotFound. The decorator fires first in production, so _can_get_volume never returns False for a deleted volume and crashes check_volume_deleted.\n\n**Severity**: HIGH | **Confidence**: 0.9\n\n**Risk**: During migration cleanup, check_volume_deleted calls _can_get_volume -\u003e get_volume. If the volume is gone, the decorator converts NotFound to StorageResourceNotFound, which _can_get_volume does not catch. The exception propagates, crashing migration instead of returning False.\n\n**Priority**: Before merge\n**Why This Matters**: This silently changes the control flow of volume-deletion verification. The unit test (test_can_get_volume_fail) masks the bug by mocking get_volume directly, bypassing the decorator. In production the decorator is live and the regression is real.\n\n**Recommendation**:\nEither update _can_get_volume to catch exception.StorageResourceNotFound instead of (or in addition to) cinder_exception.NotFound, or remove the @handle_cinder_error decorator from get_volume since it already has its own internal NotFound handling for the find() fallback. Add a test that exercises the real decorated get_volume path (not a mock) to verify _can_get_volume returns False when the volume is absent.","commit_id":"bcb15e0cd4849841745eff3b802622ce33794602"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"04f42407042544cbc6838a059ca59e9ad3d90ebf","unresolved":false,"context_lines":[{"line_number":498,"context_line":""},{"line_number":499,"context_line":"        return True"},{"line_number":500,"context_line":""},{"line_number":501,"context_line":"    def migrate(self, volume, dest_node):"},{"line_number":502,"context_line":"        \"\"\"Migrate volume to dest_node\"\"\""},{"line_number":503,"context_line":"        volume \u003d self.get_volume(volume)"},{"line_number":504,"context_line":"        pool \u003d self.get_storage_pool_by_name(dest_node)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9576c23e_af4034d6","line":501,"updated":"2026-06-25 06:59:52.000000000","message":"The @handle_cinder_error decorator is applied to all read/list methods but not to migrate() and retype(), which call cinder.volumes.migrate_volume() and retype(). NovaHelper decorates write methods too, so cinder client exceptions from these will propagate as raw cinderclient exceptions.\n\n**Severity**: WARNING | **Confidence**: 0.8\n\n**Impact**: If migrate_volume or retype raises a cinder_exception.ClientException, it will propagate unwrapped rather than being converted to CinderClientError, creating an inconsistency in how callers must handle errors from CinderHelper methods.\n\n**Suggestion**:\nEither decorate migrate() and retype() with @handle_cinder_error, or document why these orchestration methods are intentionally left undecorated while the individual API-call methods are wrapped.","commit_id":"bcb15e0cd4849841745eff3b802622ce33794602"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"abe8acfb98bf8c9b72d5d7fc2c46965d909f4dd1","unresolved":false,"context_lines":[{"line_number":58,"context_line":"                    resource_id \u003d \u0027unknown\u0027"},{"line_number":59,"context_line":"                LOG.debug(\"%s %s was not found\", resource_type, resource_id)"},{"line_number":60,"context_line":"                msg \u003d f\"{resource_id} of type {resource_type}\""},{"line_number":61,"context_line":"                raise exception.StorageResourceNotFound(msg)"},{"line_number":62,"context_line":"            except cinder_exception.ClientException as e:"},{"line_number":63,"context_line":"                LOG.error(\"Cinder client error: %s\", e)"},{"line_number":64,"context_line":"                raise exception.CinderClientError(reason\u003dstr(e))"}],"source_content_type":"text/x-python","patch_set":3,"id":"e7dda3cb_7a6fff25","line":61,"updated":"2026-06-25 13:54:48.000000000","message":"The handle_cinder_error decorator raises StorageResourceNotFound with a positional message argument, bypassing the msg_fmt template. StorageResourceNotFound.msg_fmt expects a \u0027name\u0027 kwarg, but the decorator passes a raw f-string as the positional \u0027message\u0027 arg, so the i18n template is never applied.\n\n**Severity**: HIGH | **Confidence**: 0.9\n\n**Risk**: Exception messages lose i18n translation and are inconsistent with StorageNodeNotFound and PoolNotFound which use the name\u003d kwarg. Operators see raw text instead of the documented template message.\n\n**Priority**: Before merge\n**Why This Matters**: All NotFound paths through the decorator (get_volume, get_volume_list, get_storage_pool_list, etc.) produce un-translated, inconsistent error messages that differ from the existing StorageNodeNotFound/PoolNotFound pattern used elsewhere in the same module.\n\n**Recommendation**:\nPass the resource identifier as a kwarg matching the template: raise exception.StorageResourceNotFound(name\u003dmsg) instead of raise exception.StorageResourceNotFound(msg).","commit_id":"4f5171fdc56b7660e39a6c3e93cc77a15f75a7c4"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"abe8acfb98bf8c9b72d5d7fc2c46965d909f4dd1","unresolved":false,"context_lines":[{"line_number":260,"context_line":"            for s in self.cinder.services.list(binary\u003d\u0027cinder-volume\u0027)"},{"line_number":261,"context_line":"        ]"},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"    @handle_cinder_error(\"Storage service\")"},{"line_number":264,"context_line":"    def get_storage_node_by_name(self, name):"},{"line_number":265,"context_line":"        \"\"\"Get storage node by name(host@backendname)\"\"\""},{"line_number":266,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":3,"id":"721cff9e_838a40ff","line":263,"updated":"2026-06-25 13:54:48.000000000","message":"The @handle_cinder_error decorator on get_storage_node_by_name and get_storage_pool_by_name is dead code. Both methods have an inner \u0027except Exception as exc\u0027 that catches all exceptions (including cinder_exception.NotFound and ClientException) and re-raises before the decorator can ever see them.\n\n**Severity**: WARNING | **Confidence**: 0.9\n\n**Impact**: The decorator provides no exception handling value on these two methods because the broad inner except Exception swallows everything first. This is misleading to maintainers who expect the decorator to convert cinderclient exceptions here.\n\n**Suggestion**:\nEither remove the @handle_cinder_error decorator from get_storage_node_by_name and get_storage_pool_by_name, or narrow the inner except to let cinderclient exceptions propagate to the decorator.","commit_id":"4f5171fdc56b7660e39a6c3e93cc77a15f75a7c4"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"abe8acfb98bf8c9b72d5d7fc2c46965d909f4dd1","unresolved":false,"context_lines":[{"line_number":346,"context_line":"        try:"},{"line_number":347,"context_line":"            v \u003d self.cinder.volumes.get(volume_id)"},{"line_number":348,"context_line":"            return Volume.from_cinderclient(v)"},{"line_number":349,"context_line":"        except cinder_exception.NotFound:"},{"line_number":350,"context_line":"            v \u003d self.cinder.volumes.find(name\u003dvolume_id)"},{"line_number":351,"context_line":"            return Volume.from_cinderclient(v)"},{"line_number":352,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"b9a73e59_a89e3337","line":349,"updated":"2026-06-25 13:54:48.000000000","message":"The get_volume method has both the @handle_cinder_error decorator AND an inner try/except for cinder_exception.NotFound. The inner catch handles the fallback to volumes.find(). While functionally correct, the dual-layer NotFound handling is subtle and warrants a clarifying comment.\n\n**Severity**: SUGGESTION | **Confidence**: 0.8\n\n**Benefit**: A brief comment explaining that the inner catch provides the find() fallback while the decorator handles the case where find() also fails would prevent future maintainers from mistakenly removing one layer.\n\n**Recommendation**:\nAdd an inline comment such as: \u0027# NotFound from get() triggers fallback to find(); if find() also raises NotFound, the @handle_cinder_error decorator converts it to StorageResourceNotFound.\u0027","commit_id":"4f5171fdc56b7660e39a6c3e93cc77a15f75a7c4"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"576253600ace7ff6ae74d471b8f57643706175f9","unresolved":false,"context_lines":[{"line_number":96,"context_line":"        )"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":""},{"line_number":99,"context_line":"@dc.dataclass"},{"line_number":100,"context_line":"class StoragePool:"},{"line_number":101,"context_line":"    \"\"\"Pure dataclass for Cinder storage pool data."},{"line_number":102,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"f3f6f8a5_75197e71","line":99,"updated":"2026-06-25 15:50:42.000000000","message":"The StoragePool dataclass is intentionally non-frozen to allow mutation of free_capacity_gb by storage_capacity_balance. This mutable state is shared across the strategy execution and could cause subtle bugs if the same pool list is reused.\n\n**Severity**: SUGGESTION | **Confidence**: 0.7\n\n**Benefit**: Documenting the mutation contract explicitly prevents future developers from accidentally freezing this class or assuming immutability. The docstring mentions it but a class-level comment would be more visible.\n\n**Recommendation**:\nConsider adding a brief note near the free_capacity_gb field definition highlighting that storage_capacity_balance mutates it during pool selection, so the mutation contract is obvious at the field level, not just in the class docstring.","commit_id":"f9121e9826101acc5a5f92278bfa66068c4eeab5"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"576253600ace7ff6ae74d471b8f57643706175f9","unresolved":false,"context_lines":[{"line_number":136,"context_line":"            value \u003d capabilities.get(field)"},{"line_number":137,"context_line":"            if value \u003d\u003d \"unknown\" or value is None:"},{"line_number":138,"context_line":"                # NOTE (jgilaber) according to the cinder V3 api docs"},{"line_number":139,"context_line":"                # https://docs.openstack.org/api-ref/block-storage/v3/index.html#back-end-storage-pools"},{"line_number":140,"context_line":"                # the capacity values can be the string \u0027unknown\u0027"},{"line_number":141,"context_line":"                capacity_values[field] \u003d None"},{"line_number":142,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":4,"id":"d74ab0da_b86f95d8","line":139,"updated":"2026-06-25 15:50:42.000000000","message":"URL comment on line 139 exceeds the 79-character pep8 limit (103 chars). This is a newly introduced line that will fail tox -e pep8 / ruff E501 checks.\n\n**Severity**: WARNING | **Confidence**: 1.0\n\n**Impact**: The CI pep8/ruff linting job will fail on this line, blocking merge. Watcher enforces a strict 79-character line length per its ruff configuration.\n\n**Suggestion**:\nShorten the URL or wrap the comment. For example, use a shortened reference or break the URL across lines with a comment continuation. Alternatively, reference the Cinder API docs section name without the full URL.","commit_id":"f9121e9826101acc5a5f92278bfa66068c4eeab5"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"576253600ace7ff6ae74d471b8f57643706175f9","unresolved":false,"context_lines":[{"line_number":201,"context_line":"            created_at\u003dvolume.created_at,"},{"line_number":202,"context_line":"            migration_status\u003dvolume.migration_status,"},{"line_number":203,"context_line":"            host\u003dvol_dict.get(\u0027os-vol-host-attr:host\u0027),"},{"line_number":204,"context_line":"            tenant_id\u003dvol_dict[\u0027os-vol-tenant-attr:tenant_id\u0027],"},{"line_number":205,"context_line":"            name_id\u003dvol_dict.get(\u0027os-vol-mig-status-attr:name_id\u0027),"},{"line_number":206,"context_line":"        )"},{"line_number":207,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"27583151_97887604","line":204,"updated":"2026-06-25 15:50:42.000000000","message":"Volume.from_cinderclient uses bracket access vol_dict[\u0027os-vol-tenant-attr:tenant_id\u0027] which raises uncaught KeyError if the attribute is absent. Other hyphenated attrs use .get() for safety.\n\n**Severity**: HIGH | **Confidence**: 0.8\n\n**Risk**: A KeyError will crash the calling method and propagate as an unhandled exception. The handle_cinder_error decorator only catches cinder_exception types, not KeyError, so this would crash the watcher decision engine or applier.\n\n**Priority**: Before merge\n**Why This Matters**: Cinder volumes may not always have os-vol-tenant-attr:tenant_id populated, especially in edge cases or certain Cinder configurations. The inconsistent use of bracket access vs .get() for hyphenated attributes introduces a crash risk that did not exist with getattr().\n\n**Recommendation**:\nChange vol_dict[\u0027os-vol-tenant-attr:tenant_id\u0027] to vol_dict.get(\u0027os-vol-tenant-attr:tenant_id\u0027) to match the pattern used for host and name_id on lines 203 and 205. If tenant_id is truly required, add explicit validation and raise a descriptive WatcherException instead of letting KeyError propagate.","commit_id":"f9121e9826101acc5a5f92278bfa66068c4eeab5"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"576253600ace7ff6ae74d471b8f57643706175f9","unresolved":false,"context_lines":[{"line_number":235,"context_line":"    resolved at construction time."},{"line_number":236,"context_line":"    \"\"\""},{"line_number":237,"context_line":""},{"line_number":238,"context_line":"    volume_id: str"},{"line_number":239,"context_line":""},{"line_number":240,"context_line":"    @classmethod"},{"line_number":241,"context_line":"    def from_cinderclient(cls, snapshot):"}],"source_content_type":"text/x-python","patch_set":4,"id":"cc523da0_01d01c5f","line":238,"updated":"2026-06-25 15:50:42.000000000","message":"VolumeSnapshot dataclass only exposes volume_id, discarding id, status, and other fields from the cinderclient Snapshot. The test factory creates snapshots with id and status fields that are silently ignored.\n\n**Severity**: SUGGESTION | **Confidence**: 0.7\n\n**Benefit**: Adding at minimum an id field to VolumeSnapshot would make the dataclass more useful for debugging and consistent with other wrappers. If the strategy code only needs volume_id, this is acceptable but should be documented.\n\n**Recommendation**:\nIf only volume_id is needed by consumers, add a comment explaining the intentional minimality. Otherwise, consider adding id and status fields for future-proofing and to avoid losing data that might be needed later.","commit_id":"f9121e9826101acc5a5f92278bfa66068c4eeab5"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"576253600ace7ff6ae74d471b8f57643706175f9","unresolved":false,"context_lines":[{"line_number":260,"context_line":"            for s in self.cinder.services.list(binary\u003d\u0027cinder-volume\u0027)"},{"line_number":261,"context_line":"        ]"},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"    @handle_cinder_error(\"Storage service\")"},{"line_number":264,"context_line":"    def get_storage_node_by_name(self, name):"},{"line_number":265,"context_line":"        \"\"\"Get storage node by name(host@backendname)\"\"\""},{"line_number":266,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":4,"id":"deb22598_8a79ad08","line":263,"updated":"2026-06-25 15:50:42.000000000","message":"The handle_cinder_error decorator on get_storage_node_by_name and get_storage_pool_by_name is redundant. These methods already have their own try/except Exception blocks that handle all errors internally.\n\n**Severity**: SUGGESTION | **Confidence**: 0.7\n\n**Benefit**: Removing the decorator from these two methods avoids double exception handling and makes the control flow clearer. The decorator\u0027s NotFound-to-StorageResourceNotFound conversion will never fire because the inner try/except catches everything first.\n\n**Recommendation**:\nConsider removing @handle_cinder_error from get_storage_node_by_name and get_storage_pool_by_name. Their broad except Exception blocks already convert all errors to StorageNodeNotFound/PoolNotFound. If the decorator is kept for consistency, document why.","commit_id":"f9121e9826101acc5a5f92278bfa66068c4eeab5"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"576253600ace7ff6ae74d471b8f57643706175f9","unresolved":false,"context_lines":[{"line_number":433,"context_line":"        final_status \u003d (\u0027success\u0027, \u0027error\u0027)"},{"line_number":434,"context_line":"        while volume.migration_status not in final_status:"},{"line_number":435,"context_line":"            volume \u003d self.get_volume(volume.id)"},{"line_number":436,"context_line":"            LOG.debug(\u0027Waiting the migration of %s\u0027, volume)"},{"line_number":437,"context_line":"            time.sleep(retry_interval)"},{"line_number":438,"context_line":"            if volume.migration_status \u003d\u003d \u0027error\u0027:"},{"line_number":439,"context_line":"                error_msg \u003d ("}],"source_content_type":"text/x-python","patch_set":4,"id":"cccf9651_f7bbca4d","line":436,"updated":"2026-06-25 15:50:42.000000000","message":"LOG.debug(\u0027Waiting the migration of %s\u0027, volume) on line 436 logs the entire Volume dataclass object. With the new frozen dataclass, this produces a verbose repr of all 15 fields every retry interval.\n\n**Severity**: WARNING | **Confidence**: 0.8\n\n**Impact**: Log files will be polluted with full dataclass repr output during migration polling loops (every 10 seconds), making it harder to diagnose issues. The check_retyped method correctly uses volume.id on line 491.\n\n**Suggestion**:\nChange to LOG.debug(\u0027Waiting the migration of %s\u0027, volume.id) for consistency with check_retyped (line 491) and to avoid logging potentially sensitive metadata/tenant_id fields.","commit_id":"f9121e9826101acc5a5f92278bfa66068c4eeab5"}],"watcher/decision_engine/model/collector/cinder.py":[{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"04f42407042544cbc6838a059ca59e9ad3d90ebf","unresolved":false,"context_lines":[{"line_number":239,"context_line":"        node_attributes \u003d {\"name\": pool.name}"},{"line_number":240,"context_line":"        for attr in attrs:"},{"line_number":241,"context_line":"            try:"},{"line_number":242,"context_line":"                node_attributes[attr] \u003d int(getattr(pool, attr))"},{"line_number":243,"context_line":"            except AttributeError:"},{"line_number":244,"context_line":"                LOG.debug("},{"line_number":245,"context_line":"                    \"Attribute %s for pool %s is not provided\", attr, pool.name"}],"source_content_type":"text/x-python","patch_set":1,"id":"05931f9d_8528fb9d","line":242,"updated":"2026-06-25 06:59:52.000000000","message":"_build_storage_pool does int(getattr(pool, attr)) but StoragePool.from_cinderclient now converts \u0027unknown\u0027 capacity values to None. int(None) raises TypeError, not caught by the except AttributeError or except ValueError handlers, bypassing the clean InvalidPoolAttributeValue path.\n\n**Severity**: WARNING | **Confidence**: 0.8\n\n**Impact**: A pool reporting \u0027unknown\u0027 for total_capacity_gb or free_capacity_gb will cause an unhandled TypeError in _build_storage_pool instead of the intended InvalidPoolAttributeValue. While execute may still wrap it into ClusterDataModelCollectionError, the specific error handling is lost.\n\n**Suggestion**:\nAdd \u0027except TypeError\u0027 alongside the existing \u0027except ValueError\u0027 handler in _build_storage_pool, or handle None explicitly before calling int(). Alternatively, keep the \u0027unknown\u0027 string rather than converting to None for capacity fields consumed by the collector.","commit_id":"bcb15e0cd4849841745eff3b802622ce33794602"}],"watcher/decision_engine/strategy/strategies/storage_capacity_balance.py":[{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"04f42407042544cbc6838a059ca59e9ad3d90ebf","unresolved":false,"context_lines":[{"line_number":217,"context_line":"            if free_cap \u003e (1 - threshold) * total_cap:"},{"line_number":218,"context_line":"                target_pool_name \u003d pool.name"},{"line_number":219,"context_line":"                index \u003d self.dest_pools.index(pool)"},{"line_number":220,"context_line":"                self.dest_pools[index].free_capacity_gb \u003d str(free_cap)"},{"line_number":221,"context_line":"                LOG.info("},{"line_number":222,"context_line":"                    \"volume: get pool %s for vol %s\","},{"line_number":223,"context_line":"                    target_pool_name,"}],"source_content_type":"text/x-python","patch_set":1,"id":"2c8f8985_834f97ce","line":220,"updated":"2026-06-25 06:59:52.000000000","message":"storage_capacity_balance assigns str(free_cap) to StoragePool.free_capacity_gb, but the dataclass field is declared as \u0027float | None\u0027. The old code used setattr with a string, but now that the field has an explicit type annotation the assignment violates it.\n\n**Severity**: SUGGESTION | **Confidence**: 0.8\n\n**Benefit**: Type consistency between the dataclass declaration and runtime values. If static type checking is added later, this assignment would flag as an error.\n\n**Recommendation**:\nAssign the float value directly (self.dest_pools[index].free_capacity_gb \u003d free_cap) instead of str(free_cap). The consumer code at lines 159-170 already wraps the value in float(), so the string conversion is unnecessary.","commit_id":"bcb15e0cd4849841745eff3b802622ce33794602"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"fb0665922af732f12507e04f3d5075025e850ead","unresolved":false,"context_lines":[{"line_number":240,"context_line":"            )"},{"line_number":241,"context_line":"        )"},{"line_number":242,"context_line":"        if volume_type:"},{"line_number":243,"context_line":"            src_extra_specs \u003d volume_type[0].extra_specs"},{"line_number":244,"context_line":"            src_extra_specs.pop(\u0027volume_backend_name\u0027, None)"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"        backendname \u003d dest_pool.volume_backend_name"}],"source_content_type":"text/x-python","patch_set":2,"id":"d1dbdd57_5f77bc50","line":243,"updated":"2026-06-25 11:41:14.000000000","message":"check_pool_type mutates the shared extra_specs dict of a frozen VolumeType dataclass via .pop(\u0027volume_backend_name\u0027). The dict field is mutable by reference; mutating a returned object\u0027s nested dict is surprising.\n\n**Severity**: SUGGESTION | **Confidence**: 0.9\n\n**Benefit**: Avoids aliasing the dataclass\u0027s internal dict and prevents accidental mutation of shared state if get_volume_type_list() is ever cached in a future change.\n\n**Recommendation**:\nCopy before mutating: src_extra_specs \u003d dict(volume_type[0].extra_specs) then src_extra_specs.pop(\u0027volume_backend_name\u0027, None).","commit_id":"4723f99f3a9f6774f6db175f83e938f04ffabe69"}],"watcher/decision_engine/strategy/strategies/zone_migration.py":[{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"576253600ace7ff6ae74d471b8f57643706175f9","unresolved":false,"context_lines":[{"line_number":19,"context_line":"from watcher.common import cinder_helper"},{"line_number":20,"context_line":"from watcher.common import exception"},{"line_number":21,"context_line":"from watcher.common import nova_helper"},{"line_number":22,"context_line":"from watcher.common.cinder_helper import Volume"},{"line_number":23,"context_line":"from watcher.decision_engine.model import element"},{"line_number":24,"context_line":"from watcher.decision_engine.strategy.strategies import base"},{"line_number":25,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"214afe29_a92ea605","line":22,"updated":"2026-06-25 15:50:42.000000000","message":"zone_migration.py imports cinder_helper as a module (line 19) AND separately imports Volume from it (line 22). This mixed import pattern is inconsistent and creates confusion about the canonical access path.\n\n**Severity**: WARNING | **Confidence**: 0.8\n\n**Impact**: Future maintainers may not realize that cinder_helper.Volume and the directly imported Volume are the same object. This pattern deviates from the rest of the codebase where helper classes are accessed via the module.\n\n**Suggestion**:\nUse cinder_helper.Volume throughout the file instead of a direct import, or if a direct import is preferred for the isinstance check, add a noqa or restructure to import only from the module.","commit_id":"f9121e9826101acc5a5f92278bfa66068c4eeab5"}],"watcher/tests/unit/applier/actions/test_volume_migration.py":[{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"tag":"autogenerated:zuul:automatic-ci","change_message_id":"fb0665922af732f12507e04f3d5075025e850ead","unresolved":false,"context_lines":[{"line_number":249,"context_line":"        )"},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"    def test_pre_condition_volume_not_found(self):"},{"line_number":252,"context_line":"        self.m_c_helper.get_volume.side_effect \u003d cinder_exception.NotFound("},{"line_number":253,"context_line":"            \u0027404\u0027"},{"line_number":254,"context_line":"        )"},{"line_number":255,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"8b43788e_c4152c7e","line":252,"updated":"2026-06-25 11:41:14.000000000","message":"The unit test test_pre_condition_volume_not_found stubs get_volume.side_effect \u003d cinder_exception.NotFound, which replaces the decorated method entirely and hides the fact that the real method now raises StorageResourceNotFound. This masks the dead exception handler in pre_condition.\n\n**Severity**: WARNING | **Confidence**: 0.8\n\n**Impact**: The test asserts ActionSkipped is raised, but because the mock short-circuits the decorator, it validates behavior that cannot occur in production. A real NotFound will not produce ActionSkipped.\n\n**Suggestion**:\nAdd or adjust a test that mocks the underlying cinder.volumes.get/find calls (so the decorator runs) and assert that ActionSkipped is raised via StorageResourceNotFound, ensuring the production path is covered.","commit_id":"4723f99f3a9f6774f6db175f83e938f04ffabe69"}]}