Adjust/lustre quota from cronjob to grid daemon #393

Martin-Rehr · 2025-11-25T14:19:19Z

Changed the lustre quota system from using a cronjob to use the grid daemon setup and thereby align with our existing grid services. This provide a tighter integration with the existing codebase as well as the possibility to extend with other storage quota systems.

jonasbardino · 2025-11-27T10:41:34Z

mig/install/migrid-init.d-rh-template

    export PYTHONPATH=${MIG_PATH}:$PYTHONPATH
 fi
+# Make sure '/usr/local/(s)bin' is in path and force lookup order
+export PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"


Hmmm, I'm not sure we really want to replace or even tweak PATH in general here ... it could have ugly side effects.
If at all perhaps only add the missing /usr/local/(s)bin paths to any existing PATH and preferably only in the start_quota function if it's only needed there?
The same applies for the deb version of the script.

The advantage of setting the PATH in general is that we control the order.

If we don't wan't to set the full PATH I think we should add /usr/local/(s)bin to the existing PATH in the general case.

jonasbardino · 2025-11-27T10:52:59Z

mig/lib/lustrequota.py

-from mig.shared.logger import daemon_logger
-
-from pylustrequota.lfs import lfs_set_project_id, lfs_get_project_quota, \
+from lustreclient.lfs import lfs_set_project_id, lfs_get_project_quota, \


I think this is the source of some of the current CI errors. Should it perhaps be conditional like other imports relying on optional extras and only fail if actually ever used?
Something like:

try: import XYX as X except ImportError: X = None

then inside function(s) have an

if X is None: print("You need module X installed for XYZ") exit(1)

lustreclient.lfs is not optional for mig/lib/lustrequota.py

But we can make a:

try: import XYX as X except ImportError: sys.exit(1)

To satisfy the linter

Well, mostly a theoretical point as long as we only really support lustre as backend of grid_quota, but if we intend it to act as a generic quota handler, we should avoid it crashing from missing lustre client as soon as it tries to import lib/quota.py - which in turn unconditionally imports from lib/lustrequota.py.
So I guess the import would better be conditional either in quota.py or not fail until used in lustrequota.py to get around that, which would hopefully not just satisfy the linter but also solve practical use e.g. for any other future backends.
While at it perhaps it's worth considering an else clause to fail with a log error if update_quota is called without a valid backend?

jonasbardino · 2025-11-27T10:58:00Z

mig/lib/lustrequota.py

                    # Only take dirs into account
-                    msg = "Skiping non-dir path: %r" % entry.path
-                    DEBUG(configuration, msg, verbose)
+                    logger.debug("Skiping non-dir path: %r" % entry.path)


Nitpick: typo in 'Skipping'

jonasbardino · 2025-11-27T11:00:12Z

mig/src/lustreclient/lustreclient/__init__.py

 # --- BEGIN_HEADER ---
 #
-# __init__ - luste quota python extensions
+# __init__ - luste client python extension


Nitpick: 'lustre' typo

jonasbardino · 2025-11-27T11:00:22Z

mig/src/lustreclient/lustreclient/__init__.py

 # -- END_HEADER ---
 #
-"""This package provide luste quota functionality"""
+"""This package provide luste client functionality"""


Nitpick: 'lustre' typo

jonasbardino · 2025-11-27T11:03:17Z

sbin/grid_quota.py

+#
+# --- BEGIN_HEADER ---
+#
+# grid_quota - daemon to create storage quota


minor suggestion: 'daemon to manage storage quotas' or similar?

jonasbardino · 2025-11-27T11:05:08Z

sbin/grid_quota.py

+
+if __name__ == "__main__":
+    print(
+        """This is the MiG lustre quota daemon which collect storage quota


Not sure if you want to drop 'lustre' here with the generalization? and nitpicking on it should be 'collects'.

jonasbardino

Generally looks alright to me.

In line with previous internal discussions in the team I have a few additional remarks in regards to merging especially new code like this:

Github check failures should be addressed before we merge
all new code should have unit tests with decent test coverage to the extent possible.
all new code should be 'style clean'

I commented on the CI error of (1) inline and perhaps the C-extension part needs some lustre header include adjustments to succeed.

We decided to be particularly strict about (2+3) for code in mig/lib/ and sbin/ as outlined on Milestone 7 and Milestone 11 .

Further hints and pointers are summarized on Project Contribution

Martin-Rehr · 2025-11-27T13:03:27Z

We don't really wan't to include the lustre headers in the migrid codebase since the header version is bound to the version of lustre installed on the system where migrid is deployed.
Unit tests are not really applicable since they would require a working lustre storage system

jonasbardino · 2025-11-27T17:39:52Z

We don't really wan't to include the lustre headers in the migrid codebase since the header version is bound to the version of lustre installed on the system where migrid is deployed.

Unit tests are not really applicable since they would require a working lustre storage system

I wonder if we could pull in the headers from the lustre rpm package in the github action like we install other packages, but if not we should probably exclude the lustre client from the c-extension action completely, even if not so nice.
the generic lib/quota.py is small and simple so far but could come with a basic unit test of update_quota already, and be ready for any future extensions. In fact the invalid backend value mentioned in a recent comment could be one thing to unit test there.

Martin-Rehr · 2025-11-28T10:02:27Z

Thanks for the input, I'll try to pull in the headers for 'the github action' and make a unit test for the lib/quota.py

jonasbardino · 2025-12-01T10:20:24Z

Thanks for the input, I'll try to pull in the headers for 'the github action' and make a unit test for the lib/quota.py

You're welcome, and sounds good 👍
The c-extension checks are defined in .github/workflows/python-c-ext-sanity-check.yml and that is also where we'd want to extend paths-ignore with whatever pattern matches the lustre client source if we can't get the checks running. So feel free to include changes there as part of the PR or separately.

…ds on the 'lustre' source code which we do not wan't to include in our code path.

…sfy pylint

Martin-Rehr · 2025-12-02T13:46:38Z

Thanks for the input, I'll try to pull in the headers for 'the github action' and make a unit test for the lib/quota.py

You're welcome, and sounds good 👍 The c-extension checks are defined in .github/workflows/python-c-ext-sanity-check.yml and that is also where we'd want to extend paths-ignore with whatever pattern matches the lustre client source if we can't get the checks running. So feel free to include changes there as part of the PR or separately.

The paths-ignore isn't enough, as far as I can tell these are trigger ignores, if we wan't to ignore specific C files then we need to filter them out in the run splint like we already filter out non-C files:
git diff --diff-filter=ACMRTB --name-only HEAD^1 -- | grep -E '\.c$' | xargs -r splint ...

Martin-Rehr added 4 commits November 25, 2025 13:08

Changed pylustrequota to a generic python lustreclient extension

af7021c

Changed lustre quota from cron job to grid daemon

437a6a7

Updated unit tests to match new grid quota daemon setup

fabce46

Added 'quota_update_interval' option to generateconfs

b6c56f1

Martin-Rehr mentioned this pull request Nov 25, 2025

Adjust/lustre quota from cronjob to grid daemon ucphhpc/docker-migrid#137

Open

Martin-Rehr added 3 commits November 25, 2025 17:56

Addwed missing 'user_quota_log' entry to configuration

5261194

Added missing 'update_interval' to MiGserver-template.conf

375f634

Added 'update_interval' to MiGserver.conf fixture

9c9e976

Martin-Rehr requested a review from jonasbardino November 27, 2025 10:32

jonasbardino reviewed Nov 27, 2025

View reviewed changes

Minor comment corrections thanks to @jonasbardino

aa83180

Martin-Rehr added 5 commits December 1, 2025 13:34

Added 'lustreclient' to 'c-ext-sanity-check' ignore paths as it depen…

1132a2d

…ds on the 'lustre' source code which we do not wan't to include in our code path.

Added check for 'lustreclient' module import

be7f1bc

Added log error message if requested 'quota' backend is unsupported

d5a9d3c

Removed trailing whitespace

3933cd8

Restrict 'lustreclient.lfs' import exception to 'ImportError' to sati…

8312ebd

…sfy pylint

Martin-Rehr added 2 commits December 2, 2025 14:51

Removed absolete 'import sys'

9dba870

lustrequota: Added check for 'psutil' module

559f60c

Added quota unittest

17f4194

Adjust/lustre quota from cronjob to grid daemon #393

Are you sure you want to change the base?

Adjust/lustre quota from cronjob to grid daemon #393

Uh oh!

Conversation

Martin-Rehr commented Nov 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Martin-Rehr Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasbardino left a comment

Choose a reason for hiding this comment

Uh oh!

Martin-Rehr commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasbardino commented Nov 27, 2025

Uh oh!

Martin-Rehr commented Nov 28, 2025

Uh oh!

jonasbardino commented Dec 1, 2025

Uh oh!

Martin-Rehr commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Martin-Rehr Nov 27, 2025 •

edited

Loading

Martin-Rehr commented Nov 27, 2025 •

edited

Loading