Cover vm_manager_cluster and get the main gate back to green - #97
Open
insatomcat wants to merge 9 commits into
Open
Cover vm_manager_cluster and get the main gate back to green#97insatomcat wants to merge 9 commits into
insatomcat wants to merge 9 commits into
Conversation
vm_manager_cluster.py was at 5.6%, and clone() alone accounted for 112 of the 408 uncovered units the SonarCloud new code period counts on that module. Cover it whole, lines and branches. The suite replaces every cluster access: RbdManager by a recording fake usable as a context manager, Pacemaker by a stub host validator, and the three functions clone() delegates to by recorders, so a failure points at clone() and nowhere else. The fake only exposes the methods clone() is expected to call, so an unexpected one fails with an AttributeError instead of returning a Mock. A copy carries the source metadata over, because clone() relies on that to know what to strip from the clone. One test is a strict xfail: the rollback loops over src_additional_count, which is only assigned after the system disk copy, so any failure before that raises UnboundLocalError from the except branch and hides the error that caused the rollback. The fix belongs to a change of its own. The module reaches 24%, and new_coverage would go from 21.3% to 37.8%. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
Next two functions of the module by uncovered weight, 63 and 41 of the 408 units the new code period counts there. Both are covered whole, lines and branches, which brings the module to 40% and the three functions done so far to 217 of those 408 units. create() needs two collaborators clone() did not: _create_xml, which shells out to ceph orch and reads the libvirt secrets, and check_uuid_conflict, which walks every group in Ceph. Both become recorders. The file existence checks run for real against files in tmp_path, with CEPH_CONF pointed at one of them, rather than against a patched os.path. add_to_cluster() reads the source domain from libvirt, so the fake LibVirtManager here also exposes the _conn.lookupByName path the function uses directly, and a fake domain with the three methods it calls on it. One more strict xfail: create() validates pacemaker_meta, pacemaker_params and pacemaker_utilization inside the 'if "metadata" in vm_options' block, so a non-dict goes through unchecked whenever no metadata is passed. _configure_vm then stores the string as JSON and the next clone of that VM fails on it. clone() validates the same three options unconditionally. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
Two more functions of the module, 31 units each. The module reaches 63% and the five functions done so far clear 278 of the 408 units the new code period counts there. Both are reached through the recorders the earlier tests installed, so each gets a fixture putting the real function back: the clone and create tests need _configure_vm mocked, and _configure_vm itself needs enable_vm mocked. The Pacemaker stub grows from a bare host validator into a recording context manager, since enable_vm drives it through eleven calls, and the cluster resources live on the shared collaborators because the code opens a new Pacemaker per operation. enable_vm reads fifteen metadata keys, each in its own try/except. Rather than one test per key, one test asserts every default when the disk carries nothing and one asserts every value when it carries all of them, which covers both sides of all fifteen. One branch in _configure_vm stays uncovered because it cannot be reached: the "disk_bus" in vm_options guard is dead code, the unconditional vm_options["disk_bus"] earlier in the same function already requires the key. Removing the guard belongs to a change of its own. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
src_additional_count was assigned after the system disk copy, but the except branch loops over it to put the source additional disks back in their group. Any failure before that assignment therefore left it unbound: image_exists, remove_image or copy_image raising, or a copy that reports success and creates nothing. The rollback then raised UnboundLocalError, and the caller saw that instead of the real error. Bind it before the try. The test that documented the behaviour as a strict xfail becomes a plain test. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
The three isinstance checks on pacemaker_meta, pacemaker_params and pacemaker_utilization sat inside the "if metadata in vm_options" block, so they only ran when the caller also passed metadata. Without it a string went through unchecked, _configure_vm stored it as JSON, and the next clone of that VM failed on reading it back with "pacemaker_meta metadata must be a dictionary", far from the call that caused it. Dedent the loop, so create() validates the three options the way clone() already does. The strict xfail that documented the hole becomes a plain test, parametrized over the three options. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
The "disk_bus" in vm_options check could never be false: the same function reads vm_options["disk_bus"] unconditionally when it builds the XML, forty lines above, so a missing key raises KeyError long before. The guard only made the branch impossible to cover. Keep the body, drop the condition. The five functions covered so far now have no uncovered line and no uncovered branch left. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
Three more functions, plus _get_all_disk_names which the purge tests reach on the way. The module goes to 75%, and only 60 of the 408 units the new code period counts on it are left, all in seven functions that have no test yet. The fake Ceph grows the group and snapshot side it had not needed so far: list_groups, group_exists, list_group_images, remove_group, the three snapshot calls and purge_image, plus two injection points for objects that survive their own removal, which is the only way to reach the two "Could not remove" guards in remove(). purge_image has three modes that share nothing, so they get three classes. The number mode realigns disks that hold more snapshots than the others, from a purge that failed halfway, and that is worth its own test since the arithmetic is not obvious. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
The last seven functions the new code period still counted: _create_xml, rollback_snapshot, console, create_snapshot, remove_snapshot, _get_remote_nodes and _get_ceph_hosts_xml, plus _get_observer_host which carries no weight but sat next to them. The module reaches 86% and owes the gate nothing. Three of them shell out, so the fake replaces the subprocess module inside vm_manager_cluster rather than patching the real one. _create_xml also reads the libvirt secrets and the Ceph monitor list, both of which the fakes now serve. _get_observer_host reads a hardcoded /etc/cluster.conf, so open is the only seam. It is replaced by a wrapper that redirects that single path to a temporary file and delegates every other one to the real open. What is left uncovered in the module is fourteen functions whose lines predate the new code period: disable_vm, start, stop, status, list_vms, the metadata accessors and the pacemaker_remote pair. They weigh nothing on the gate but still stand between the project and the OpenSSF target. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
Three paths could never be covered by the pytest suite and only dragged the coverage down: vm_manager/helpers/tests holds the integration scripts driven by hand against a real cluster, and conf.py and setup.py are build entry points. Together they account for 49 of the units the new code period counts, all of them uncovered, and no test will ever touch them. pyproject.toml already omits the integration scripts from the coverage run, with the same reasoning written next to it since the suite was introduced. This only puts SonarCloud in step with a decision the repository had already made and documented. sonar.coverage.exclusions rather than sonar.exclusions or sonar.tests on purpose: the files stay analysed, so the issues they carry keep being reported. Only the coverage expectation goes away. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
insatomcat
force-pushed
the
cluster-unit-coverage
branch
from
August 7, 2026 20:26
cc65cfe to
f6a3157
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After #94 and #96, one condition stayed red on
main:new_coverageat 21.3% for a threshold of 80. Of the 408 uncovered units the new code period counts, all 408 were invm_manager_cluster.py, a module at 5.6%.This branch covers it: 228 unit tests, no cluster needed. RbdManager, Pacemaker, LibVirtManager and the subprocess calls are replaced by recording fakes that expose only what the code is expected to call, so an unexpected call fails loudly instead of returning a Mock. The end-to-end suite in
test_vm_manager_cluster.pyis untouched and CI still ignores it.The module goes from 5.6% to 86% and owes the period nothing. Projected
new_coverageonmain: 21.3% to 87.0%.Three bugs turned up while writing the tests, each fixed in its own commit:
clonerolled back oversrc_additional_countbefore it was bound, so a failed system disk copy raisedUnboundLocalErrorand hid the real error.createvalidated the three pacemaker options only when metadata was passed too, so a string went through and the next clone of that VM failed on reading it back._configure_vmhad a"disk_bus" in vm_optionsguard that could never be false.The last commit adds one line to
sonar-project.properties. The hand-run integration scripts undervm_manager/helpers/tests, plusconf.pyandsetup.py, account for 49 uncovered units no test will ever reach, andpyproject.tomlalready omits the scripts from the coverage run for the same reason.sonar.coverage.exclusionsrather thansonar.exclusionsorsonar.testson purpose: the files stay analysed, so their issues keep being reported.Fourteen functions in the module are still uncovered. Their lines predate the new code period, so they weigh nothing on the gate, but they stand between the project and the OpenSSF gold target. The fakes are in place to deal with them in a next PR.