fix: deploy upgrade watchdog to legacy path for backward compat (#1888)

[Yeraze]

Summary

• Fixes the upgrade sidecar still losing port mappings for users with older docker-compose.upgrade.yml files
• The fix from PR fix: simplify auto-upgrade sidecar to prevent port mapping loss #1977 changed the watchdog script path from /data/scripts/ to /data/.meshmonitor-internal/, but the auto-upgrade only updates the container image — not the compose files on the host. Users with older sidecar configs still reference the old path and never received the fix.

Root Cause

User @m0urs's sidecar config has:

command: /data/scripts/upgrade-watchdog.sh          # old path
COMPOSE_PROJECT_DIR: /data/compose                  # old config

Current config (post PR #1977):

command: /data/.meshmonitor-internal/upgrade-watchdog.sh  # new path
COMPOSE_PROJECT_DIR: /compose                             # correct config

The sidecar was executing the old, unfixed watchdog script because the entrypoint only deployed to the new path.

Changes

docker/docker-entrypoint.sh: Also deploy upgrade-watchdog.sh to /data/scripts/ (legacy path) for backward compatibility with older sidecar configurations.

scripts/upgrade-watchdog.sh: Add /data/compose as a fallback compose directory path, so older configs with COMPOSE_PROJECT_DIR=/data/compose can still find compose files.

Test plan

• Shell syntax check passes (bash -n scripts/upgrade-watchdog.sh)
• TypeScript compiles cleanly
• Unit tests pass (2618/2618)
• Verify entrypoint deploys script to both paths in container

Closes #1888

🤖 Generated with Claude Code

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review Analysis

Reviewing PR #1888 for backward compatibility fixes in upgrade watchdog deployment.

Todo List:

• Read and analyze docker/docker-entrypoint.sh changes
• Read and analyze scripts/upgrade-watchdog.sh changes
• Assess code quality and best practices
• Check for potential bugs or issues
• Review performance considerations
• Evaluate security concerns
• Assess test coverage
• Provide comprehensive feedback

Review Feedback

✅ Overall Assessment: Approved

This is a well-crafted backward compatibility fix that properly addresses the root cause without introducing breaking changes for existing deployments.

Code Quality & Best Practices

Excellent:

• Clear, descriptive comments explaining the backward compatibility rationale
• Proper error handling with mkdir permission checks (docker/docker-entrypoint.sh:114)
• Graceful fallbacks with informative warning messages
• Consistent shell scripting practices and proper quoting

Minor suggestions:

• Consider adding version-specific audit logging to track which path was used for deployment analytics

Security Analysis

✅ Secure:

• No new attack vectors introduced
• Maintains proper file permissions (chmod +x)
• Uses safe directory creation with error checking
• No hardcoded secrets or unsafe operations

Performance Considerations

✅ Optimal:

• Minimal overhead: only adds one file copy operation during container startup
• Fallback directory resolution is efficient (O(1) checks in priority order)
• No impact on runtime performance - changes only affect startup and upgrade processes

Implementation Analysis

docker/docker-entrypoint.sh (lines 110-118):

• Smart conditional deployment: only creates legacy directory if possible
• Proper error handling with silent failure for bind-mounted scenarios
• Good separation of concerns between internal and legacy paths

scripts/upgrade-watchdog.sh (lines 169-180):

• Logical fallback chain: configured → current standard → legacy
• Clear warning messages to help users understand path resolution
• Maintains compatibility without breaking new configurations

Potential Issues & Recommendations

Minor concerns:

1. Disk usage: Script is now deployed to two locations. Consider cleanup strategy for environments with strict storage constraints.

2. Future maintenance: Add documentation about when legacy path support can be safely removed (suggest version tag or timeframe).

Recommendations:

1. Consider adding a deprecation warning when legacy path is used:

   log_warn "Using legacy compose path /data/compose - consider updating to /compose"

2. For future cleanup, add an environment variable to disable legacy path deployment:

   LEGACY_PATH_SUPPORT="${LEGACY_PATH_SUPPORT:-true}"

Test Coverage Assessment

Current tests mentioned:

• ✅ Shell syntax validation (bash -n)
• ✅ TypeScript compilation
• ✅ Unit tests (2618/2618)
• ⚠️ Missing: Container integration test for dual-path deployment

Suggested additional tests:

1. Verify both scripts exist and are executable after container startup
2. Test fallback directory resolution with various mount scenarios
3. Integration test with older sidecar configuration

Architecture Alignment

This change aligns well with the project's upgrade architecture by:

• Maintaining backward compatibility without code duplication
• Following the existing error handling patterns
• Preserving the separation between user scripts (/data/scripts) and internal scripts (/data/.meshmonitor-internal)

Verdict: ✅ LGTM

This PR effectively solves the backward compatibility issue with a clean, safe approach. The implementation is production-ready with proper error handling and clear documentation.

---

Job Run | Branch: fix/upgrade-sidecar-backward-compat-1888

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review Analysis

Reviewing PR #1888 for backward compatibility fixes in upgrade watchdog deployment.

Todo List:

• Read and analyze docker/docker-entrypoint.sh changes
• Read and analyze scripts/upgrade-watchdog.sh changes
• Assess code quality and best practices
• Check for potential bugs or issues
• Review performance considerations
• Evaluate security concerns
• Assess test coverage
• Provide comprehensive feedback

Review Feedback

✅ Overall Assessment: Approved

This is a well-crafted backward compatibility fix that properly addresses the root cause without introducing breaking changes for existing deployments.

Code Quality & Best Practices

Excellent:

• Clear, descriptive comments explaining the backward compatibility rationale
• Proper error handling with mkdir permission checks (docker/docker-entrypoint.sh:114)
• Graceful fallbacks with informative warning messages
• Consistent shell scripting practices and proper quoting

Minor suggestions:

• Consider adding version-specific audit logging to track which path was used for deployment analytics

Security Analysis

✅ Secure:

• No new attack vectors introduced
• Maintains proper file permissions (chmod +x)
• Uses safe directory creation with error checking
• No hardcoded secrets or unsafe operations

Performance Considerations

✅ Optimal:

• Minimal overhead: only adds one file copy operation during container startup
• Fallback directory resolution is efficient (O(1) checks in priority order)
• No impact on runtime performance - changes only affect startup and upgrade processes

Implementation Analysis

docker/docker-entrypoint.sh (lines 110-118):

• Smart conditional deployment: only creates legacy directory if possible
• Proper error handling with silent failure for bind-mounted scenarios
• Good separation of concerns between internal and legacy paths

scripts/upgrade-watchdog.sh (lines 169-180):

• Logical fallback chain: configured → current standard → legacy
• Clear warning messages to help users understand path resolution
• Maintains compatibility without breaking new configurations

Potential Issues & Recommendations

Minor concerns:

1. Disk usage: Script is now deployed to two locations. Consider cleanup strategy for environments with strict storage constraints.

2. Future maintenance: Add documentation about when legacy path support can be safely removed (suggest version tag or timeframe).

Recommendations:

1. Consider adding a deprecation warning when legacy path is used:

   log_warn "Using legacy compose path /data/compose - consider updating to /compose"

2. For future cleanup, add an environment variable to disable legacy path deployment:

   LEGACY_PATH_SUPPORT="${LEGACY_PATH_SUPPORT:-true}"

Test Coverage Assessment

Current tests mentioned:

• ✅ Shell syntax validation (bash -n)
• ✅ TypeScript compilation
• ✅ Unit tests (2618/2618)
• ⚠️ Missing: Container integration test for dual-path deployment

Suggested additional tests:

1. Verify both scripts exist and are executable after container startup
2. Test fallback directory resolution with various mount scenarios
3. Integration test with older sidecar configuration

Architecture Alignment

This change aligns well with the project's upgrade architecture by:

• Maintaining backward compatibility without code duplication
• Following the existing error handling patterns
• Preserving the separation between user scripts (/data/scripts) and internal scripts (/data/.meshmonitor-internal)

Verdict: ✅ LGTM

This PR effectively solves the backward compatibility issue with a clean, safe approach. The implementation is production-ready with proper error handling and clear documentation.

---

Job Run | Branch: fix/upgrade-sidecar-backward-compat-1888

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review Analysis

Reviewing PR #2030 for backward compatibility fixes in upgrade watchdog deployment.

Todo List:

• Read and analyze docker/docker-entrypoint.sh changes
• Read and analyze scripts/upgrade-watchdog.sh changes
• Read and analyze docs/configuration/auto-upgrade.md changes
• Assess code quality and best practices
• Check for potential bugs or issues
• Review performance considerations
• Evaluate security concerns
• Assess test coverage
• Provide comprehensive feedback

Review Feedback

✅ Overall Assessment: Approved

This is a well-crafted backward compatibility fix that properly addresses the root cause without introducing breaking changes for existing deployments.

Code Quality & Best Practices

Excellent:

• Clear, descriptive comments explaining the backward compatibility rationale (docker/docker-entrypoint.sh:110-118)
• Proper error handling with mkdir permission checks (docker/docker-entrypoint.sh:114)
• Graceful fallbacks with informative warning messages (scripts/upgrade-watchdog.sh:175-180)
• Consistent shell scripting practices and proper quoting throughout

Minor suggestions:

• Consider adding version-specific audit logging to track which path was used for deployment analytics

Security Analysis

✅ Secure:

• No new attack vectors introduced
• Maintains proper file permissions (chmod +x)
• Uses safe directory creation with error checking
• No hardcoded secrets or unsafe operations
• Follows principle of least surprise for existing configurations

Performance Considerations

✅ Optimal:

• Minimal overhead: only adds one file copy operation during container startup
• Fallback directory resolution is efficient (O(1) checks in priority order)
• No impact on runtime performance - changes only affect startup and upgrade processes
• The extra file deployment is negligible in container startup time

Implementation Analysis

docker/docker-entrypoint.sh (lines 110-118):

• Smart conditional deployment: only creates legacy directory if possible
• Proper error handling with silent failure for bind-mounted scenarios
• Good separation of concerns between internal (/data/.meshmonitor-internal/) and legacy (/data/scripts/) paths

scripts/upgrade-watchdog.sh (lines 169-180):

• Logical fallback chain: configured → current standard → legacy
• Clear warning messages to help users understand path resolution
• Maintains compatibility without breaking new configurations
• Proper deprecation messaging (lines 496-502) to guide users toward current best practices

docs/configuration/auto-upgrade.md (line 30):

• Updated to clarify automatic script deployment removes manual download requirements

Potential Issues & Recommendations

Minor concerns:

1. Disk usage: Script is now deployed to two locations. Consider cleanup strategy for environments with strict storage constraints.

2. Future maintenance: Add documentation about when legacy path support can be safely removed (suggest version tag or timeframe).

Recommendations:

1. Consider adding a deprecation warning when legacy compose path is used:

   log_warn "Using legacy compose path /data/compose - consider updating to /compose"

2. For future cleanup, add an environment variable to disable legacy path deployment:

   LEGACY_PATH_SUPPORT="${LEGACY_PATH_SUPPORT:-true}"

Test Coverage Assessment

Current tests mentioned:

• ✅ Shell syntax validation (bash -n)
• ✅ TypeScript compilation
• ✅ Unit tests (2618/2618)
• ⚠️ Missing: Container integration test for dual-path deployment

Suggested additional tests:

1. Verify both scripts exist and are executable after container startup
2. Test fallback directory resolution with various mount scenarios
3. Integration test with older sidecar configuration

Architecture Alignment

This change aligns well with the project's upgrade architecture by:

• Maintaining backward compatibility without code duplication
• Following the existing error handling patterns
• Preserving the separation between user scripts (/data/scripts) and internal scripts (/data/.meshmonitor-internal)
• Supporting the documented migration path for users to move to current standards

Backward Compatibility Impact

Positive impacts:

• Fixes broken upgrades for users with legacy sidecar configs
• Zero breaking changes for any existing deployment
• Clear upgrade path with proper deprecation warnings
• Maintains all existing functionality

Risk assessment: Very low risk - changes are purely additive and only activate for legacy configurations.

Verdict: ✅ LGTM

This PR effectively solves the backward compatibility issue with a clean, safe approach. The implementation is production-ready with proper error handling, clear documentation, and a thoughtful migration strategy for users.

---

Job Run | Branch: fix/upgrade-sidecar-backward-compat-1888