Sysdeploy/idlers-agent
2
0
Fork 2
Code Issues 5 Pull Requests Packages Projects Releases Wiki Activity

disk space calculation using /sys/block/ #15

Merged
PeterSurda merged 1 commits from shailaja/idlers-agent:disk/space into main 2024-06-27 10:48:12 +02:00
Conversation 1 Commits 1 Files Changed 1 +23 -11
shailaja commented 2024-06-24 17:20:42 +02:00
Member
Copy Link
No description provided.
shailaja force-pushed disk/space from c0d919ce2c to 217e433f76 2024-06-24 17:25:32 +02:00 Compare
PeterSurda requested changes 2024-06-25 00:52:01 +02:00
agent.py Outdated
@ -37,0 +41,4 @@
size = int(f.read().strip())
disk += size
except Exception as e:
logging.error(f"Failed to read disk size for {device}: {e}")
PeterSurda commented 2024-06-25 00:51:55 +02:00
Owner
Copy Link

We shouldn't log it, just skip it, or we should first test if the file exists or is readable to filter the obvious exceptions.

We shouldn't log it, just skip it, or we should first test if the file exists or is readable to filter the obvious exceptions.
PeterSurda marked this conversation as resolved
shailaja force-pushed disk/space from 1e586266f7 to 5b61f38715 2024-06-25 13:17:42 +02:00 Compare
shailaja requested review from swapnil 2024-06-25 13:18:12 +02:00
shailaja requested review from PeterSurda 2024-06-25 13:18:33 +02:00
swapnil reviewed 2024-06-25 13:52:43 +02:00
agent.py Outdated
@ -36,3 +30,1 @@
disk = sum(int(x.split()[9]) for x in diskstats.split('\n') if x) * 512 // 10**9
logging.info(f"RAM: {ram}MB, Disk: {disk}GB")
return ram, disk
def get_ram_and_disk(self):
swapnil commented 2024-06-25 13:52:42 +02:00
Member
Copy Link

Indentation error

Indentation error
PeterSurda marked this conversation as resolved
swapnil reviewed 2024-06-25 13:53:14 +02:00
agent.py Outdated
@ -39,0 +46,4 @@
pass # Skip the device if any exception occurs
disk = disk * 512 // (1024**3) # convert to GB
logging.info(f"RAM: {ram}MB, Disk: {disk}GB")
swapnil commented 2024-06-25 13:53:14 +02:00
Member
Copy Link

We shouldn't log it, just skip it, or we should first test if the file exists or is readable to filter the obvious exceptions.

> We shouldn't log it, just skip it, or we should first test if the file exists or is readable to filter the obvious exceptions.
PeterSurda commented 2024-06-26 01:16:12 +02:00
Owner
Copy Link

Well, this logging.info we could keep but I would prefer if we got rid of f-strings due to #9

Well, this `logging.info` we could keep but I would prefer if we got rid of f-strings due to #9
PeterSurda marked this conversation as resolved
shailaja force-pushed disk/space from aa577a0d02 to 1021fdb0e1 2024-06-25 14:16:41 +02:00 Compare
swapnil reviewed 2024-06-25 14:25:46 +02:00
agent.py Outdated
@ -39,0 +35,4 @@
# Disk space information
disk = 0
for device in os.listdir('/sys/block'):
swapnil commented 2024-06-25 14:25:46 +02:00
Member
Copy Link

Maybe we should fallback to /proc/diskstats if /sys/block is not available, @PeterSurda ?

Maybe we should fallback to `/proc/diskstats` if `/sys/block` is not available, @PeterSurda ?
PeterSurda commented 2024-06-26 01:14:08 +02:00
Owner
Copy Link

We shouldn't use /proc/diskstats at all because it doesn't contain the information we need. We could use /proc/partitions if /sys/block isn't available. But I don't think I have such a system. The wiki page for sysfs says it's available since kernel 2.5. I got rid of my last machine running 2.4 around 2014-2015.

We shouldn't use `/proc/diskstats` at all because it doesn't contain the information we need. We could use `/proc/partitions` if `/sys/block` isn't available. But I don't think I have such a system. The [wiki page for sysfs](https://en.wikipedia.org/wiki/Sysfs) says it's available since kernel 2.5. I got rid of my last machine running 2.4 around 2014-2015.
PeterSurda marked this conversation as resolved
shailaja force-pushed disk/space from a502498394 to fe6dbebf57 2024-06-25 14:35:27 +02:00 Compare
PeterSurda requested changes 2024-06-26 01:21:01 +02:00
agent.py Outdated
@ -37,0 +37,4 @@
disk = 0
for device in os.listdir('/sys/block'):
size_path = f'/sys/block/{device}/size'
if os.path.exists(size_path) and os.access(size_path, os.R_OK):
PeterSurda commented 2024-06-26 01:20:42 +02:00
Owner
Copy Link

We also need to check for /sys/block/*/device as I explained in the chat. The logic is supposed to be like this.

If /sys/block/{device}/device is a symlink, then
   open /sys/block/{device}/size
      and read from it

The code as it is now doesn't filter virtual devices.

Also please avoid f-strings due to #9.

We also need to check for `/sys/block/*/device` as I explained in the chat. The logic is supposed to be like this. ``` If /sys/block/{device}/device is a symlink, then open /sys/block/{device}/size and read from it ``` The code as it is now doesn't filter virtual devices. Also please avoid f-strings due to #9.
PeterSurda marked this conversation as resolved
PeterSurda commented 2024-06-26 01:23:49 +02:00
Owner
Copy Link

Also please watching out for needing to rebase PRs.

Also please watching out for needing to rebase PRs.
shailaja force-pushed disk/space from fe6dbebf57 to ddbf2143d0 2024-06-26 14:03:15 +02:00 Compare
shailaja force-pushed disk/space from e545bcefa4 to f051a6e29d 2024-06-27 08:50:14 +02:00 Compare
shailaja force-pushed disk/space from f051a6e29d to 54d0b0decd 2024-06-27 09:05:43 +02:00 Compare
PeterSurda approved these changes 2024-06-27 10:47:23 +02:00
PeterSurda left a comment
Owner
Copy Link

I'm merging, please address my comments in the next PR.

I'm merging, please address my comments in the next PR.
agent.py
@ -95,3 +95,4 @@
# RAM information
with open('/proc/meminfo', 'r') as f:
meminfo = f.read()
ram = int([x for x in meminfo.split('\n') if 'MemTotal' in x][0].split()[1]) // 1024
PeterSurda commented 2024-06-27 10:46:29 +02:00
Owner
Copy Link

This can be rounded up, because on systems which have 1GB of RAM, it will result in 0GB (because it subtracts the amount allocated for the kernel).

This can be rounded up, because on systems which have 1GB of RAM, it will result in 0GB (because it subtracts the amount allocated for the kernel).
agent.py
@ -102,0 +101,4 @@
for device in os.listdir('/sys/block'):
device_path = '/sys/block/{}/device'.format(device)
size_path = '/sys/block/{}/size'.format(device)
if os.path.islink(device_path):
PeterSurda commented 2024-06-27 10:44:37 +02:00
Owner
Copy Link

There is a trick to make the code more readable. Use a negative condition and continue. This will reduce indentation.

e.g.

if not os.path.islink(device_path):
    continue
try:
    with open(size_path, 'r') as f:
...
There is a trick to make the code more readable. Use a negative condition and `continue`. This will reduce indentation. e.g. ``` if not os.path.islink(device_path): continue try: with open(size_path, 'r') as f: ... ```
PeterSurda merged commit 54d0b0decd into main 2024-06-27 10:48:12 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
swapnil
PeterSurda
Labels
Clear labels   
bug

Something is not working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
invalid

Something is wrong

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Sysdeploy/idlers-agent#15
No description provided.
Delete Branch "shailaja/idlers-agent:disk/space"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?