Fix node config API to preserve fields on partial updates

The PUT /api/config/nodes/{id} endpoint was corrupting node configurations
when making partial updates (e.g., updating just monitorPhysicalDisks):

- Authentication fields (tokenName, tokenValue, password) were being cleared
  when updating unrelated settings
- Name field was being blanked when not included in request
- Monitor* boolean fields were defaulting to false

Changes:
- Only update name field if explicitly provided in request
- Only switch authentication method when auth fields are explicitly provided
- Preserve existing auth credentials on non-auth updates
- Applied fix to all node types (PVE, PBS, PMG)

Also enables physical disk monitoring by default (opt-out instead of opt-in)
and preserves disk data between polling intervals.
This commit is contained in:
rcourtman 2025-10-12 17:50:55 +00:00
parent a328dbd8e6
commit c18cf3d4b8
3 changed files with 216 additions and 185 deletions

View file

@ -290,7 +290,7 @@ type NodeConfigRequest struct {
MonitorContainers bool `json:"monitorContainers,omitempty"` // PVE only
MonitorStorage bool `json:"monitorStorage,omitempty"` // PVE only
MonitorBackups bool `json:"monitorBackups,omitempty"` // PVE only
MonitorPhysicalDisks bool `json:"monitorPhysicalDisks,omitempty"` // PVE only
MonitorPhysicalDisks *bool `json:"monitorPhysicalDisks,omitempty"` // PVE only (nil = enabled by default)
MonitorDatastores bool `json:"monitorDatastores,omitempty"` // PBS only
MonitorSyncJobs bool `json:"monitorSyncJobs,omitempty"` // PBS only
MonitorVerifyJobs bool `json:"monitorVerifyJobs,omitempty"` // PBS only
@ -318,7 +318,7 @@ type NodeResponse struct {
MonitorContainers bool `json:"monitorContainers,omitempty"`
MonitorStorage bool `json:"monitorStorage,omitempty"`
MonitorBackups bool `json:"monitorBackups,omitempty"`
MonitorPhysicalDisks bool `json:"monitorPhysicalDisks,omitempty"`
MonitorPhysicalDisks *bool `json:"monitorPhysicalDisks,omitempty"`
MonitorDatastores bool `json:"monitorDatastores,omitempty"`
MonitorSyncJobs bool `json:"monitorSyncJobs,omitempty"`
MonitorVerifyJobs bool `json:"monitorVerifyJobs,omitempty"`
@ -720,7 +720,7 @@ func (h *ConfigHandlers) HandleGetNodes(w http.ResponseWriter, r *http.Request)
MonitorContainers: true,
MonitorStorage: true,
MonitorBackups: true,
MonitorPhysicalDisks: true,
MonitorPhysicalDisks: nil, // nil = enabled by default
Status: "connected",
IsCluster: true,
ClusterName: "mock-cluster",
@ -746,7 +746,7 @@ func (h *ConfigHandlers) HandleGetNodes(w http.ResponseWriter, r *http.Request)
MonitorContainers: true,
MonitorStorage: true,
MonitorBackups: true,
MonitorPhysicalDisks: true,
MonitorPhysicalDisks: nil, // nil = enabled by default
Status: "connected",
IsCluster: false, // Not part of a cluster
ClusterName: "",
@ -1437,7 +1437,11 @@ func (h *ConfigHandlers) HandleUpdateNode(w http.ResponseWriter, r *http.Request
// Update the node
if nodeType == "pve" && index < len(h.config.PVEInstances) {
pve := &h.config.PVEInstances[index]
pve.Name = req.Name
// Only update name if provided
if req.Name != "" {
pve.Name = req.Name
}
if req.Host != "" {
host := req.Host
@ -1456,30 +1460,37 @@ func (h *ConfigHandlers) HandleUpdateNode(w http.ResponseWriter, r *http.Request
pve.Host = host
}
// Handle authentication updates - only switch auth method if explicitly provided
if req.TokenName != "" || req.TokenValue != "" {
// Switching to or updating token authentication
if req.TokenName != "" {
pve.TokenName = req.TokenName
}
if req.TokenValue != "" {
pve.TokenValue = req.TokenValue
}
// When using token authentication, clear password to avoid conflicts
// Clear password to avoid conflicts
pve.Password = ""
if req.User != "" {
pve.User = req.User
}
} else {
} else if req.Password != "" {
// Explicitly switching to password authentication
if req.User != "" {
pve.User = normalizePVEUser(req.User)
} else if pve.User != "" {
pve.User = normalizePVEUser(pve.User)
}
if req.Password != "" {
pve.Password = req.Password
}
// Clear token fields when using password authentication
pve.Password = req.Password
// Clear token fields when switching to password auth
pve.TokenName = ""
pve.TokenValue = ""
} else {
// No authentication changes - preserve existing auth fields
// Only normalize user if it exists
if pve.User != "" {
pve.User = normalizePVEUser(pve.User)
}
}
pve.Fingerprint = req.Fingerprint
@ -1511,22 +1522,22 @@ func (h *ConfigHandlers) HandleUpdateNode(w http.ResponseWriter, r *http.Request
}
pbs.Host = host
// Determine authentication method and clear the unused fields
// Handle authentication updates - only switch auth method if explicitly provided
if req.TokenName != "" && req.TokenValue != "" {
// Using token authentication - clear user/password
pbs.User = ""
pbs.Password = ""
// Switching to token authentication
pbs.TokenName = req.TokenName
pbs.TokenValue = req.TokenValue
} else if req.TokenName != "" {
// Token name provided without new value - keep existing token value
// Clear user/password when switching to token auth
pbs.User = ""
pbs.Password = ""
} else if req.TokenName != "" {
// Token name provided without new value - keep existing token value
pbs.TokenName = req.TokenName
// Clear user/password when using token auth
pbs.User = ""
pbs.Password = ""
} else if req.Password != "" {
// Using password authentication - clear token fields
pbs.TokenName = ""
pbs.TokenValue = ""
// Switching to password authentication
pbs.Password = req.Password
// Ensure user has realm for PBS
pbsUser := req.User
@ -1534,17 +1545,22 @@ func (h *ConfigHandlers) HandleUpdateNode(w http.ResponseWriter, r *http.Request
pbsUser = req.User + "@pbs" // Default to @pbs realm if not specified
}
pbs.User = pbsUser
} else if req.User != "" {
// User provided without password - keep existing password if any
// Clear token fields when switching to password auth
pbs.TokenName = ""
pbs.TokenValue = ""
} else if req.User != "" {
// User provided - assume password auth but keep existing password
// Ensure user has realm for PBS
pbsUser := req.User
if !strings.Contains(req.User, "@") {
pbsUser = req.User + "@pbs" // Default to @pbs realm if not specified
}
pbs.User = pbsUser
// Clear token fields when using password auth
pbs.TokenName = ""
pbs.TokenValue = ""
}
// else: No authentication changes - preserve existing auth fields
pbs.Fingerprint = req.Fingerprint
pbs.VerifySSL = req.VerifySSL
@ -1575,12 +1591,16 @@ func (h *ConfigHandlers) HandleUpdateNode(w http.ResponseWriter, r *http.Request
pmgInst.Host = host
}
// Handle authentication updates - only switch auth method if explicitly provided
if req.TokenName != "" && req.TokenValue != "" {
pmgInst.User = ""
pmgInst.Password = ""
// Switching to token authentication
pmgInst.TokenName = req.TokenName
pmgInst.TokenValue = req.TokenValue
} else {
// Clear user/password when switching to token auth
pmgInst.User = ""
pmgInst.Password = ""
} else if req.Password != "" {
// Switching to password authentication
if req.User != "" {
user := req.User
if !strings.Contains(user, "@") {
@ -1588,14 +1608,22 @@ func (h *ConfigHandlers) HandleUpdateNode(w http.ResponseWriter, r *http.Request
}
pmgInst.User = user
}
if req.Password != "" {
pmgInst.Password = req.Password
}
if req.TokenName == "" && req.TokenValue == "" {
pmgInst.TokenName = ""
pmgInst.TokenValue = ""
pmgInst.Password = req.Password
// Clear token fields when switching to password auth
pmgInst.TokenName = ""
pmgInst.TokenValue = ""
} else if req.User != "" {
// User provided - assume password auth but keep existing password
user := req.User
if !strings.Contains(user, "@") {
user = user + "@pmg"
}
pmgInst.User = user
// Clear token fields when using password auth
pmgInst.TokenName = ""
pmgInst.TokenValue = ""
}
// else: No authentication changes - preserve existing auth fields
pmgInst.Fingerprint = req.Fingerprint
pmgInst.VerifySSL = req.VerifySSL

View file

@ -145,8 +145,8 @@ type PVEInstance struct {
MonitorContainers bool
MonitorStorage bool
MonitorBackups bool
MonitorPhysicalDisks bool // Monitor physical disks (polled less frequently to avoid spinning up HDDs)
PhysicalDiskPollingMinutes int // How often to poll physical disks (0 = use default)
MonitorPhysicalDisks *bool // Monitor physical disks (nil = enabled by default, can be explicitly disabled)
PhysicalDiskPollingMinutes int // How often to poll physical disks (0 = use default)
// Cluster support
IsCluster bool // True if this is a cluster

View file

@ -2145,183 +2145,186 @@ func (m *Monitor) pollPVEInstance(ctx context.Context, instanceName string, clie
}
}
// Poll physical disks for health monitoring (only if enabled and interval elapsed)
if instanceCfg.MonitorPhysicalDisks {
// Poll physical disks for health monitoring (enabled by default unless explicitly disabled)
// Skip if MonitorPhysicalDisks is explicitly set to false
if instanceCfg.MonitorPhysicalDisks != nil && !*instanceCfg.MonitorPhysicalDisks {
log.Debug().Str("instance", instanceName).Msg("Physical disk monitoring explicitly disabled")
// Keep any existing disk data visible (don't clear it)
} else {
// Enabled by default (when nil or true)
// Determine polling interval (default 5 minutes to avoid spinning up HDDs too frequently)
pollingInterval := 5 * time.Minute
if instanceCfg.PhysicalDiskPollingMinutes > 0 {
pollingInterval = time.Duration(instanceCfg.PhysicalDiskPollingMinutes) * time.Minute
if instanceCfg.PhysicalDiskPollingMinutes > 0 {
pollingInterval = time.Duration(instanceCfg.PhysicalDiskPollingMinutes) * time.Minute
}
// Check if enough time has elapsed since last poll
m.mu.Lock()
lastPoll, exists := m.lastPhysicalDiskPoll[instanceName]
shouldPoll := !exists || time.Since(lastPoll) >= pollingInterval
if shouldPoll {
m.lastPhysicalDiskPoll[instanceName] = time.Now()
}
m.mu.Unlock()
if !shouldPoll {
log.Debug().
Str("instance", instanceName).
Dur("sinceLastPoll", time.Since(lastPoll)).
Dur("interval", pollingInterval).
Msg("Skipping physical disk poll - interval not elapsed")
// Refresh NVMe temperatures using the latest sensor data even when we skip the disk poll
currentState := m.state.GetSnapshot()
existing := make([]models.PhysicalDisk, 0)
for _, disk := range currentState.PhysicalDisks {
if disk.Instance == instanceName {
existing = append(existing, disk)
}
}
if len(existing) > 0 {
updated := mergeNVMeTempsIntoDisks(existing, modelNodes)
m.state.UpdatePhysicalDisks(instanceName, updated)
}
} else {
log.Debug().
Int("nodeCount", len(nodes)).
Dur("interval", pollingInterval).
Msg("Starting disk health polling")
// Get existing disks from state to preserve data for offline nodes
currentState := m.state.GetSnapshot()
existingDisksMap := make(map[string]models.PhysicalDisk)
for _, disk := range currentState.PhysicalDisks {
if disk.Instance == instanceName {
existingDisksMap[disk.ID] = disk
}
}
// Check if enough time has elapsed since last poll
m.mu.Lock()
lastPoll, exists := m.lastPhysicalDiskPoll[instanceName]
shouldPoll := !exists || time.Since(lastPoll) >= pollingInterval
if shouldPoll {
m.lastPhysicalDiskPoll[instanceName] = time.Now()
}
m.mu.Unlock()
var allDisks []models.PhysicalDisk
polledNodes := make(map[string]bool) // Track which nodes we successfully polled
for _, node := range nodes {
// Skip offline nodes but preserve their existing disk data
if node.Status != "online" {
log.Debug().Str("node", node.Node).Msg("Skipping disk poll for offline node - preserving existing data")
continue
}
// Get disk list for this node
log.Debug().Str("node", node.Node).Msg("Getting disk list for node")
disks, err := client.GetDisks(ctx, node.Node)
if err != nil {
// Check if it's a permission error or if the endpoint doesn't exist
if strings.Contains(err.Error(), "401") || strings.Contains(err.Error(), "403") {
log.Warn().
Str("node", node.Node).
Err(err).
Msg("Insufficient permissions to access disk information - check API token permissions")
} else if strings.Contains(err.Error(), "404") || strings.Contains(err.Error(), "501") {
log.Info().
Str("node", node.Node).
Msg("Disk monitoring not available on this node (may be using non-standard storage)")
} else {
log.Warn().
Str("node", node.Node).
Err(err).
Msg("Failed to get disk list")
}
continue
}
if !shouldPoll {
log.Debug().
Str("instance", instanceName).
Dur("sinceLastPoll", time.Since(lastPoll)).
Dur("interval", pollingInterval).
Msg("Skipping physical disk poll - interval not elapsed")
// Refresh NVMe temperatures using the latest sensor data even when we skip the disk poll
currentState := m.state.GetSnapshot()
existing := make([]models.PhysicalDisk, 0)
for _, disk := range currentState.PhysicalDisks {
if disk.Instance == instanceName {
existing = append(existing, disk)
}
}
if len(existing) > 0 {
updated := mergeNVMeTempsIntoDisks(existing, modelNodes)
m.state.UpdatePhysicalDisks(instanceName, updated)
}
} else {
log.Debug().
Int("nodeCount", len(nodes)).
Dur("interval", pollingInterval).
Msg("Starting disk health polling")
Str("node", node.Node).
Int("diskCount", len(disks)).
Msg("Got disk list for node")
// Get existing disks from state to preserve data for offline nodes
currentState := m.state.GetSnapshot()
existingDisksMap := make(map[string]models.PhysicalDisk)
for _, disk := range currentState.PhysicalDisks {
if disk.Instance == instanceName {
existingDisksMap[disk.ID] = disk
}
}
// Mark this node as successfully polled
polledNodes[node.Node] = true
var allDisks []models.PhysicalDisk
polledNodes := make(map[string]bool) // Track which nodes we successfully polled
for _, node := range nodes {
// Skip offline nodes but preserve their existing disk data
if node.Status != "online" {
log.Debug().Str("node", node.Node).Msg("Skipping disk poll for offline node - preserving existing data")
continue
// Check each disk for health issues and add to state
for _, disk := range disks {
// Create PhysicalDisk model
diskID := fmt.Sprintf("%s-%s-%s", instanceName, node.Node, strings.ReplaceAll(disk.DevPath, "/", "-"))
physicalDisk := models.PhysicalDisk{
ID: diskID,
Node: node.Node,
Instance: instanceName,
DevPath: disk.DevPath,
Model: disk.Model,
Serial: disk.Serial,
Type: disk.Type,
Size: disk.Size,
Health: disk.Health,
Wearout: disk.Wearout,
RPM: disk.RPM,
Used: disk.Used,
LastChecked: time.Now(),
}
// Get disk list for this node
log.Debug().Str("node", node.Node).Msg("Getting disk list for node")
disks, err := client.GetDisks(ctx, node.Node)
if err != nil {
// Check if it's a permission error or if the endpoint doesn't exist
if strings.Contains(err.Error(), "401") || strings.Contains(err.Error(), "403") {
log.Warn().
Str("node", node.Node).
Err(err).
Msg("Insufficient permissions to access disk information - check API token permissions")
} else if strings.Contains(err.Error(), "404") || strings.Contains(err.Error(), "501") {
log.Info().
Str("node", node.Node).
Msg("Disk monitoring not available on this node (may be using non-standard storage)")
} else {
log.Warn().
Str("node", node.Node).
Err(err).
Msg("Failed to get disk list")
}
continue
}
allDisks = append(allDisks, physicalDisk)
log.Debug().
Str("node", node.Node).
Int("diskCount", len(disks)).
Msg("Got disk list for node")
Str("disk", disk.DevPath).
Str("model", disk.Model).
Str("health", disk.Health).
Int("wearout", disk.Wearout).
Msg("Checking disk health")
// Mark this node as successfully polled
polledNodes[node.Node] = true
// Check each disk for health issues and add to state
for _, disk := range disks {
// Create PhysicalDisk model
diskID := fmt.Sprintf("%s-%s-%s", instanceName, node.Node, strings.ReplaceAll(disk.DevPath, "/", "-"))
physicalDisk := models.PhysicalDisk{
ID: diskID,
Node: node.Node,
Instance: instanceName,
DevPath: disk.DevPath,
Model: disk.Model,
Serial: disk.Serial,
Type: disk.Type,
Size: disk.Size,
Health: disk.Health,
Wearout: disk.Wearout,
RPM: disk.RPM,
Used: disk.Used,
LastChecked: time.Now(),
}
allDisks = append(allDisks, physicalDisk)
log.Debug().
normalizedHealth := strings.ToUpper(strings.TrimSpace(disk.Health))
if normalizedHealth != "" && normalizedHealth != "UNKNOWN" && normalizedHealth != "PASSED" && normalizedHealth != "OK" {
// Disk has failed or is failing - alert manager will handle this
log.Warn().
Str("node", node.Node).
Str("disk", disk.DevPath).
Str("model", disk.Model).
Str("health", disk.Health).
Int("wearout", disk.Wearout).
Msg("Checking disk health")
Msg("Disk health issue detected")
normalizedHealth := strings.ToUpper(strings.TrimSpace(disk.Health))
if normalizedHealth != "" && normalizedHealth != "UNKNOWN" && normalizedHealth != "PASSED" && normalizedHealth != "OK" {
// Disk has failed or is failing - alert manager will handle this
log.Warn().
Str("node", node.Node).
Str("disk", disk.DevPath).
Str("model", disk.Model).
Str("health", disk.Health).
Int("wearout", disk.Wearout).
Msg("Disk health issue detected")
// Pass disk info to alert manager
m.alertManager.CheckDiskHealth(instanceName, node.Node, disk)
} else if disk.Wearout > 0 && disk.Wearout < 10 {
// Low wearout warning (less than 10% life remaining)
log.Warn().
Str("node", node.Node).
Str("disk", disk.DevPath).
Str("model", disk.Model).
Int("wearout", disk.Wearout).
Msg("SSD wearout critical - less than 10% life remaining")
// Pass disk info to alert manager
m.alertManager.CheckDiskHealth(instanceName, node.Node, disk)
} else if disk.Wearout > 0 && disk.Wearout < 10 {
// Low wearout warning (less than 10% life remaining)
log.Warn().
Str("node", node.Node).
Str("disk", disk.DevPath).
Str("model", disk.Model).
Int("wearout", disk.Wearout).
Msg("SSD wearout critical - less than 10% life remaining")
// Pass to alert manager for wearout alert
m.alertManager.CheckDiskHealth(instanceName, node.Node, disk)
}
// Pass to alert manager for wearout alert
m.alertManager.CheckDiskHealth(instanceName, node.Node, disk)
}
}
// Preserve existing disk data for nodes that weren't polled (offline or error)
for _, existingDisk := range existingDisksMap {
// Only preserve if we didn't poll this node
if !polledNodes[existingDisk.Node] {
// Keep the existing disk data but update the LastChecked to indicate it's stale
allDisks = append(allDisks, existingDisk)
log.Debug().
Str("node", existingDisk.Node).
Str("disk", existingDisk.DevPath).
Msg("Preserving existing disk data for unpolled node")
}
}
allDisks = mergeNVMeTempsIntoDisks(allDisks, modelNodes)
// Update physical disks in state
log.Debug().
Str("instance", instanceName).
Int("diskCount", len(allDisks)).
Int("preservedCount", len(existingDisksMap)-len(polledNodes)).
Msg("Updating physical disks in state")
m.state.UpdatePhysicalDisks(instanceName, allDisks)
}
} else {
// Physical disk monitoring is disabled - clear any existing disk data for this instance
log.Debug().Str("instance", instanceName).Msg("Physical disk monitoring disabled - clearing disk data")
m.state.UpdatePhysicalDisks(instanceName, []models.PhysicalDisk{})
// Preserve existing disk data for nodes that weren't polled (offline or error)
for _, existingDisk := range existingDisksMap {
// Only preserve if we didn't poll this node
if !polledNodes[existingDisk.Node] {
// Keep the existing disk data but update the LastChecked to indicate it's stale
allDisks = append(allDisks, existingDisk)
log.Debug().
Str("node", existingDisk.Node).
Str("disk", existingDisk.DevPath).
Msg("Preserving existing disk data for unpolled node")
}
}
allDisks = mergeNVMeTempsIntoDisks(allDisks, modelNodes)
// Update physical disks in state
log.Debug().
Str("instance", instanceName).
Int("diskCount", len(allDisks)).
Int("preservedCount", len(existingDisksMap)-len(polledNodes)).
Msg("Updating physical disks in state")
m.state.UpdatePhysicalDisks(instanceName, allDisks)
}
}
// Note: Physical disk monitoring is now enabled by default with a 5-minute polling interval.
// Users can explicitly disable it in node settings. Disk data is preserved between polls.
// Update nodes with storage fallback if rootfs was not available
for i := range modelNodes {