Skip to content

storage: report success from rollback and commit - #608

Merged
chombourger merged 1 commit into
siemens:nextfrom
Poseidonas:storage-rollback-commit-report-success
Aug 24, 2026
Merged

storage: report success from rollback and commit#608
chombourger merged 1 commit into
siemens:nextfrom
Poseidonas:storage-rollback-commit-report-success

Conversation

@Poseidonas

Copy link
Copy Markdown

Fixes #597.

mtda-cli storage rollback prints could not rollback changes made to shared storage! and exits non-zero after a rollback which did exactly what it was asked to, so the reporter has to write || : around it.

Where the false failure comes from

usbf.rollback()                 no return statement          -> None
main.storage_rollback()         result = self.storage...     -> None
grpc/servicer._bool_response()  BoolResponse(value=bool(v))  -> False
mtda-cli storage_rollback()     if status is False           -> prints, exits 1

bool(None) is where it turns into a failure. The same applies to commit, and to both the usbf and qemu controllers.

The fix

return True on the paths that completed, and on the ignore_missing path where there was nothing to do.

This is not a new convention: every other storage method already returns a value.

rollback commit to_host to_target mount open close
before no no yes yes yes yes yes

I checked the other callers before changing the signature — helpers/image.py:72, usbf.py:286 and qemu.py:64 all discard the return value, so only the CLI path behaves differently.

On ignore_missing

I chose True there. The only caller is helpers/image._close(), which ignores the result, so nothing depends on it; True seemed the better reading of "there was no copy-on-write device, so there was nothing to roll back" — False would report a failure that did not happen. Happy to flip it if you read it the other way.

What I verified, and what I could not

Verified by driving both controllers with the subprocess calls patched out: all four methods return True on the normal path and on ignore_missing. flake8 is clean with the exclusions from tox.ini.

I could not run scripts/test-using-docker here: the archlinux image has no linux/arm64 manifest, and mtda-service imports systemd, which is not installable on macOS. So this rests on reading the call chain and on the isolated check above rather than on the integration suite — worth a second look on that account.

I did not add a test. The suite is integration-based against a live service, and the docker backend has no rollback to exercise. If you would like one, I am glad to add it in whatever shape fits.

mtda-cli prints "could not rollback changes made to shared storage!" and
exits non-zero after a rollback which did what it was asked to, as reported
in siemens#597.

Neither rollback() nor commit() returns anything in the usbf and qemu
controllers, so they yield None. main.storage_rollback() passes that on,
and the gRPC servicer wraps it with bool(value), which turns None into
False. mtda-cli then takes `status is False` as a failure.

Both now return True on the paths that completed, and on the ignore_missing
path where there was nothing to do. Every other storage method - to_host,
to_target, mount, open, close - already returns a value; these four were the
exception.

The other callers of rollback() and commit() ignore the return value
(helpers/image.py, and the internal calls in usbf.py and qemu.py), so only
the CLI path changes behaviour.

Signed-off-by: George Vasiliades <1221374+Poseidonas@users.noreply.github.com>
@chombourger

Copy link
Copy Markdown
Collaborator

thank you @Poseidonas - changes LGTM. I am running our CI even though I understand that your changes will not be exercised. that's more of a checklist type of things than anything else.

@chombourger
chombourger changed the base branch from main to next August 24, 2026 13:41
@chombourger chombourger added check and removed check labels Aug 24, 2026
@chombourger

Copy link
Copy Markdown
Collaborator

I also changed the target branch from main to next. we should probably update our contributing guide for this recent policy change

@chombourger
chombourger merged commit f770e5c into siemens:next Aug 24, 2026
8 checks passed
@Poseidonas Poseidonas moved this from Waiting on review to Done in Industrial open source Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: rollback reports an error even if the action is successful

2 participants