[improvement]: Suggestions to improve robustness and reliability

[tdltdl]

FreePBX Version

FreePBX 17

Improvement Description

Here are some suggestions for the script "sng_freepbx_debian_install.sh"

• add "set -e" at the beginning of the script so if anything wrong occurs, in conjunction with the second suggestion about trap, it is noticeable (and we do not end up with a partially installed system).
• add a "trap xxx EXIT" where xxx is doing some cleanup (delete the pidfile, detect incomplete installation and maybe do some cleanup if the installation was not completed or store the steps that were already completed if they are not idempotent so at next run they can be skipped). That sill also remove the need to explicitly call "terminate" which is not done consistently (example if an unknown option is passed)
• add a date/time to the log filename (something like $(date "+%Y.%m.%d-%H.%M.%S") concatenated in the logfilename). This ensure that we keep th logs of the previous attempts.
• check for root should be one of the first thing to do (before trying to create the directories and log file)
• not sure it is desired, but if mongodb fails to install, the installation is not stopped
• if a package fails to install, the return code is 0 (due to the exit 0 in the terminate function), I would expect a non zero exit code.
• should the pid filename indicate it is the installation of freePBX17 (something like "/var/run/freepbx17_install.pid")?
• usually the pidfile contains a PID (echo $$ > pid_file). Having this could also allow to check if the proces ID is still actually running making the manual deletion unneeded if the proces is anyway not running anymore
• improve the postfix config (hardcoding "my.hostname.example" could be replace by $(hostname -f)" and "Internet Site" could be asked?)
• usage of apt-key add is deprecated when adding http://deb.freepbx.org/gpg/aptly-pubkey.asc
• would it make sense to automatically configure letsencrypt for SSL?
• some part of the script seems to support arm64 (deb in mongodb lists both amd64 and arm64), but the lib aac always download the amd64 arch.

Question

• What is the rationale for calling many times the pkg_install with each packages individually versus passing once the full list?
• aac library installed ins not the latest one (2.0.1-1 from March 2020, latest is 2.0.2-1 from Sept 2021). Should it be updated?

Disclosure: Those suggestions are only based on reading the script without executing it -so mileage can vary ;-).

[tdltdl]

What is the rationale to specify options "--force-confnew" and "--force-overwrite" when installing packages if the installation is only done when the package is not already installed?

[kguptasangoma]

Associated PR merged hence closing this jira. thanks

[tdltdl]

Thanks for accepting my PR. As per my comments in the PR, I believe th pkg install should remain as I changed it and we should also drop the force newconf and forceoverwrite.

[kguptasangoma]

Hey @tdltdl I merged the PR so we can test the new script. Got some hiccups and updated the codebase. Also looks like packages.sury.org is failing as well.

[tdltdl]

Hey @tdltdl I merged the PR so we can test the new script. Got some hiccups and updated the codebase. Also looks like packages.sury.org is failing as well. !

This is wget complaining that the certificate presented by the site is not known. This can be caused by the fact that ca-certificates package is not installed by default on your system (this can be the case for debian minimal? I do not know). So maybe before running the setting up repositories, it would be safer to install the following packages:

• wget
• gnugpg
• ca-certificates

Also what is the rationale to add twice the same deb entry (I see you restore this)

        add-apt-repository -y -S "deb [ arch=${arch} ] http://deb.freepbx.org/freepbx17-dev bookworm main" >> "$log" 2>&1
        add-apt-repository -y -S "deb [ arch=${arch} ] http://deb.freepbx.org/freepbx17-dev bookworm main" >> "$log" 2>&1

[kguptasangoma]

Error is for"certificate of the repo url is not trusted" so wondering if we can remove that repo ?

We have seen adding one time was not fixing the preference issue so had to do twice (some sort of repo issue) because we want "ffmpeg" to download from our repo not from debian one.

[tdltdl]

Error is for"certificate of the repo url is not trusted" so wondering if we can remove that repo ?

We have seen adding one time was not fixing the preference issue so had to do twice (some sort of repo issue) because we want "ffmpeg" to download from our repo not from debian one.

The repo is signed by let's encrypt, so a very common one. This does not make sense to add it twice and that it would "fix the issue". Maybe the ffmpeg was installed first before setting up the repo priority? This is also why I first setup the repositories and the priority, before installing al packages, but indeed, we must make sure that those packages we use in the setup of the repo are installed in the first place.

[kguptasangoma]

I agree we need to review this in deep why adding twice. will revisit this. Regarding repo as of now for other network or vultr it worked fine so not removing and will see if we hear complaints from others as well.

Thank you very much for your excellent work in improving this script.

Regards Kapil

[tdltdl]

Do you plan to change back the pkg_install, as it is a very important change in my opinion and there is no reason for me to undo it (and I would, as already mentioned, remove the "special options" that are either inactive or probably incorrect and probably add an apt upgrade while making environment sane).

[tdltdl]

Also note that I checked with apt policy ffmpeg and while having the "deb" line only once in my repo, I can confirm this is the Sangoma version that has been installed.

root@myhost# apt policy ffmpeg
ffmpeg:
  Installed: 5.1.4-7.sng12
  Candidate: 5.1.4-7.sng12
  Version table:
     7:4.4.2-0ubuntu0.22.04.1 500
        500 http://be.archive.ubuntu.com/ubuntu jammy-updates/universe amd64 Packages
        500 http://be.archive.ubuntu.com/ubuntu jammy-security/universe amd64 Packages
     7:4.4.1-3ubuntu5 500
        500 http://be.archive.ubuntu.com/ubuntu jammy/universe amd64 Packages
 *** 5.1.4-7.sng12 600
        600 http://deb.freepbx.org/freepbx17-prod bookworm/main amd64 Packages
        100 /var/lib/dpkg/status
     5.1.4-6.sng12 600
        600 http://deb.freepbx.org/freepbx17-prod bookworm/main amd64 Packages
     5.1.4-5.sng12 600
        600 http://deb.freepbx.org/freepbx17-prod bookworm/main amd64 Packages
     5.1.4-3.sng12 600
        600 http://deb.freepbx.org/freepbx17-prod bookworm/main amd64 Packages

[kguptasangoma]

pkg_install is doing below stuff - 1) Checking If package installed then do not install it again. 2) install the package 3) check again if package installed or not as due to various reason we may fail to install the package so if package is not installed properly then its exiting the process.

3rd one is an important step as there is no point of moving further if we fail to install the package.

[kguptasangoma]

Of course we can improve the script further to remove that duplicate apt-add line and test to ensure all good (may be initial days something else was happening), it just today considered other changes to kept those changes as it is.

[tdltdl]

pkg_install is doing below stuff -

1. Checking If package installed then do not install it again.
2. install the package
3. check again if package installed or not as due to various reason we may fail to install the package so if package is not installed properly then its exiting the process.

3rd one is an important step as there is no point of moving further if we fail to install the package (except of course if you pass the option "force overwrite - which I strongly believe we should remove - as anyway today, we will never attempt to overwrite it, because if we want to do it, it means it was already installed hence we will not even call the apt install again).

Step 1 is redundant as doing an apt install "existing package" multiple times will not attempt to reinstall it (except if you pass force overwrite, which I strongly believe we should remove). Step 3 is redundant as, if the installation fails for whatever reason the apt install with return a non zero code and that will be catched by the set -e and exit the installation, passing through the trap error to log the exact error and line it occurred and command that was executed. This is why I believe it is really not needed to do this.

[kguptasangoma]

Okay ,thanks for the explanation. will revert back the pkg_install fix.

[tdltdl]

I know now why you are enabling overwrite config:

Unpacking sangoma-pbx17 (2402-3.sng12) ...
dpkg: error processing archive /var/cache/apt/archives/sangoma-pbx17_2402-3.sng12_all.deb (--unpack):
 trying to overwrite '/etc/fail2ban/action.d/iptables-allports.conf', which is also in package fail2ban 0.11.2-6

I do not believe it is wise that sanfoma-pbx17 overwrites a config file from another package ;-(

Will create a dedicated issue for that, should those changes be part of the install script?