During cluster startup, nodes were temporarily using the primary cluster
endpoint for temperature collection before cluster metadata validation
completed. This caused all nodes to show the same (incorrect) temperature
values for ~4 minutes until validation finished and per-node endpoints
were established.
Example: minipc would show delly's temperature (90°C) instead of its own
(50°C) from startup until cluster validation completed.
Root cause:
- Temperature collection started immediately at startup
- Cluster endpoint validation happened asynchronously
- Code fell back to primary endpoint when ClusterEndpoints was empty
- All nodes used same endpoint, got same temperature data
Fix: Skip temperature collection for cluster nodes until:
1. ClusterEndpoints array is populated (validation complete)
2. Node's specific endpoint is found in the cluster metadata
This ensures correct temperature data from the very first collection,
maintaining data integrity during startup. When persisted config exists,
endpoints are available immediately so no delay occurs. For new clusters,
temperature collection begins once validation completes (~30s).
Preserves Pulse's correctness guarantee: users can trust metrics
immediately after restart without waiting for "warm-up" period.
The update_allowed_nodes function was changing ownership of the temp file
before all writes were complete, causing 'Permission denied' errors when
appending the allowed_nodes section.
Root cause:
- mktemp creates file owned by script runner (root)
- chown changed ownership to pulse-sensor-proxy:pulse-sensor-proxy
- Subsequent append (>>) failed because root can't write to the file
Fix: Defer chown until after all writes complete and file is moved to
final location. Ownership is still correctly set on the final config file.
The full cleanup implementation (ed65fda74) has architectural issues that need addressing:
1. Read-only /usr filesystems prevent binary removal
2. Process isolation issues cause cleanup service to be killed
3. Cleanup loops from improper request file handling
This TODO documents the required changes following Codex recommendations:
- Relocate binaries to /opt/pulse/sensor-proxy/
- Use transient systemd units for uninstall orchestration
- Add flock serialization and proper cleanup-request handling
Current state: SSH keys are removed (critical), full cleanup needs refactoring.
When a Proxmox node is removed from Pulse, the cleanup now performs full uninstallation:
- SSH keys removal (existing functionality)
- Uninstalls pulse-sensor-proxy service
- Removes LXC bind mounts from container configs
- Deletes Proxmox API tokens
- Removes pulse-monitor@pam user
This aligns with security best practices and user expectations - "remove node"
should completely sever trust with that machine, not leave credentials and
privileged services behind.
The cleanup script now calls the uninstaller (--uninstall) and uses pveum
to remove API tokens. This prevents leftover artifacts if the host is
repurposed or compromised.
Related: config_handlers.go triggerPVEHostCleanup() at node deletion
- Change mktemp to use /tmp/pulse-config.XXXXXXXXXX template
- Prevents conflicts with stale temp files from previous runs
- Fixes 'Permission denied' errors when script re-runs
- Add trap to remove temp file on function return (success or failure)
- Add error check for mv command with descriptive message
- Ensure config file has proper permissions after update
This prevents orphaned temp files when errors occur and provides
better diagnostics when file operations fail.
The all_nodes arrays were declared with 'local' keyword outside of
functions, causing bash syntax error:
'local: can only be used in a function'
Fixed by removing 'local' keyword - arrays in main script scope don't
need it and it's actually invalid syntax.
The awk logic was removing allowed_nodes sections but leaving their
comment headers behind. When multiple sections existed, comments would
accumulate.
New approach:
- Buffer all comment lines encountered outside sections
- When a non-comment line is found, flush buffered comments
- When allowed_nodes is found, discard buffered comments (they belonged
to the section we're removing)
- This cleanly removes section headers like:
'# Cluster nodes (auto-discovered during installation)'
'# These nodes are allowed to request...'
Tested with config containing duplicate allowed_nodes sections - now
correctly produces clean output with all duplicates and headers removed.
The installer was only creating base config.yaml in standalone mode,
but update_allowed_nodes() is also called in LXC mode. When the config
didn't exist, update_allowed_nodes() would create an empty file and only
add the allowed_nodes section, missing required fields like
allowed_peer_uids, metrics_address, rate_limit, etc.
This caused the proxy to fail when it tried to parse the incomplete config.
Now creates a proper base config with all required fields if the file
doesn't exist, before any mode-specific configuration is added.
The install-sensor-proxy.sh script was blindly appending allowed_nodes
sections to the config file without checking if they already existed.
When the script was re-run or if the initial config already had an
allowed_nodes section, this created duplicate YAML keys that caused
the proxy service to fail with parse errors.
Changes:
- Add update_allowed_nodes() helper function that safely updates the
allowed_nodes section by removing any existing ones first
- Replace all three cat >> config.yaml heredocs with calls to the
helper function (cluster nodes, standalone mode, pvecm fallback)
- Uses awk to properly parse and remove multi-line YAML sections
This makes the installer idempotent and prevents config corruption on
re-runs.
Fixes issue where proxy service crashed with:
'mapping key "allowed_nodes" already defined at line X'
