-
Notifications
You must be signed in to change notification settings - Fork 138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NFS exports restored as read-only #2912
Comments
With what is essentially a default NFS export created showing as:
we have the following config-save data based on the DB From the first (in our config_backup file format) DB dump of storageadmin, our default DB, we have the following relevant entries: {"model": "storageadmin.nfsexport", "pk": 1, "fields": {"export_group": 1, "share": 1, "mount": "/export/NFS-share"}},
{"model": "storageadmin.nfsexportgroup", "pk": 1, "fields": {"host_str": "*", "editable": "rw", "syncable": "async", "mount_security": "insecure", "nohide": false, "enabled": true, "admin_host": null}},
{"model": "storageadmin.share", "pk": 1, "fields": {"pool": 1, "qgroup": "0/445", "pqgroup": "2015/3", "name": "NFS-share", "uuid": null, "size": 10485760, "owner": "root", "group": "root", "perms": "755", "toc": "2024-11-01T12:09:41.121Z", "subvol_name": "NFS-share", "replica": false, "compression_algo": null, "rusage": 16, "eusage": 16, "pqgroup_rusage": 16, "pqgroup_eusage": 16} Where the top level model "nfsexport" references, in-turn:
|
With some additional debug logging we have the following config-save file replay on the original system:
I.e. as expected we have no action and our exports file is unchanged:
|
NFS export restoreOn the original system the single NFS export was deleted (Web-UI) and the NFS service left enabled. The OS was confirmed to have been updated via:
returning no entries. The same config-save as per the last comment was replayed again, with the same additional debug log entries in-play:
With our issue subject 'flip' to a non-default read-only status for the restored NFS export:
And as expected we have the expected underlying OS config as per our Web-UI report:
|
It is currently assumed that we are failing re matching the raw DB value for 'editable' in the DB dump based config-backup file contents with what is accepted as input for NFS export creation via the API /api/nfs-exports. E.g.: From the above and our nfs_export tests: there are disparities between our re-play (from config-backup file) |
Next focus is our API parsing to DB model translation: src/rockstor/storageadmin/views/nfs_exports.py rockstor-core/src/rockstor/storageadmin/views/nfs_exports.py Lines 105 to 116 in e88854b
With our model defined here: with the referenced validators:
defined here: https://github.com/rockstor/rockstor-core/blob/testing/src/rockstor/storageadmin/models/validators.py |
# Includes - Instructions on fixture creation. - Instructions on running nfs export tests. - Modernise test_nfs_export.py by re-instating test_nfs.json fixture and removing prior hard-coded object mock stand-ins for DB models during these test runs.
The associated tests for NFS export are in need of update/maintenance, containing many TODOs. I am including at least part of this maintenance against this issue as it should help to clarify expected function of our existing API - used in the NFS restore process. Which is where the fault lies with this issue: i.e. we restore via DB naming, where-as our current NFS export API has some differences re field names, and then some more re Web-UI naming. Improving our tests in this area can only help with current (this issue) and future development re NFS capability. |
- Add default ordering to NFSExportGroup models. Addresses re-enabled test provoked warnings re: "UnorderedObjectListWarning: Pagination may yield inconsistent results with an unordered object_list"
Resolve remaining TODOs in test_nfs_export.py. Includes: - Removing redundant tests. - Modifying previously remarked out sanity check tests to instead cover our umbrella low-level exception. Once more specific sanity checks are in place, we can add tests to prove their function.
- Black reformatting test_nfs_export.py.
- Enable previously dormant field validation via addition of: NFSExportGroup.clean_fields() prior to model.save(). Affects: host_str, modify_str, & sync_choice during post & put. - Additional tests to prove field validation function within current validation capability (need refinement).
- Additional test_post() with API / DB / Web-UI mapping info.
- nfs_export_group.py comment correction & black formatting.
- Additional NFS debug logging & comments: config_backup.py. - Some string.format to fstring updates.
Linking back to spin-off issue that is now deemed a dependency of this issue:
|
DB to API translationAs we simply dump our DB contents, and our API and DB have differing fields for our current NFS export create, the problem here is that we currently fail to translate NFS export DB field names back to API filed names. I.e. from our prior example of a reproducer NFS export restore from saved config: with rw:
it is evident that we are currently generating a restore payload with 'editable': 'rw', however there is no
Given the above, we either:
It is proposed that option 2. is strongly preferred here as:
As such I will pursue the minimal fix of option (2.) give we are late RC, with comments to indicate the nature of this change. Future testing channel phases could consider modifying our front-end to make this translation redundant. There-after, if we maintain the original |
Fix translation of DB dump derived config-save field names to NFS export create API field names. Affects Web-UI surfaced NFS export config of: - Access type ('Writable'/'Read only') - Response type ('async'/'sync') for NFS exports created via a config restore process. Note that we maintain DB dump derived config-save field names in the generated API calls (ignored currently), to enable future API field name alignment to DB model field names. At which point the additional fields added here to the API payload would themselves become redundant and up for deprecation, and in the change-over period ignored.
…read-only NFS exports restored as read-only #2912
Closing as: |
Thanks to user simon-77 on the community forum who observed that their nfs exports were restored, however without the read-write flag set (moving from 4.6.1-0 and restoring onto a 5.0.14-0 environment). They are recreated as read-only (which might be the nfs default).
https://forum.rockstor.com/t/missing-repository-rockstor-stable-on-15-6/9643/18
The problem might occur in this area (but might also be before when preparing the restore data elsewhere):
rockstor-core/src/rockstor/storageadmin/views/config_backup.py
Lines 121 to 135 in 1ddcf4b
Here's an example of the nfs json entry taken from here in the discussion thread:
where one can see that
rw
for the read-write setting is available.The text was updated successfully, but these errors were encountered: