Skip to content

Update MerlinAU.sh#33

Merged
ExtremeFiretop merged 1 commit intoExtremeFiretop:devfrom
Martinski4GitHub:dev
Dec 16, 2023
Merged

Update MerlinAU.sh#33
ExtremeFiretop merged 1 commit intoExtremeFiretop:devfrom
Martinski4GitHub:dev

Conversation

@Martinski4GitHub
Copy link
Collaborator

Some code improvements WRT getting the currently available RAM that may be "free" to be reclaimed at the time to be able to proceed with the F/W Update process.

Some code improvements WRT getting the currently available RAM that may be "free" to be reclaimed at the time to proceed with the F/W Update process.
@ExtremeFiretop ExtremeFiretop merged commit 6d41199 into ExtremeFiretop:dev Dec 16, 2023
@ExtremeFiretop
Copy link
Owner

All looks great to me!

I created a new PR for the discussed warning messages:

https://github.com/ExtremeFiretop/MerlinAutoUpdate-Router/pull/34/files

@ExtremeFiretop ExtremeFiretop added the question Further information is requested label Jan 26, 2024
@ExtremeFiretop
Copy link
Owner

@Martinski4GitHub

Why is it that since we stopped using get_free_ram() function that my memory in the script no longer follows the memory in the WebUI?

For example, at the backup stage my WebUI got to 55MB of free memory, but the script was still reporting some 330MB of free memory. When I got back to get_free_ram function, now all of a sudden the script follows the WebUI perfectly.

I understand your method is technically more comprehensive, but I feel like it would throw users off actually comparing and monitoring like I just did.

@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Jan 26, 2024

And I should mention that while the /proc/meminfo is technically more accurate, Is it worth the potential confusion?

@Martinski4GitHub
Copy link
Collaborator Author

@Martinski4GitHub

Why is it that since we stopped using get_free_ram() function that my memory in the script no longer follows the memory in the WebUI?

For example, at the backup stage my WebUI got to 55MB of free memory, but the script was still reporting some 330MB of free memory. When I got back to get_free_ram function, now all of a sudden the script follows the WebUI perfectly.

I understand your method is technically more comprehensive, but I feel like it would throw users off actually comparing and monitoring like I just did.

From the perspective of the kernel & the OS memory management, the definition of "free" memory is different from the definition of "available" memory. The webGUI shows users the amount of "free" RAM, but what we want to know in the script is the amount of "available" RAM because that's more accurate when deciding whether we have enough to proceed with flashing the F/W image.

Some users may be confused but that's likely because of a lack of knowledge & understanding of the difference between "free" & "available" RAM. Note that the message output by the script is correct & clear:

It says:
Required RAM: XXXX KB - Available RAM: YYYY KB"

It does not say:
Required RAM: XXXX KB - Free RAM: YYYY KB"

Here's a screenshot showing the current memory on my router. Notice where the output from "/proc/meminfo" clearly labels the amounts for "Free" vs. "Available" RAM. Those are the correct amounts, regardless of some users being confused by them.

RT-AC86U_Mem_Free_vs_Available

@ExtremeFiretop
Copy link
Owner

@Martinski4GitHub
Why is it that since we stopped using get_free_ram() function that my memory in the script no longer follows the memory in the WebUI?
For example, at the backup stage my WebUI got to 55MB of free memory, but the script was still reporting some 330MB of free memory. When I got back to get_free_ram function, now all of a sudden the script follows the WebUI perfectly.
I understand your method is technically more comprehensive, but I feel like it would throw users off actually comparing and monitoring like I just did.

From the perspective of the kernel & the OS memory management, the definition of "free" memory is different from the definition of "available" memory. The webGUI shows users the amount of "free" RAM, but what we want to know in the script is the amount of "available" RAM because that's more accurate when deciding whether we have enough to proceed with flashing the F/W image.

Some users may be confused but that's likely because of a lack of knowledge & understanding of the difference between "free" & "available" RAM. Note that the message output by the script is correct & clear:

It says: Required RAM: XXXX KB - Available RAM: YYYY KB"

It does not say: Required RAM: XXXX KB - Free RAM: YYYY KB"

Here's a screenshot showing the current memory on my router. Notice where the output from "/proc/meminfo" clearly labels the amounts for "Free" vs. "Available" RAM. Those are the correct amounts, regardless of some users being confused by them.

RT-AC86U_Mem_Free_vs_Available

Thanks for the great answer!

I'm willing to accept that answer, I understand that it's better for the script to have a more comprehensive view, my concern was really on user interaction.

In this case, in willing to just educate any user that may be confused or claim it isn't working correctly.

I figured maybe if we displayed the actual free ram, not available, then maybe we could avoid this.

@Martinski4GitHub
Copy link
Collaborator Author

In this case, in willing to just educate any user that may be confused or claim it isn't working correctly.

I figured maybe if we displayed the actual free ram, not available, then maybe we could avoid this.

We could easily change all the output messages to display all 3 amounts; something like this:

Required RAM: XXXX KB - Available RAM: YYYY KB" - Free RAM: ZZZZ KB.

This way, it becomes clearer for users who are non-technical or don't know any better, and they will see the difference between "Free" & "Available" RAM.

@ExtremeFiretop
Copy link
Owner

In this case, in willing to just educate any user that may be confused or claim it isn't working correctly.
I figured maybe if we displayed the actual free ram, not available, then maybe we could avoid this.

We could easily change all the output messages to display all 3 amounts; something like this:

Required RAM: XXXX KB - Available RAM: YYYY KB" - Free RAM: ZZZZ KB.

This way, it becomes clearer for users who are non-technical or don't know any better, and they will see the difference between "Free" & "Available" RAM.

Your right. this is the real solution.
Display free, but let the script actually care about availability. That way users don't freak out and the script still has the more comprehensive view.

@ExtremeFiretop
Copy link
Owner

In this case, in willing to just educate any user that may be confused or claim it isn't working correctly.
I figured maybe if we displayed the actual free ram, not available, then maybe we could avoid this.

We could easily change all the output messages to display all 3 amounts; something like this:

Required RAM: XXXX KB - Available RAM: YYYY KB" - Free RAM: ZZZZ KB.

This way, it becomes clearer for users who are non-technical or don't know any better, and they will see the difference between "Free" & "Available" RAM.

Let me fix these since I'm the one that changed them on the PR. I'll submit another shortly

@Martinski4GitHub
Copy link
Collaborator Author

Martinski4GitHub commented Jan 27, 2024

BTW, I'm starting to read through the DMs WRT Backupmon; but man, there's a boatload of them!!
I'm not sure I'll read them all tonight. Today was a long day for me, and reading emails/DMs is not really my idea of "relaxing after work." I'll try to get the gist of what you guys discussed & resolved if I can tonight, but I make no promises.

@ExtremeFiretop
Copy link
Owner

BTW, I'm starting to read through the DMs WRT Backupmon; but man, there's a boatload of them!! I'm not sure I'll read them all tonight. Today was a long day for me, and reading emails/DMs is not really my idea of "relaxing after work." I'll try to get the gist of what you guys discussed & resolved if I can tonight.

I didn't even expect you to reply tonight at all!
But I'm kinda happy you did, I'll handle the fixes you found and resubmit for you tomorrow if you have a few seconds.

As for the forums, um yeah... BACKUPMON is acting fishy. Viktor was hoping you could provide some advise because essentially it always unmounts my USB and not only that, also creates mount points which then cause the issue we had previously on our script of mounting as SDA(1) instead of SDA.

Viktor believes it's related to the labels, it's possible, but based on my tests, it's unlikely.

@Martinski4GitHub
Copy link
Collaborator Author

I didn't even expect you to reply tonight at all! But I'm kinda happy you did, I'll handle the fixes you found and resubmit for you tomorrow if you have a few seconds.

As for the forums, um yeah... BACKUPMON is acting fishy. Viktor was hoping you could provide some advise because essentially it always unmounts my USB and not only that, also creates mount points which then cause the issue we had previously on our script of mounting as SDA(1) instead of SDA.

Viktor believes it's related to the labels, it's possible, but based on my tests, it's unlikely.

IIRC, at some point Viktor was having problems with accurately detecting all the mount points to display them in a menu for users to be able to select, so I provided a function to do that. Then I vaguely remember him having issues with labels being blank, or getting the same label with a numeric suffix. I don't recall all the details, but I thought he had figured out the solution.

I'll likely need to know more context about the problem and when exactly happens so that I can recreate it if I can, or at least have a better idea of what's going on.

@Martinski4GitHub
Copy link
Collaborator Author

Also, IIRC, Backupmon originally did not make backups to the router USB-attached drives. It required a remote destination (e.g. NAS) so you needed to provide a UNC path. I have not followed very closely about what happens when you select a local USB drive as the destination. That's something that could potentially create problems for users when our script runs unattended via a cron job.

@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Jan 27, 2024

Also, IIRC, Backupmon originally did not make backups to the router USB-attached drives. It required a remote destination (e.g. NAS) so you needed to provide a UNC path. I have not followed very closely about what happens when you select a local USB drive as the destination. That's something that could potentially create problems for users when our script runs unattended via a cron job.

According to Viktor the logic is supposed to allow for it to backup to a USB and not unmount. So as long as that's the case I don't expect too many issues. Backupmon would backup to the NAS or USB, and then our script handles entware and other services and then the flash.

I guess I see potentially issues on a small USB or one low on storage if the logic doesn't cleanup like ours does, but otherwise I would think it's fine, right now the issue is it just rips the USB right out from under us so not only our script but other scripts can run into issues with this.

That could be a show stopper.

@Martinski4GitHub
Copy link
Collaborator Author

Also, IIRC, Backupmon originally did not make backups to the router USB-attached drives. It required a remote destination (e.g. NAS) so you needed to provide a UNC path. I have not followed very closely about what happens when you select a local USB drive as the destination. That's something that could potentially create problems for users when our script runs unattended via a cron job.

According to Viktor the logic is supposed to allow for it to backup to a USB and not unmount. So as long as that's the case I don't expect too many issues. Backupmon would backup to the NAS or USB, and then our script handles entware and other services and then the flash.

I guess I see potentially issues on a small USB or one low on storage if the logic doesn't cleanup like ours does, but otherwise I would think it's fine, right now the issue is it just rips the USB right out from under us so not only our script but other scripts can run into issues with this.

That could be a show stopper.

If at this point there are still issues with unmounting the USB drive unexpectedly, or with having the wrong labels attached to the mount point, I'd suggest postponing the integration with Backumon to a later release (post-AMTM integration) so that those issues can investigated and resolve without any pressures. Viktor may be able to find a solution with our help, but it sounds like it's not going to happen by tomorrow night unless we get lucky.

@ExtremeFiretop
Copy link
Owner

Also, IIRC, Backupmon originally did not make backups to the router USB-attached drives. It required a remote destination (e.g. NAS) so you needed to provide a UNC path. I have not followed very closely about what happens when you select a local USB drive as the destination. That's something that could potentially create problems for users when our script runs unattended via a cron job.

According to Viktor the logic is supposed to allow for it to backup to a USB and not unmount. So as long as that's the case I don't expect too many issues. Backupmon would backup to the NAS or USB, and then our script handles entware and other services and then the flash.
I guess I see potentially issues on a small USB or one low on storage if the logic doesn't cleanup like ours does, but otherwise I would think it's fine, right now the issue is it just rips the USB right out from under us so not only our script but other scripts can run into issues with this.
That could be a show stopper.

If at this point there are still issues with unmounting the USB drive unexpectedly, or with having the wrong labels attached to the mount point, I'd suggest postponing the integration with Backumon to a later release (post-AMTM integration) so that those issues can investigated and resolve without any pressures. Viktor may be able to find a solution with our help, but it sounds like it's not going to happen by tomorrow night unless we get lucky.

I was thinking the same thing

But thelonelycoder doesn't seem too pressured himse.
I'll give Viktor tomorrow. Otherwise we push an update after the fact with the code integration with Backupmon

@Martinski4GitHub
Copy link
Collaborator Author

If at this point there are still issues with unmounting the USB drive unexpectedly, or with having the wrong labels attached to the mount point, I'd suggest postponing the integration with Backumon to a later release (post-AMTM integration) so that those issues can investigated and resolve without any pressures. Viktor may be able to find a solution with our help, but it sounds like it's not going to happen by tomorrow night unless we get lucky.

I was thinking the same thing

But thelonelycoder doesn't seem too pressured himse. I'll give Viktor tomorrow. Otherwise we push an update after the fact with the code integration with Backupmon.

OK, I'll leave it up to you. IMO, tomorrow we should focus on the things we can control ourselves within our script, like making sure we are handling the Alpha & Beta releases correctly (i.e. not updating from a master release to an Alpha or Beta version; and based on user selection, to update from an Alpha/Beta version to the next master release). Also, I saw a possible issue (which I put a comment on) about selecting the default mount point for ZIP & LOG files instead of the user-selected destination. As I said in my comment, we need to make sure we honor the user's selection.

@ExtremeFiretop
Copy link
Owner

If at this point there are still issues with unmounting the USB drive unexpectedly, or with having the wrong labels attached to the mount point, I'd suggest postponing the integration with Backumon to a later release (post-AMTM integration) so that those issues can investigated and resolve without any pressures. Viktor may be able to find a solution with our help, but it sounds like it's not going to happen by tomorrow night unless we get lucky.

I was thinking the same thing
But thelonelycoder doesn't seem too pressured himse. I'll give Viktor tomorrow. Otherwise we push an update after the fact with the code integration with Backupmon.

OK, I'll leave it up to you. IMO, tomorrow we should focus on the things we can control ourselves within our script, like making sure we are handling the Alpha & Beta releases correctly (i.e. not updating from a master release to an Alpha or Beta version; and based on user selection, to update from an Alpha/Beta version to the next master release). Also, I saw a possible issue (which I put a comment on) about selecting the default mount point for ZIP & LOG files instead of the user-selected destination. As I said in my comment, we need to make sure we honor the user's selection.

Agreed.

It's not a deal breaker, but I want to get it done. I keep pushing it off for other things like Backupmon today, but like you said I rather just tighten everything up tomorrow so we can send it off.

I also saw your notes, I fixed the one I'm just trying to wrap my brain around the other lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

question Further information is requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants