[in-manishkr]

Two commits on master.

Why

• FCPs stay reserved after a VM is deleted. SMTClient.delete_vm() reset the VM's FCP records only at the end, after namelist removal, vswitch revoke and network cleanup, and each of those re-raises. If any of them failed, the reset never ran and the FCPs stayed reserved for a VM that no longer exists. The free FCP pool shrinks until attach or boot-from-volume fails with "no available FCP".
• State and owner stay out of date until the next sync. reset_fcps_of_assigner() left state='active' and owner=<deleted user>, and undedicate left the old values.
• Sync failures were silent. FCPManager.sync_db() wrapped the z/VM sync in ignore_errors().
• "database is locked". sdkserver and zvmsdk-getpchid open the same SQLite file. With the driver's default 5s wait in rollback-journal mode, normal contention could fail a zvmsdk-getpchid run.
• One CHPID without a PCHID broke FCP handling on the whole host. Every z/VM FCP query ran two vmcp commands per FCP device. One CHPID whose PCHID could not be found failed the query for all FCP devices: the FCP table was no longer synced, and every new FCP allocation on the host failed. zvmsdk-getpchid still exited 0, so its caller took the stale usage as current.
• FCP devices stay reserved for z/VM users that no longer exist, for example after a boot-from-volume deploy failed before the user was created. Nothing released them.

What changes

Fix FCP release on VM delete and keep FCP state current

• smtclient.delete_vm: the FCPs are released right after delete_userid() succeeds. A failure is logged with a traceback naming the userid, and the delete goes on. The count is logged for every DB backend.
• reset_fcps_of_assigner: an FCP that is active for the deleted user becomes free/none. Other states and owners are left to the next sync. It returns the number of rows reset.
• bulk_update_state_in_fcp_table: takes an optional new_owner. _undedicate_fcp writes free/none only after a successful undedicate, not when the FCP was already undedicated. The write is best-effort, so a DB error can't fail a z/VM operation that already succeeded.
• FCPManager.sync_db: logs a failure with a traceback. It still doesn't raise.
• SQLite:
  • PRAGMA journal_mode=WAL; it logs a warning if WAL can't be set, for example on read-only access.
  • New [database] connection_timeout option (default 30s).
  • The md5-to-sha512 migration backs up with the SQLite backup API, so rows still in the WAL are included, and checks the copy with PRAGMA quick_check.
• db_interface: contract docstrings updated; the contract tests cover the new behaviour for every backend.
• test_zvmsdk-getpchid: mocks fcp_ops.get_pchid_usage, which the script reads since database: add a pluggable DB backend and make FCP DB work whole operations #965.

Look up FCP PCHIDs once per CHPID, report failed syncs, free leaked FCPs

• Each z/VM FCP query checks CHPID virtualization once and looks up the PCHID once per CHPID.
• An FCP device whose PCHID isn't found keeps the PCHID already in the DB, or gets an empty one if it's new. FCP devices on other CHPIDs are not affected.
• FCPManager.synced_with_zvm records whether the last sync worked. zvmsdk-getpchid exits 1 with an error when it didn't, instead of printing the stale usage.
• zvmsdk-getpchid --list-held (read-only) lists the FCP devices that are reserved, connected or allocated from a template, per user. For each user it shows whether the user exists on z/VM (exists / missing / unknown; only z/VM's explicit "Invalid option - " reply counts as missing) and whether it is a guest of the host. A known guest isn't queried on z/VM.
• zvmsdk-getpchid --release USERID[,USERID...] frees the FCP devices of the given users, only of a user that is missing on z/VM and not a guest of the host. It checks again before releasing.

[Rajat-0]

Review of #974 (head d5a7431)

Review body

Review of the new commit only ("Fix FCP release on VM delete and keep FCP state current"); the other commits are from #965. Inline comments are below; two findings sit on lines outside this diff, so they are here:

•	zvmsdk/database_migration_md5_to_sha512.py:102: the backup misses WAL contents. With WAL now on for every SDK connection, backup_database() copies only the main .sqlite file with shutil.copy2. Writes that are committed but not yet checkpointed sit in -wal and are not in the backup, and the size check still passes. Restoring that backup after a failed migration silently loses those rows. Run a checkpoint before the copy (PRAGMA wal_checkpoint(TRUNCATE)), or use sqlite3.Connection.backup().
•	zvmsdk/volumeop.py:1883: boot-from-volume never gets active/<userid>. The new dedicate-time state/owner write is only in _dedicate_fcp, but attach() returns early for a root volume because refresh_bootmap already did the dedicate. Those FCPs keep the state/owner from the last sync (usually free/none) while connections=1. The write belongs in the shared dedicate path, not in one caller.

Generated by Claude Code

Inline comments

zvmsdk/database.py:163 — PRAGMA synchronous=NORMAL

In WAL mode, synchronous=NORMAL lets a committed transaction roll back after a power loss or OS crash. Scenario: reserve_fcps or increase_connections commits, z/VM dedicates the FCP, then the LPAR loses power before the WAL is fsynced. After restart the row shows reserved=0, connections=0, so the FCP can be allocated to a second VM while it is still dedicated in the directory. Since this DB tracks allocation of real devices, I'd keep synchronous=FULL.

zvmsdk/smtclient.py:2593 — with zvmutils.ignore_errors():

ignore_errors() logs only str(err), with no traceback, and delete_vm still reports success. A "database is locked" or other DB error here leaves the deleted VM's FCPs reserved, the leak this PR sets out to fix, with only one "ignore an error" line in the log. sync_db was changed to LOG.exception in this same PR; please do the same here (catch, LOG.exception naming the userid, continue).

zvmsdk/database.py:516 — state='free', owner='none'

This sets state='free' on every FCP of the assigner, including ones the sync marked offline or notfound. After the VM is deleted, a missing device then matches the allocator's state='free' AND connections=0 AND reserved=0 filter, and usage reports show it as free until a sync deletes it. Suggest resetting only the assigner fields, or changing state only where it is 'active'.

zvmsdk/volumeop.py:2122 — bulk_update_state_in_fcp_table([fcp], 'free', 'none')

This path also runs when z/VM returned 404/204 rs=8 ("device not defined"). That means the FCP is not dedicated to this user, not that it is free: it may be offline/notfound, or now dedicated to another user. Writing free/none here can make an offline FCP allocatable, or clear another user's owner, until the next sync. Suggest skipping the write in the rs=8 case (or writing only owner if it still equals this userid).

zvmsdk/volumeop.py:1729 — bulk_update_state_in_fcp_table( (dedicate)

z/VM reports a dedicated FCP of a logged-off guest as free/NONE. When volumes are attached during provisioning, before power-on, this writes active/, the next sync_with_zvm rewrites it to free/none, and the next dedicate flips it back. Anything that reads state/owner sees values that depend on sync timing. Either align the write with what the sync will compute, or have the sync keep owner for rows with an assigner_id.

zvmsdk/database.py:424 — UPDATE fcp SET reserved=0, assigner_id=''

Clearing assigner_id on unreserve means later cleanup keyed on it can no longer find rows whose state/owner are stale. Example: the undedicate on detach fails and its best-effort DB write is skipped, so state/owner stay active/USER; release then clears assigner_id; when USER is deleted, reset_fcps_of_assigner(USER) no longer matches that row. It also loses the last assigner for diagnostics. If clearing is kept, consider also resetting state/owner in the same statement.

zvmsdk/database.py:161 — PRAGMA journal_mode=WAL

This runs on every new connection, outside the get_*_conn try blocks. A user with read-only access (for example running zvmsdk-getpchid) or a read-only mount now fails on open with "attempt to write a readonly database", as a raw sqlite3.OperationalError rather than an SDK exception. Suggest setting WAL once at DB creation (it persists in the file) and wrapping the pragma in the existing error handling.

zvmsdk/config.py:539 — Opt('connection_timeout',

The new [database] connection_timeout option isn't in doc/source/zvmsdk.conf.sample; this PR adds backend there but not this one. Operators tuning lock waits won't find it.

[in-manishkr]

Thanks for the detailed review. All points are addressed in the first commit. I also rebased on master, so #965's commits no longer show up here.

• Migration backup and WAL: backup_database() now uses sqlite3.Connection.backup(), so committed rows still sitting in -wal are included. The size comparison doesn't hold for that kind of copy, so the backup is checked with PRAGMA quick_check instead.
• BFV / dedicate-time write: dropped the state/owner write at dedicate. As you said, z/VM reports a dedicated FCP of a logged-off guest as free/none, so any value written there just fights the sync. The allocator already skips these FCPs because of connections/reserved, so root volumes are covered the same way.
• synchronous=NORMAL: removed, back to the default FULL.
• delete_vm: the FCP release now catches the error, logs it with LOG.exception naming the userid, and lets the delete finish.
• reset_fcps_of_assigner: only an FCP that is active for the deleted user is set to free/none. none is lower case, the same format the sync stores. Offline, notfound, or active for another user are left to the next sync.
• Undedicate rs=8: free/none is written only after a successful undedicate. The "already undedicated" case doesn't touch state/owner.
• unreserve clearing assigner_id: reverted. It was only needed for the automatic reclaim, which isn't part of this PR anymore. Your detach-failure example is a good reason to keep the assigner.
• journal_mode=WAL on read-only access: wrapped. On OperationalError it logs a warning and uses the connection as is.
• connection_timeout: regenerated doc/source/zvmsdk.conf.sample.
• Second commit: while testing I found that the z/VM FCP query looked up the PCHID twice per FCP device. One CHPID without a PCHID also failed the sync for the whole host, and zvmsdk-getpchid still exited 0. It now looks up each CHPID once and keeps the stored PCHID if a lookup fails. zvmsdk-getpchid exits 1 when the sync fails.

I re-tested the changed paths on a z/VM host: VM delete with and without a DB error, dedicate/undedicate, reset with offline and other-owner rows, migration backup with rows still in the WAL, and read-only open. I also re-ran zvmsdk-getpchid and the IIM sync/detect commands on top of it.

[Rajat-0]

New findings (second commit)

1. --release doesn't uppercase the userid before the DB reset (bug). In scripts/zvmsdk-getpchid:267, the guest check and the vmcp query both use userid.upper(), but db.reset_fcps_of_assigner(userid) gets the userid exactly as typed. assigner_id is stored in uppercase (connection_info['assigner_id'].upper()), and the reset matches it with a case-sensitive WHERE assigner_id=?. So --release abcd0001 passes both checks and then prints "released": {"abcd0001": 0}. Nothing is freed and no error is shown. Fix: uppercase each userid once when parsing args.release, and add a test with lowercase input.

2. --release can free the FCPs of a boot-from-volume deploy that is still running. During a BFV deploy, the FCPs are reserved under the new userid before the z/VM user and the guest DB row exist. In that window the user is "missing on z/VM" and "not a guest of the host", which is exactly what --release accepts. Releasing then lets those FCPs go to a second VM while the first deploy is about to dedicate them. The tool is manual and the risk is small, but the --release help text and docstring should say to run it only when no deploy is running for that userid. You could also mention the timing in the --list-held output.

3. WAL can stop read-only users from reading at all (low severity). WAL mode is stored in the database file. When no other connection is open, the -wal and -shm files don't exist, and a process that can't write the database directory can't create them. Opening the database then fails with "unable to open database file". The new try/except only catches the pragma itself, not the queries after it. Before this change, a read-only process could still read in rollback-journal mode. Worth asking which user runs zvmsdk-getpchid in practice. If it's root or the zvmsdk user, this doesn't matter.

4. Minor:

   • A new FCP whose PCHID lookup fails is stored with pchid='' (volumeop.py:810). If it's in a template, get_pchids_from_all_fcp_templates will probably return '', and zvmsdk-getpchid would print a {"pchid": "", ...} entry. Consider filtering it out or flagging it.
   • In _cached_pchid_lookup, the CHPID virtualization check sits outside the try, so if vmcp q chpidv fails, the whole query still fails. That's the same as before and arguably correct for a host-wide problem; the comment could just say so.
   • bulk_update_state_in_fcp_table gained an argument in the abstract base class. An out-of-tree backend that hasn't added new_owner will raise TypeError on every undedicate. The new try/except catches and logs it, but it's worth a line in the release notes for anyone with their own backend.

[in-manishkr]

Thanks Rajat, updated (still the same two commits, now rebased on master):

1. I tried this on a node first. --release abcd0001 does free the FCPs there: fcp.assigner_id is declared COLLATE NOCASE in sqlite, so WHERE assigner_id=? matches regardless of case, and MariaDB (the remote backend we use) compares case-insensitively too. The only visible effect was the userid in the result showing as typed. I still upper-case each userid once in release_fcp_devices() so the output shows the stored userid and the release doesn't depend on the column collation. I also strip spaces around the commas. The test passes lower-case userids now.
2. Agreed. The --release help and docstring now say to run it only when no deploy is running for those userids, since a BFV deploy reserves its FCPs before the z/VM user exists. The --list-held help also says an assigner whose deploy is still running can show as missing.
3. In practice it's root: IIM runs /usr/bin/zvmsdk-getpchid over SSH as root, so I left this one as is.
4. • get_fcp_devices_per_pchid() now leaves the '' PCHID out and logs a warning with the in-use FCP devices that have no PCHID. Test added.
   • The comment in _cached_pchid_lookup now says a failed virtualization check fails the whole query on purpose.
   • Added a note on the new new_owner argument to doc/source/dbbackend.rst, in the Operators part. change.rst stops at 1.4.0, so the backend guide looked like the place someone writing a backend would read.