NM-288: fix skipping autoupdate for containers

[abhishek9686]

Describe your changes

Provide Issue ticket number if applicable/not in title

Provide link to Netmaker PR if required

Provide testing steps

Checklist before requesting a review

• My changes affect only 10 files or less.
• I have performed a self-review of my code and tested it.
• If it is a new feature, I have added thorough tests, my code is <= 1450 lines.
• If it is a bugfix, my code is <= 200 lines.
• My functions are <= 80 lines.
• I have had my code reviewed by a peer.
• My unit tests pass locally.
• Netclient & Netmaker are awesome.

[tenki-reviewer]

Tenki Code Review - Complete

Files Reviewed: 5
Findings: 3

By Severity:

• 🔴 High: 1
• 🟠 Medium: 1
• 🟡 Low: 1

---

This PR correctly refactors UseVersion to return a (skip bool, err error) tuple for container-aware upgrade skipping, but introduces a production-impacting bug: debug tracing calls in TraceCaller() were accidentally changed from slog.Debug to slog.Error, meaning every daemon restart will emit spurious ERROR-level log lines. There are also minor CLI formatting issues in cmd/use.go.

Files Reviewed (5 files)

cmd/use.go
daemon/common.go
functions/mqhandlers.go
functions/use_version.go
ncutils/netclientutils.go

[tenki-reviewer]

Overview

This PR adds container-awareness to the auto-upgrade flow by changing UseVersion from returning error to returning (skip bool, err error). When running inside a container, the function now returns (true, nil) instead of silently returning nil, allowing callers to emit an appropriate warning. All call sites in functions/mqhandlers.go and cmd/use.go have been updated consistently.

Issues Found

🔴 High — Spurious ERROR logs on every daemon restart (ncutils/netclientutils.go)

The TraceCaller() helper in ncutils/netclientutils.go lines 380-381 was changed from slog.Debug(...) to slog.Error(...). This function is called by daemon.Restart() (pre-existing) and now also daemon.HardRestart() (added in this PR). Because HardRestart() is invoked by every successful auto-upgrade path in mqhandlers.go, every successful upgrade will emit two ## TRACE -> lines at ERROR severity in production. This will trigger monitoring alerts, pollute error dashboards, and confuse operators — the ## TRACE prefix and the debug-style language make it obvious these belong to a temporary debugging tool, not a real error condition.

🟡 Medium — fmt.Println in cmd/use.go prints unformatted key-value arguments

In cmd/use.go line 20, fmt.Println("skipping auto-upgrade inside container, update the container image instead", "version", args[0]) passes three arguments. fmt.Println space-joins all arguments, so the output is skipping auto-upgrade inside container, update the container image instead version v0.18.0 — the literal word "version" appears mid-sentence rather than as a structured label. The equivalent daemon path (mqhandlers.go) correctly uses slog.Warn with key-value semantics.

🔵 Low — Independent if blocks instead of if/else-if in cmd/use.go

In cmd/use.go lines 19-24, skip and err are checked in separate if blocks rather than an if/else-if chain (as all mqhandlers.go sites do). Currently harmless because UseVersion always returns (true, nil) for the container case, but inconsistent and fragile if that contract ever changes.

What the PR Does Well

• The UseVersion signature change is clean and all return sites correctly propagate false for non-container paths.
• All four mqhandlers.go call sites consistently apply the if skip { ... } else if err != nil { ... } else { ... } pattern.
• The slog.Warn log message text is informative and actionable.