Set Meta.base_manager_name on 'storageadmin.Disk' #2666

[FroggyFlox]

Fixes #2666
@phillxnet, @Hooverdan96: it is ready for review but I'm leaving the PR as draft because I'm still not 100% I tested correctly; see below for more details.

This pull request simply applies the changes requested by the warning detailed in #2666.

Functional testing

@phillxnet, this area is your expertise, I believe, so the following testing may not be the most relevant/best.
If I'm correct, this model manager is used to list a disk as detached. I thus tested that as follows:

1. Attach a new disk to the VM
2. Create a pool with this disk
3. Turn off the VM and remove the disk
4. Turn the VM back ON and navigate to the "Disks" page: the disk that was removed in step 3 does show as "-detached".
5. Pressing the "Rescan" button does not throw any error and does not trigger the deprecation warning described in (t) use_for_related_fields is deprecated in Django 2.0 #2666 anymore.

Unit testing

buildvm155:/opt/rockstor # cd src/rockstor/ && poetry run django-admin test ; cd -
Creating test database for alias 'default'...
Creating test database for alias 'smart_manager'...
System check identified no issues (0 silenced).
......................................................................................................................................................................................................................................................................
----------------------------------------------------------------------
Ran 262 tests in 16.888s

OK
Destroying test database for alias 'default'...
Destroying test database for alias 'smart_manager'...

[phillxnet]

@FroggyFlox Thanks for looking into this one.

If I'm correct, this model manager is used to list a disk as detached.

From the comments (mine from some time ago now) it's more to be able to easily list attached disks only. I.e.:

class AttachedManager(models.Manager):
    """Manager subclass to return only attached disks"""
...
    def attached(self):
        # Return default queryset after excluding name="detached-*" items.
        # Alternative lookup type __regex=r'^detached-'

At the time we had very little capability re disk tracking (state etc): so I added this as a custom manager so we could ask for only attached disks easily. And it would, in turn, auto filter out all disks with our 'tell' (added a little while before hand) of detached status: that of a rename to "detached-<random_uuid_here>". So it was simply a convenience call to be used in for example:

rockstor-core/src/rockstor/storageadmin/views/command.py

Lines 86 to 87 in b4128ad

	# Log if no attached members are found, ie all devs are detached.
	if p.disk_set.attached().count() == 0:

In comparison to the long-winded, non-centralised way of doing the same: outside the model as it were, we have a remaining legacy way of doing the same that still existing here:

rockstor-core/src/rockstor/storageadmin/views/pool.py

Lines 529 to 533 in b4128ad

	if (
	pool.disk_set.filter(name__startswith="detached-").count()
	== pool.disk_set.count()
	):
	all_members_detached = True

More uses of the manager are:

rockstor-core/src/rockstor/storageadmin/views/command.py

Lines 106 to 107 in b4128ad

	# Get and save what info we can prior to mount.
	first_dev = p.disk_set.attached().first()

Similarly we can likewise reference starting from the share thus:

rockstor-core/src/rockstor/storageadmin/views/command.py

Lines 163 to 164 in b4128ad

	if share.pool.disk_set.attached().count() == 0:
	continue

So, in summary, the manager allows for an easy way to return attached disks, and it 'keys' from the name we give disks when they are detached: which is "detached-random_here".

• Our device name gives us it's detached status.
• The custom manager (named attached) builds on this to provide a baked-in query of the remaining disks (of a pool).

Not sure is that helps, but if this attached custom manager (query) method was dis-functional we could still tell if a disk was detached due to it's name. But we could no longer effectively use the Pool's disk_set (a special Django build-in relational filter magic) for Disks belonging to that Pool) via the custom manager here and get a list (from the pool's perspective) of attached disks that belong to it (the Pool).

I have not yet looked more closely at if your test method shows this: but just wanted to point out that this manager is used from the perspective of the Pool, via the built-in disk_set qualifier. I think this is why I used "use_for_related_fields".

A doc entry from the (Django) time this was added:
https://docs.djangoproject.com/en/1.8/topics/db/managers/#using-managers-for-related-object-access
https://docs.djangoproject.com/en/1.8/topics/db/managers/#controlling-automatic-manager-types

Correctly or not as it goes. However to date this approach seems to have worked as expected. And from a quick look the modification proposed here may pertain to the entire model, yet we have the following in-play also:

rockstor-core/src/rockstor/storageadmin/models/disk.py

Lines 86 to 88 in b4128ad

	# The default manager is the first defined.
	attached = AttachedManager() # Only return attached Disk objects.
	objects = models.Manager() # Ensure Object manager is still accessible.

So I'm not entirely sure if your proposal is transitioning (to newer Django) a prior definition that was not actually required, or out of place but without subsequent issue (until not :) ) or we can instead just drop the "use_for_related_fields" definition. After all we are only after a custom manager that can be used from the Pool's perspective. And we have maintainer access to the default manager via the Objects qualifier.

A little reading does suggest we did need this "use_for_related_fields" when it was the default manager, which it looks to be for us (first listed in last code snippet) but that we also enable plain Objects access.

If we can prove that our pool.disk_set.attached() still works, and that we can also retrieve all disks via Objects then I think we are good here. My apologies for not focusing on the test required, but I also wanted to raise questions on if I had done the correct thing initially here: but without known consequence until now.

[phillxnet]

If we can prove that our pool.disk_set.attached() still works,

Build a 5.0.3-2667 rpm:

• Created a raid1 with 3 disks.
• Shutdown and removed all disks (expected 3 detached disks)
• Power-on and we have the expected following error:

[15/Sep/2023 11:19:32] INFO [scripts.initrock:459] ### DONE establishing poetry path to binaries in local files.
[15/Sep/2023 11:19:38] ERROR [storageadmin.views.command:90] Skipping Pool (rock-pool) mount as there are no attached devices. Moving on.
[15/Sep/2023 11:19:39] ERROR [storageadmin.views.command:90] Skipping Pool (rock-pool) mount as there are no attached devices. Moving on.
[15/Sep/2023 11:19:39] INFO [fs.btrfs:771] Mount by label (/dev/disk/by-label/rock-pool) failed.
[15/Sep/2023 11:19:39] ERROR [storageadmin.middleware:34] Exception occurred while processing a request. Path: /api/commands/refresh-share-state method: POST
[15/Sep/2023 11:19:39] ERROR [storageadmin.middleware:35] Failed to mount Pool(rock-pool) due to an unknown reason. Command used ['/usr/bin/mount', '/dev/disk
/by-label/rock-pool', '/mnt2/rock-pool']

Where the first two ERRORS are our initial pool mount attempt.
Later we see less elegant/informed subsequent share mount failures: but we have our proof that our explicit call to our Pool.disk_set.attached() manager functions as expected.
Given the pool in question has not been deleted, as per the first part of below code, we can also make other assumptions:

It is here assumed that the object filter is returning the detached disks as expected.
I.e.

• Pool.disk_set.attached.count == 0 (expected ERROR)
• Pool.disk_set.count != 0 (as pool still exists)

Re:

rockstor-core/src/rockstor/storageadmin/views/command.py

Lines 80 to 92 in b4128ad

	for p in Pool.objects.all():
	# If our pool has no disks, detached included, then delete it.
	# We leave pools with all detached members in place intentionally.
	if p.disk_set.count() == 0:
	p.delete()
	continue
	# Log if no attached members are found, ie all devs are detached.
	if p.disk_set.attached().count() == 0:
	logger.error(
	"Skipping Pool ({}) mount as there "
	"are no attached devices. Moving on.".format(p.name)
	)
	continue

[phillxnet]

@FroggyFlox I'm still a little unsure here if we've done the right thing, but I've now duplicated your existing test, as per my last comment, and I think we are good here.

Thanks for seeing to this, much appreciated. And for clearing the way further on the associated Milestone.

We are still fairly early in our current testing and it's important that we get to the associated milestone so I'll merge this so we can move on. Functionally it looks to be a continuation, given the indicated code on boot-up.