Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[armhf][Nokia-7215] changes fstrim.timer to daily #14723

Merged
merged 1 commit into from
May 3, 2023

Conversation

jon-nokia
Copy link
Contributor

@jon-nokia jon-nokia commented Apr 19, 2023

Using override.conf, we modify the fstrim.timer service.

For armhf, Nokia-7215 platform, we modify fstrim.timer to run daily
instead of weekly. This is required because the size of the SSD on
this platform is 16GB, which on average is nearly 10 times smaller than
most other sonic platforms. With smaller disk and the ever increasing
level of logging done by sonic, this change is required to prevent
the SSD from entering a read-only state due to inadequate free blocks.

Why I did it

Required to prevent the SSD from entering a read-only state due to inadequate free blocks

Work item tracking
  • Microsoft ADO (number only):

How I did it

How to verify it

Verified that override works.

admin@sonic-amazon:~$ sudo systemctl status fstrim.timer 
\u25cf fstrim.timer - Discard unused blocks daily
     Loaded: loaded (/lib/systemd/system/fstrim.timer; enabled; vendor preset: enabled)
    Drop-In: /usr/lib/systemd/system/fstrim.timer.d
             \u2514\u2500timer-override.conf
     Active: active (waiting) since Thu 2023-04-27 14:14:18 UTC; 36min ago
    Trigger: Fri 2023-04-28 00:00:00 UTC; 9h left
   Triggers: \u25cf fstrim.service
       Docs: man:fstrim

Apr 27 14:14:18 sonic systemd[1]: Started Discard unused blocks daily.
admin@sonic-amazon:~$ cat /usr/lib/systemd/system/fstrim.timer.d/timer-override.conf 
[Unit]
Description=Discard unused blocks daily

[Timer]
OnCalendar=daily
admin@sonic-amazon:~$ 

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211

Tested branch (Please provide the tested image version)

  • 202012
  • master

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@jon-nokia jon-nokia marked this pull request as ready for review April 19, 2023 17:05
@jon-nokia jon-nokia requested a review from lguohan as a code owner April 19, 2023 17:05
@jon-nokia
Copy link
Contributor Author

Additional reviewers @carl-nokia @mlok-nokia @Pavan-Nokia


if [ -r /host/machine.conf ]; then
# fstrim frequency to daily for our platform
desc="Discard unused blocks ($platform)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jon-nokia where is 'desc' used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used in the sed command to change the description from "Discard unused blocks once a week". Since I was changing the OnCalendar value the description no longer made sense.

desc="Discard unused blocks ($platform)"
sed -i -e "s,OnCalendar=.*,OnCalendar=daily,g" \
-e "s,Description=.*,Description=$desc,g" \
/usr/lib/systemd/system/fstrim.timer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of modifying the packaged file, please create an override file for this timer, in /etc/systemd/system/fstrim.timer.d/. This is far cleaner and less likely to break something in the case of some updates/changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. That is much cleaner. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced original commit with this use of override.conf.

@@ -1,5 +1,6 @@
nokia-7215_plt_setup.sh usr/sbin
7215/scripts/nokia-7215init.sh usr/local/bin
7215/service/nokia-7215init.service etc/systemd/system
7215/service/fstrim.timer/override.conf /etc/systemd/system/fstrim.timer.d
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, if installing in a package, it's better to have it be in /lib/systemd/system/fstrim.timer.d. Also, I'd recommend naming it something besides override.conf, in case there happens to be some conflict (which is unlikely).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For /etc/systemd, I followed your example as well as that of the file, nokia-7215init.service. I'm not sure when to use /etc/systemd vs /lib/systemd and what the convention really is. Are you sure we need to change this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The /etc/systemd directory is meant for service files and overrides that are created by the system administrator. These are generally not installed through a package. The /lib/systemd directory are for service files that do come from a package. See this for a more detailed description.

I would recommend following the convention, at least for consistency. At the very least, since override.conf is a common name for service file overrides, and since this is coming from a package, I'd highly recommend renaming this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saiarcot895, Please check if the latest update is satisfactory. Thanks

prgeor
prgeor previously approved these changes Apr 25, 2023
Using timer-override.conf, we modify the fstrim.timer service.

For armhf, Nokia-7215 platform, we modify fstrim.timer to run daily
instead of weekly.  This is required because the size of the SSD on
this platform is 16GB, which on average is nearly 10 times smaller than
most other sonic platforms.  With smaller disk and the ever increasing
level of logging done by sonic, this change is required to prevent
the SSD from entering a read-only state due to inadequate free blocks.
@prgeor
Copy link
Contributor

prgeor commented Apr 28, 2023

@jon-nokia have you tested this new change?

@jon-nokia
Copy link
Contributor Author

@jon-nokia have you tested this new change?

Yes, I posted results in verification section for you just now.

@prgeor
Copy link
Contributor

prgeor commented Apr 28, 2023

@lguohan please help merge this PR

@prgeor
Copy link
Contributor

prgeor commented May 2, 2023

@lguohan please help merge this PR

@lguohan could you merge

@lguohan lguohan merged commit 0692e8a into sonic-net:master May 3, 2023
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request May 11, 2023
Using timer-override.conf, we modify the fstrim.timer service.

For armhf, Nokia-7215 platform, we modify fstrim.timer to run daily
instead of weekly.  This is required because the size of the SSD on
this platform is 16GB, which on average is nearly 10 times smaller than
most other sonic platforms.  With smaller disk and the ever increasing
level of logging done by sonic, this change is required to prevent
the SSD from entering a read-only state due to inadequate free blocks.
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202012: #15015

mssonicbld pushed a commit that referenced this pull request May 11, 2023
Using timer-override.conf, we modify the fstrim.timer service.

For armhf, Nokia-7215 platform, we modify fstrim.timer to run daily
instead of weekly.  This is required because the size of the SSD on
this platform is 16GB, which on average is nearly 10 times smaller than
most other sonic platforms.  With smaller disk and the ever increasing
level of logging done by sonic, this change is required to prevent
the SSD from entering a read-only state due to inadequate free blocks.
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request May 17, 2023
Using timer-override.conf, we modify the fstrim.timer service.

For armhf, Nokia-7215 platform, we modify fstrim.timer to run daily
instead of weekly.  This is required because the size of the SSD on
this platform is 16GB, which on average is nearly 10 times smaller than
most other sonic platforms.  With smaller disk and the ever increasing
level of logging done by sonic, this change is required to prevent
the SSD from entering a read-only state due to inadequate free blocks.
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request May 17, 2023
Using timer-override.conf, we modify the fstrim.timer service.

For armhf, Nokia-7215 platform, we modify fstrim.timer to run daily
instead of weekly.  This is required because the size of the SSD on
this platform is 16GB, which on average is nearly 10 times smaller than
most other sonic platforms.  With smaller disk and the ever increasing
level of logging done by sonic, this change is required to prevent
the SSD from entering a read-only state due to inadequate free blocks.
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202211: #15125

@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202205: #15126

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

Successfully merging this pull request may close these issues.

7 participants