roachprod/vm: fix JSON serialization of VM.Errors field

The VM.Errors field was typed as []error, but Go's encoding/json cannot
unmarshal into an error interface type because it doesn't know what
concrete type to create. This caused roachprod list to fail when loading
cached cluster info containing VMs with errors:

  json: cannot unmarshal object into Go struct field
  VM.vms.errors of type error

This change introduces a VMError wrapper type that:
- Implements the error interface
- Implements json.Marshaler to serialize as a JSON string (error message)
- Implements json.Unmarshaler to handle both the new format (strings) and
  legacy format (empty objects from broken serialization)
- Implements Unwrap() so errors.Is/As still work

All cloud providers (AWS, Azure, GCE, IBM) are updated to use the new
VMError type via NewVMError(). The BadInstanceErrors() function now
returns map[string]vm.List (grouped by error message string) instead of
map[error]vm.List since serialization loses pointer identity.

Release note: None
Epic: None

Co-authored-by: Claude <[email protected]>

Author: petermattis

pkg/cmd/roachprod/cli/BUILD.bazel

@@ -30,7 +30,6 @@ go_library(
         "//pkg/roachprod/logger",
         "//pkg/roachprod/roachprodutil",
         "//pkg/roachprod/ssh",
-        "//pkg/roachprod/ui",
         "//pkg/roachprod/vm",
         "//pkg/roachprod/vm/gce",
         "//pkg/util/envutil",

pkg/cmd/roachprod/cli/commands.go

@@ -26,7 +26,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachprod/config"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/roachprodutil"
-	"github.com/cockroachdb/cockroach/pkg/roachprod/ui"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/vm"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/vm/gce"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -547,15 +546,15 @@ hosts file.
 				if listDetails {
 					collated := filteredCloud.BadInstanceErrors()
 
-					// Sort by Error() value for stable output
-					var errors ui.ErrorsByError
-					for err := range collated {
-						errors = append(errors, err)
+					// Sort by error message for stable output
+					var errMsgs []string
+					for msg := range collated {
+						errMsgs = append(errMsgs, msg)
 					}
-					sort.Sort(errors)
+					sort.Strings(errMsgs)
 
-					for _, e := range errors {
-						fmt.Printf("%s: %s\n", e, collated[e].Names())
+					for _, msg := range errMsgs {
+						fmt.Printf("%s: %s\n", msg, collated[msg].Names())
 					}
 				}
 			}

pkg/roachprod/cloud/cluster_cloud.go

@@ -60,14 +60,17 @@ type Cloud struct {
 	BadInstances vm.List `json:"bad_instances"`
 }
 
-// BadInstanceErrors returns all bad VM instances, grouped by error.
-func (c *Cloud) BadInstanceErrors() map[error]vm.List {
-	ret := map[error]vm.List{}
+// BadInstanceErrors returns all bad VM instances, grouped by error message.
+// Note: errors are grouped by their string representation to handle cases
+// where errors have been serialized/deserialized and lost pointer identity.
+func (c *Cloud) BadInstanceErrors() map[string]vm.List {
+	ret := map[string]vm.List{}
 
 	// Expand instances and errors
-	for _, vm := range c.BadInstances {
-		for _, err := range vm.Errors {
-			ret[err] = append(ret[err], vm)
+	for _, v := range c.BadInstances {
+		for _, err := range v.Errors {
+			msg := err.Error()
+			ret[msg] = append(ret[msg], v)
 		}
 	}
 
@@ -263,11 +266,11 @@ func ListCloud(l *logger.Logger, options vm.ListOptions) (*Cloud, error) {
 			// Parse cluster/user from VM name, but only for non-local VMs
 			userName, err := v.UserName()
 			if err != nil {
-				v.Errors = append(v.Errors, vm.ErrInvalidUserName)
+				v.Errors = append(v.Errors, vm.NewVMError(vm.ErrInvalidUserName))
 			}
 			clusterName, err := v.ClusterName()
 			if err != nil {
-				v.Errors = append(v.Errors, vm.ErrInvalidClusterName)
+				v.Errors = append(v.Errors, vm.NewVMError(vm.ErrInvalidClusterName))
 			}
 
 			// Anything with an error gets tossed into the BadInstances slice, and we'll correct

pkg/roachprod/vm/aws/aws.go

@@ -1141,20 +1141,20 @@ func (in *DescribeInstancesOutputInstance) toVM(
 	// Convert the tag map into a more useful representation
 	tagMap := in.Tags.MakeMap()
 
-	var errs []error
+	var errs []vm.VMError
 	createdAt, err := time.Parse(time.RFC3339, in.LaunchTime)
 	if err != nil {
-		errs = append(errs, vm.ErrNoExpiration)
+		errs = append(errs, vm.NewVMError(vm.ErrNoExpiration))
 	}
 
 	var lifetime time.Duration
 	if lifeText, ok := tagMap[vm.TagLifetime]; ok {
 		lifetime, err = time.ParseDuration(lifeText)
 		if err != nil {
-			errs = append(errs, err)
+			errs = append(errs, vm.NewVMError(err))
 		}
 	} else {
-		errs = append(errs, vm.ErrNoExpiration)
+		errs = append(errs, vm.NewVMError(vm.ErrNoExpiration))
 	}
 
 	var nonBootableVolumes []vm.Volume
@@ -1165,11 +1165,11 @@ func (in *DescribeInstancesOutputInstance) toVM(
 				if vol, ok := volumes[bdm.Disk.VolumeID]; ok {
 					nonBootableVolumes = append(nonBootableVolumes, vol)
 				} else {
-					errs = append(errs, errors.Newf(
+					errs = append(errs, vm.NewVMError(errors.Newf(
 						"Attempted to add volume %s however it is not in the attached volumes for instance %s",
 						bdm.Disk.VolumeID,
 						in.InstanceID,
-					))
+					)))
 				}
 			}
 		}

pkg/roachprod/vm/azure/azure.go

@@ -604,19 +604,19 @@ func (p *Provider) computeVirtualMachineToVM(cvm compute.VirtualMachine) (*vm.VM
 	}
 
 	if createdPtr := cvm.Tags[vm.TagCreated]; createdPtr == nil {
-		m.Errors = append(m.Errors, vm.ErrNoExpiration)
+		m.Errors = append(m.Errors, vm.NewVMError(vm.ErrNoExpiration))
 	} else if parsed, err := time.Parse(time.RFC3339, *createdPtr); err == nil {
 		m.CreatedAt = parsed
 	} else {
-		m.Errors = append(m.Errors, vm.ErrNoExpiration)
+		m.Errors = append(m.Errors, vm.NewVMError(vm.ErrNoExpiration))
 	}
 
 	if lifetimePtr := cvm.Tags[vm.TagLifetime]; lifetimePtr == nil {
-		m.Errors = append(m.Errors, vm.ErrNoExpiration)
+		m.Errors = append(m.Errors, vm.NewVMError(vm.ErrNoExpiration))
 	} else if parsed, err := time.ParseDuration(*lifetimePtr); err == nil {
 		m.Lifetime = parsed
 	} else {
-		m.Errors = append(m.Errors, vm.ErrNoExpiration)
+		m.Errors = append(m.Errors, vm.NewVMError(vm.ErrNoExpiration))
 	}
 
 	// The network info needs a separate request.
@@ -625,7 +625,7 @@ func (p *Provider) computeVirtualMachineToVM(cvm compute.VirtualMachine) (*vm.VM
 		return nil, err
 	}
 	if err := p.fillNetworkDetails(context.Background(), m, nicID); errors.Is(err, vm.ErrBadNetwork) {
-		m.Errors = append(m.Errors, err)
+		m.Errors = append(m.Errors, vm.NewVMError(err))
 	} else if err != nil {
 		return nil, err
 	}

pkg/roachprod/vm/gce/gcloud.go

@@ -209,27 +209,27 @@ type jsonVM struct {
 
 // Convert the JSON VM data into our common VM type.
 func (jsonVM *jsonVM) toVM(project string, dnsDomain string) (ret *vm.VM) {
-	var vmErrors []error
+	var vmErrors []vm.VMError
 	var err error
 
 	// Check "lifetime" label.
 	var lifetime time.Duration
 	if lifetimeStr, ok := jsonVM.Labels[vm.TagLifetime]; ok {
 		if lifetime, err = time.ParseDuration(lifetimeStr); err != nil {
-			vmErrors = append(vmErrors, vm.ErrNoExpiration)
+			vmErrors = append(vmErrors, vm.NewVMError(vm.ErrNoExpiration))
 		}
 	} else {
-		vmErrors = append(vmErrors, vm.ErrNoExpiration)
+		vmErrors = append(vmErrors, vm.NewVMError(vm.ErrNoExpiration))
 	}
 
 	// Extract network information
 	var publicIP, privateIP, vpc string
 	if len(jsonVM.NetworkInterfaces) == 0 {
-		vmErrors = append(vmErrors, vm.ErrBadNetwork)
+		vmErrors = append(vmErrors, vm.NewVMError(vm.ErrBadNetwork))
 	} else {
 		privateIP = jsonVM.NetworkInterfaces[0].NetworkIP
 		if len(jsonVM.NetworkInterfaces[0].AccessConfigs) == 0 {
-			vmErrors = append(vmErrors, vm.ErrBadNetwork)
+			vmErrors = append(vmErrors, vm.NewVMError(vm.ErrBadNetwork))
 		} else {
 			_ = jsonVM.NetworkInterfaces[0].AccessConfigs[0].Name // silence unused warning
 			publicIP = jsonVM.NetworkInterfaces[0].AccessConfigs[0].NatIP
@@ -238,7 +238,7 @@ func (jsonVM *jsonVM) toVM(project string, dnsDomain string) (ret *vm.VM) {
 	}
 	if jsonVM.Scheduling.OnHostMaintenance == "" {
 		// N.B. 'onHostMaintenance' is always non-empty, hence its absense implies a parsing error
-		vmErrors = append(vmErrors, vm.ErrBadScheduling)
+		vmErrors = append(vmErrors, vm.NewVMError(vm.ErrBadScheduling))
 	}
 
 	machineType := lastComponent(jsonVM.MachineType)
@@ -261,7 +261,7 @@ func (jsonVM *jsonVM) toVM(project string, dnsDomain string) (ret *vm.VM) {
 
 		vol, volType, err := jsonVMDisk.toVolume()
 		if err != nil {
-			vmErrors = append(vmErrors, err)
+			vmErrors = append(vmErrors, vm.NewVMError(err))
 			continue
 		}
 
@@ -3115,7 +3115,7 @@ func (j *managedInstanceGroupInstance) toVM(
 ) *vm.VM {
 
 	var err error
-	var vmErrors []error
+	var vmErrors []vm.VMError
 
 	remoteUser := config.SharedUser
 	if !config.UseSharedUser {
@@ -3130,10 +3130,10 @@ func (j *managedInstanceGroupInstance) toVM(
 	var lifetime time.Duration
 	if lifetimeStr, ok := instanceTemplate.Properties.Labels[vm.TagLifetime]; ok {
 		if lifetime, err = time.ParseDuration(lifetimeStr); err != nil {
-			vmErrors = append(vmErrors, vm.ErrNoExpiration)
+			vmErrors = append(vmErrors, vm.NewVMError(vm.ErrNoExpiration))
 		}
 	} else {
-		vmErrors = append(vmErrors, vm.ErrNoExpiration)
+		vmErrors = append(vmErrors, vm.NewVMError(vm.ErrNoExpiration))
 	}
 
 	var arch vm.CPUArch
@@ -3148,7 +3148,7 @@ func (j *managedInstanceGroupInstance) toVM(
 	for _, disk := range instanceTemplate.Properties.Disks {
 		vol, volType, err := disk.toVolume(j.Name, zone)
 		if err != nil {
-			vmErrors = append(vmErrors, err)
+			vmErrors = append(vmErrors, vm.NewVMError(err))
 			continue
 		}
 

pkg/roachprod/vm/ibm/ibm_extended_types.go

@@ -548,45 +548,45 @@ func (i *instance) toVM() vm.VM {
 
 	if i == nil || i.instance == nil {
 		return vm.VM{
-			Errors: []error{errors.New("instance is nil")},
+			Errors: []vm.VMError{vm.NewVMError(errors.New("instance is nil"))},
 		}
 	}
 
 	if !i.initialized {
 		err := i.load()
 		if err != nil {
 			return vm.VM{
-				Errors: []error{errors.Wrap(err, "failed to load instance")},
+				Errors: []vm.VMError{vm.NewVMError(errors.Wrap(err, "failed to load instance"))},
 			}
 		}
 	}
 
-	var vmErrors []error
+	var vmErrors []vm.VMError
 
 	vpcID := ""
 	region, err := i.provider.zoneToRegion(*i.instance.Zone.Name)
 	if err != nil {
-		vmErrors = append(vmErrors, errors.Wrap(err, "unable to get region"))
+		vmErrors = append(vmErrors, vm.NewVMError(errors.Wrap(err, "unable to get region")))
 	} else {
 		vpcID = i.provider.config.regions[region].vpcID
 	}
 
 	// Check if the instance is in a valid state.
 	if core.StringNilMapper(i.instance.Status) == "failed" {
-		vmErrors = append(vmErrors, errors.New("instance is in failed state"))
+		vmErrors = append(vmErrors, vm.NewVMError(errors.New("instance is in failed state")))
 	}
 
 	// Gather tags
 	tags, err := i.getTagsAsMap()
 	if err != nil {
-		vmErrors = append(vmErrors, errors.Wrap(err, "unable to get tags"))
+		vmErrors = append(vmErrors, vm.NewVMError(errors.Wrap(err, "unable to get tags")))
 	}
 
 	var lifetime time.Duration
 	if lifeText, ok := tags[vm.TagLifetime]; ok {
 		lifetime, err = time.ParseDuration(lifeText)
 		if err != nil {
-			vmErrors = append(vmErrors, errors.Wrap(err, "unable to compute lifetime"))
+			vmErrors = append(vmErrors, vm.NewVMError(errors.Wrap(err, "unable to compute lifetime")))
 		}
 	} else {
 		// Missing lifetime tag, use the default lifetime.
@@ -598,7 +598,7 @@ func (i *instance) toVM() vm.VM {
 	privateIP := i.getPrivateIPAddress()
 	publicIP, err := i.getPublicIPAddress()
 	if err != nil {
-		vmErrors = append(vmErrors, errors.Wrap(err, "unable to get public IP"))
+		vmErrors = append(vmErrors, vm.NewVMError(errors.Wrap(err, "unable to get public IP")))
 	}
 
 	nonBootAttachedVolumes := []vm.Volume{}

pkg/roachprod/vm/vm.go

@@ -6,6 +6,7 @@
 package vm
 
 import (
+	"encoding/json"
 	"fmt"
 	"regexp"
 	"sort"
@@ -105,7 +106,7 @@ type VM struct {
 	CreatedAt time.Time `json:"created_at"`
 	// If non-empty, indicates that some or all of the data in the VM instance
 	// is not present or otherwise invalid.
-	Errors      []error           `json:"errors"`
+	Errors      []VMError         `json:"errors"`
 	Lifetime    time.Duration     `json:"lifetime"`
 	Preemptible bool              `json:"preemptible"`
 	Labels      map[string]string `json:"labels"`
@@ -171,7 +172,7 @@ func Name(cluster string, idx int) string {
 	return fmt.Sprintf("%s-%0.4d", cluster, idx)
 }
 
-// Error values for VM.Error
+// Error values for VM.Errors
 var (
 	ErrBadNetwork         = errors.New("could not determine network information")
 	ErrBadScheduling      = errors.New("could not determine scheduling information")
@@ -180,6 +181,72 @@ var (
 	ErrNoExpiration       = errors.New("could not determine expiration")
 )
 
+// VMError wraps an error and implements json.Marshaler/Unmarshaler so that
+// errors can be properly serialized to and from JSON. This is necessary because
+// Go's encoding/json cannot unmarshal into an error interface type.
+type VMError struct {
+	err error
+}
+
+// NewVMError creates a new VMError from an error.
+func NewVMError(err error) VMError {
+	return VMError{err: err}
+}
+
+// Error implements the error interface.
+func (e VMError) Error() string {
+	if e.err == nil {
+		return ""
+	}
+	return e.err.Error()
+}
+
+// Unwrap returns the underlying error for use with errors.Is/As.
+func (e VMError) Unwrap() error {
+	return e.err
+}
+
+// MarshalJSON implements json.Marshaler.
+func (e VMError) MarshalJSON() ([]byte, error) {
+	if e.err == nil {
+		return []byte(`""`), nil
+	}
+	return json.Marshal(e.err.Error())
+}
+
+// UnmarshalJSON implements json.Unmarshaler.
+// It handles both the new format (JSON string) and the legacy format (JSON object)
+// that was produced when []error was serialized directly. The legacy format
+// typically produces empty objects `{}` since error types don't have exported fields.
+func (e *VMError) UnmarshalJSON(data []byte) error {
+	// Try to unmarshal as a string first (new format)
+	var msg string
+	if err := json.Unmarshal(data, &msg); err == nil {
+		if msg == "" {
+			e.err = nil
+		} else {
+			// Use Newf with %s format to satisfy the fmtsafe linter which requires
+			// constant format strings.
+			e.err = errors.Newf("%s", msg)
+		}
+		return nil
+	}
+
+	// If that fails, try to unmarshal as an object (legacy format).
+	// The legacy format serialized error interfaces as objects, but since
+	// errors.errorString has no exported fields, they serialize as empty `{}`.
+	// We treat any object as a nil/unknown error to maintain backwards compatibility.
+	var obj map[string]any
+	if err := json.Unmarshal(data, &obj); err == nil {
+		// Legacy format - treat as nil error (the error message was lost in serialization)
+		e.err = nil
+		return nil
+	}
+
+	// If neither worked, return an error
+	return errors.Newf("cannot unmarshal VMError from: %s", string(data))
+}
+
 var regionRE = regexp.MustCompile(`(.*[^-])-?[0-9a-z]$`)
 
 // IsLocal returns true if the VM represents the local host.