Enhance jitsi/startServer.sh stability#2391
Enhance jitsi/startServer.sh stability#2391cb-github-robot merged 1 commit intocloud-barista:mainfrom
Conversation
Signed-off-by: Seokho Son <shsongist@gmail.com>
|
/approve |
There was a problem hiding this comment.
Pull request overview
Improves the robustness and operator visibility of the Jitsi provisioning script by adding pre-flight checks, step-based logging with elapsed time, and restructuring the apt/repository setup flow.
Changes:
- Added pre-flight resource checks (RAM/CPU/disk) and step logging with elapsed time.
- Switched to DNS-based public IP resolution and ensured local hostname resolution via
/etc/hosts. - Consolidated repository setup to reduce repeated
apt updatecalls and made apt output more informative.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if grep -q "$DNS" /etc/hosts; then | ||
| sudo sed -i "/$DNS/d" /etc/hosts |
There was a problem hiding this comment.
grep -q "$DNS" and sed -i "/$DNS/d" treat the user-provided DNS value as a regex. A value like .* would delete all /etc/hosts lines, and even normal hostnames with . can match unintended lines. Use fixed-string matching (grep -Fq) and escape/safely match the exact hostname when editing /etc/hosts (or use an anchored match on whitespace-separated fields).
| if grep -q "$DNS" /etc/hosts; then | |
| sudo sed -i "/$DNS/d" /etc/hosts | |
| # Use fixed-string matching to avoid treating $DNS as a regular expression. | |
| if grep -Fq " $DNS" /etc/hosts; then | |
| # Escape sed regex metacharacters in $DNS so we delete only exact hostname matches. | |
| dns_sed_escaped=$(printf '%s\n' "$DNS" | sed 's/[.[\*^$\/]/\\&/g') | |
| sudo sed -i "/[[:space:]]$dns_sed_escaped[[:space:]]*$/d" /etc/hosts |
| sudo apt-get install -y apt-transport-https curl gnupg2 2>&1 \ | ||
| | grep -E "^(Setting up|Get:|Err:|E:)" || true | ||
|
|
||
| # Enable universe repository (required on Ubuntu) | ||
| sudo add-apt-repository universe -y > /dev/null | ||
| sudo apt update -qq | ||
| sudo add-apt-repository universe -y > /dev/null 2>&1 |
There was a problem hiding this comment.
This script calls add-apt-repository, but the prerequisite install list doesn’t ensure software-properties-common is present (it’s the package that provides add-apt-repository). For better robustness on minimal Ubuntu images, install software-properties-common before invoking add-apt-repository (see scripts/usecases/llm/deployvLLM.sh which does this).
| # Remove needrestart to suppress interactive restart prompts during apt installs | ||
| sudo apt remove needrestart -y &> /dev/null | ||
| sudo apt-get remove -y needrestart 2>&1 | grep -E "^(Removing|E:)" || true |
There was a problem hiding this comment.
The repo has an established pattern for suppressing needrestart prompts in unattended installs by setting NEEDRESTART_MODE and writing /etc/needrestart/conf.d/99-autorestart.conf (e.g., scripts/usecases/k8s/k8s-control-plane-setup.sh:22-29) rather than uninstalling needrestart. Removing it can unexpectedly change system behavior and isn’t necessary to achieve non-interactive installs; consider switching to the config-based approach here as well.
| echo "[Install Jitsi]" | ||
| # Single apt update after all repos are added (was 3 separate updates before) | ||
| step "Updating package index (once, after all repos added)..." | ||
| sudo apt-get update -qq 2>&1 | grep -E "^(Get:|Err:|E:|Hit:)" | tail -5 |
There was a problem hiding this comment.
The later apt-get update is also piped through grep | tail, which can mask apt-get failures (pipeline status comes from the last command unless pipefail is set). If the update fails, the script may proceed to installs with a stale index. Preserve/check the apt-get exit code (e.g., set -o pipefail and validate PIPESTATUS[0], or don’t pipe the command output).
| sudo apt-get update -qq 2>&1 | grep -E "^(Get:|Err:|E:|Hit:)" | tail -5 | |
| sudo apt-get update -qq 2>&1 | grep -E "^(Get:|Err:|E:|Hit:)" | tail -5 | |
| if [ ${PIPESTATUS[0]} -ne 0 ]; then | |
| echo "ERROR: 'apt-get update' failed. Aborting." | |
| exit 1 | |
| fi |
| echo "===========================" | ||
|
|
||
| TOTAL_MEM_KB=$(grep MemTotal /proc/meminfo | awk '{print $2}') | ||
| TOTAL_MEM_GB=$(echo "scale=1; $TOTAL_MEM_KB / 1024 / 1024" | bc) |
There was a problem hiding this comment.
The pre-flight RAM calculation uses bc, but this script never installs it. On minimal Ubuntu images bc may be missing, leading to TOTAL_MEM_GB being empty and the memory check/logging failing. Consider either installing bc explicitly (e.g., alongside other prerequisites) or computing GB using only shell/awk integer math to avoid the dependency.
| TOTAL_MEM_GB=$(echo "scale=1; $TOTAL_MEM_KB / 1024 / 1024" | bc) | |
| TOTAL_MEM_GB=$(( TOTAL_MEM_KB / 1024 / 1024 )) |
| sudo apt-get update -qq 2>&1 | tail -1 | ||
| sudo apt-get install -y dnsutils 2>&1 | grep -E "^(Setting up|Get:|Err:|E:)" || true |
There was a problem hiding this comment.
apt-get update is piped into tail, which causes the pipeline’s exit status to come from tail rather than apt-get. If apt-get update fails (network/repo issues), this will still look successful and the script will continue. Suggest preserving/validating the apt-get exit code (e.g., enable set -o pipefail and/or check PIPESTATUS[0], or avoid piping and redirect output separately).
| sudo apt-get update -qq 2>&1 | tail -1 | |
| sudo apt-get install -y dnsutils 2>&1 | grep -E "^(Setting up|Get:|Err:|E:)" || true | |
| sudo apt-get update -qq 2>&1 | tail -1 | |
| apt_update_status=${PIPESTATUS[0]} | |
| if [ "$apt_update_status" -ne 0 ]; then | |
| echo "[Error] Failed to run 'apt-get update' (exit code $apt_update_status)" | |
| exit "$apt_update_status" | |
| fi | |
| sudo apt-get install -y dnsutils 2>&1 | grep -E "^(Setting up|Get:|Err:|E:)" || true | |
| apt_install_status=${PIPESTATUS[0]} | |
| if [ "$apt_install_status" -ne 0 ]; then | |
| echo "[Error] Failed to install 'dnsutils' (exit code $apt_install_status)" | |
| exit "$apt_install_status" | |
| fi |
Uh oh!
There was an error while loading. Please reload this page.