Pool Creation Date incorrect?

GeoffA · December 26, 2020, 4:39pm

I noticed purely by chance that the creation date for my pools is incorrect: it always shows the current date and time, not when the pool was actually created. Here’s an example screenshot:

This pool was created back in October.

Not a biggie, just a strange one really. Any thoughts? This is from v4.0.4.

Geoff

Flox · December 26, 2020, 5:23pm

Nice find, @GeoffA!

@phillxnet is the expert for anything pool-related, but I was curious and looked at the related code.

What we display is defined here…

github.com

rockstor/rockstor-core/blob/26be2da95db78744b00ba4aad8d3b41f8e15fc19/src/rockstor/storageadmin/static/storageadmin/js/templates/pool/pool_info_module.jst#L25


* 
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
* 
*/
</script>


<!-- Content -->
<div class="module-content">
 <h3>Details</h3>
 Created on: <strong>{{getPoolCreationDate model.toc}}</strong><br/>
 Raid configuration: <strong>{{model.raid}}</strong><br/>
 Active mount options / Status: <strong>
 {{#if model.is_mounted}}
   {{model.mount_status}}
 {{else}}
   <span style="color:red">{{model.mount_status}}</span>
 {{/if}}
 </strong><br/>
 UUID: <strong>{{model.uuid}}</strong>
</div> <!-- module-content -->

… which returns the toc value from the corresponding Pool model:

github.com

rockstor/rockstor-core/blob/26be2da95db78744b00ba4aad8d3b41f8e15fc19/src/rockstor/storageadmin/models/pool.py#L42






class Pool(models.Model):
    # Name of the pool
    name = models.CharField(max_length=4096, unique=True)
    """uuid given automatically by the client"""
    uuid = models.CharField(max_length=100, null=True)
    """size of the pool in KB"""
    size = models.BigIntegerField(default=0)
    raid = models.CharField(max_length=10)
    toc = models.DateTimeField(auto_now=True)
    compression = models.CharField(max_length=256, null=True)
    mnt_options = models.CharField(max_length=4096, null=True)
    """optional aux info. eg: role = root for OS Pool"""
    role = models.CharField(max_length=256, null=True)


    def __init__(self, *args, **kwargs):
        super(Pool, self).__init__(*args, **kwargs)
        self.update_missing_dev()
        self.update_mnt_pt_var()
        self.update_device_stats()

Now, it seems that this field is determined by its auto_now attribute, and if we look at Django’s documentation on it, we can see:
https://docs.djangoproject.com/en/3.1/ref/models/fields/#django.db.models.DateField.auto_now

Automatically set the field to now every time the object is saved.

So, from reading this, this field should be updated with the current date every time the model is saved, which means we might inadvertently update the “Creation date” every time the model is saved. As stated above, @phillxnet is the expert there so he probably knows every time the Pool model is saved, but I don’t…

My inexperienced impression would be that we have at least two options there:

Update the webUI text to state that this date corresponds to the “Last modified” time, rather than creation time.
Update the Pool model to replace the auto_now attribute with the auto_now_add attribute, which is only filled the first time the corresponding Pool object is created.

Note that anything related to Pools/Shares management is always very critical, and even more so if one is talking about updating the corresponding Django model so we need to tread with extreme caution there. It does nonetheless looks like a very good candidate for a new issue on the Github repo, though.

phillxnet · December 27, 2020, 12:38pm

@GeoffA and @Flox
Yes, I’ve noticed this myself but haven’t as yet gotten around to looking any further.
I think the current state of reporting the ‘now’ time is just not useful but it would be useful to have the pool creation time, at least from our our db point of view. In which case I vote for:

Although as stated we could run into some unexpected behaviour here. I would say that the current auto_now setting was a stop gap that never got re-addressed until now. And has only just come up. But yet, pool creation date (from btrfs) or as a simpler implementation and to keep the number of btrfs calls down the import data would at least be more usefully than our current meaningless now attribute.

Agreed. @GeoffA if you fancy creating a issue for this, given you are I believe the first to have noticed and reported this, that would server as the all important attribution element and hopefully we can make this a more useful element in the pool info. Or we remove it entirely which would be better than our current near meaningless info.

As @Flox points out, we save the pool all the time, we updating it with current system state to reflect that within the Web-UI. But I don’t see this metric being of interest to the end user. I suppose we could keep it as some kind of internal check on if we have recently updated the pool info but this would seem a little redundant as if we fail to save we get a Django error anyway.

So I personally would like to see this as set once during pool import and to represent the actual pool creation date. Not our pool model creation date. As long as we only set this during import it shouldn’t represent an additional presser on our already fairly heavy btrfs call count. The simpler model creation time seems like we would be cheaping out a little. All thoughts welcome. But the text could help to clarify this, i.e. “Poll creation date on import” or something like that. As there are potential situations where a pool is re-created under Rockstor by the same name and this way we cheap out just a little less. I’m just concerned that we are making a massive number of btrfs calls and don’t want to add load for something that has come up exactly once.

But I wholely agree that we should get this sorted because it is, as-is, a little shabby and just plain wrong.

@Flox Thanks for looking into this, much appreciated. I had just assumed myself it was a now placeholder and not looked any further. But now it’s come up it would be a nice one of the thankfully dwindling paper cuts we still have within the Web-UI.

@GeoffA Thanks for yet another keen observation and do please consider creating a GitHub issue for this as attribution is super important and it may well be with one extra grep equivalent we can grab this during a pool import and avoid future updates by carefull wording within the Web-UI.

As always all thoughts welcome. But I favour representing the actual pool creation date as otherwise we don’t fully fix this and miss the chance to represent what at least back when was considered a nice to have. And creation date would be nice to surface.

So my vote is for actual pool creation with the caveat that I’d rather not add a refresh on every pool refresh which as it goes is fairly often.

GeoffA · December 27, 2020, 1:04pm

Done