Fix adm upgrade status timeouts: Optimize MachineConfig fetches and enforce client deadlines

pkg/cli/admin/upgrade/status/status.go

@@ -55,7 +55,9 @@ func New(f kcmdutil.Factory, streams genericiooptions.IOStreams) *cobra.Command
 		Short: "Display the status of the current cluster version update or multi-arch migration",
 		Run: func(cmd *cobra.Command, args []string) {
 			kcmdutil.CheckErr(o.Complete(f, cmd, args))
-			kcmdutil.CheckErr(o.Run(cmd.Context()))
+			ctx, cancel := context.WithTimeout(cmd.Context(), 5*time.Minute)
+			defer cancel()
+			kcmdutil.CheckErr(o.Run(ctx))
 		},
 	}
 
@@ -109,6 +111,7 @@ func (o *options) Complete(f kcmdutil.Factory, cmd *cobra.Command, args []string
 			return err
 		}
 		cfg.UserAgent = rest.DefaultKubernetesUserAgent() + "(upgrade-status)"
+		cfg.Timeout = 60 * time.Second
 
 		roundTripper, err := rest.TransportFor(cfg)
 		if err != nil {
@@ -230,23 +233,13 @@ func (o *options) Run(ctx context.Context) error {
 	}
 	var machineConfigs *machineconfigv1.MachineConfigList
 	if machineConfigs = o.mockData.machineConfigs; machineConfigs == nil {
-		machineConfigs = &machineconfigv1.MachineConfigList{}
-		for _, node := range allNodes.Items {
-			for _, key := range []string{mco.CurrentMachineConfigAnnotationKey, mco.DesiredMachineConfigAnnotationKey} {
-				machineConfigName, ok := node.Annotations[key]
-				if !ok || machineConfigName == "" {
-					continue
-				}
-				mc, err := getMachineConfig(ctx, o.MachineConfigClient, machineConfigs.Items, machineConfigName)
-				if err != nil {
-					return err
-				}
-				if mc != nil {
-					machineConfigs.Items = append(machineConfigs.Items, *mc)
-				}
-			}
+		var err error
+		machineConfigs, err = o.MachineConfigClient.MachineconfigurationV1().MachineConfigs().List(ctx, metav1.ListOptions{})
+		if err != nil {
+			return fmt.Errorf("listing MachineConfigs: %w", err)
 		}
 	}
+	machineConfigVersionMap := buildMachineConfigVersionMap(machineConfigs.Items)
 
 	// The cluster stays within the same version number when it is migrated to multi-arch
 	// and thus the comparison of version numbers in a normal cluster upgrade does not work.
@@ -298,7 +291,7 @@ func (o *options) Run(ctx context.Context) error {
 	// but it cannot be opted out at the moment.
 	multiArchMigrationInProgress := multiArchMigration(cv.Status.History) && progressing.Status == configv1.ConditionTrue
 	for _, pool := range pools.Items {
-		nodesStatusData, insights := assessNodesStatus(cv, pool, nodesPerPool[pool.Name], machineConfigs.Items, multiArchMigrationInProgress)
+		nodesStatusData, insights := assessNodesStatus(cv, pool, nodesPerPool[pool.Name], machineConfigVersionMap, multiArchMigrationInProgress)
 		updateInsights = append(updateInsights, insights...)
 		poolStatus, insights := assessMachineConfigPool(pool, nodesStatusData)
 		updateInsights = append(updateInsights, insights...)

pkg/cli/admin/upgrade/status/workerpool.go

@@ -1,7 +1,6 @@
 package status
 
 import (
-	"context"
 	"fmt"
 	"io"
 	"sort"
@@ -10,12 +9,10 @@ import (
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
-	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 
 	configv1 "github.com/openshift/api/config/v1"
 	mcfgv1 "github.com/openshift/api/machineconfiguration/v1"
-	mcfgv1client "github.com/openshift/client-go/machineconfiguration/clientset/versioned"
 	"github.com/openshift/oc/pkg/cli/admin/upgrade/status/mco"
 )
 
@@ -114,15 +111,6 @@ type poolDisplayData struct {
 	Nodes         []nodeDisplayData
 }
 
-func getMachineConfig(ctx context.Context, client mcfgv1client.Interface, machineConfigs []mcfgv1.MachineConfig, machineConfigName string) (*mcfgv1.MachineConfig, error) {
-	for _, mc := range machineConfigs {
-		if mc.Name == machineConfigName {
-			return nil, nil
-		}
-	}
-	return client.MachineconfigurationV1().MachineConfigs().Get(ctx, machineConfigName, v1.GetOptions{})
-}
-
 func whichPool(master, worker labels.Selector, custom map[string]labels.Selector, node corev1.Node) string {
 	if master.Matches(labels.Set(node.Labels)) {
 		return "master"
@@ -146,13 +134,13 @@ func ellipsizeNames(message string, name string) string {
 	return strings.Replace(message, name, "<node>", -1)
 }
 
-func assessNodesStatus(cv *configv1.ClusterVersion, pool mcfgv1.MachineConfigPool, nodes []corev1.Node, machineConfigs []mcfgv1.MachineConfig, multiArchMigrationInProgress bool) ([]nodeDisplayData, []updateInsight) {
+func assessNodesStatus(cv *configv1.ClusterVersion, pool mcfgv1.MachineConfigPool, nodes []corev1.Node, machineConfigVersionMap map[string]string, multiArchMigrationInProgress bool) ([]nodeDisplayData, []updateInsight) {
 	var nodesStatusData []nodeDisplayData
 	var insights []updateInsight
 	for _, node := range nodes {
 		desiredConfig, ok := node.Annotations[mco.DesiredMachineConfigAnnotationKey]
-		currentVersion, foundCurrent := getOpenShiftVersionOfMachineConfig(machineConfigs, node.Annotations[mco.CurrentMachineConfigAnnotationKey])
-		desiredVersion, foundDesired := getOpenShiftVersionOfMachineConfig(machineConfigs, desiredConfig)
+		currentVersion, foundCurrent := getOpenShiftVersionOfMachineConfig(machineConfigVersionMap, node.Annotations[mco.CurrentMachineConfigAnnotationKey])
+		desiredVersion, foundDesired := getOpenShiftVersionOfMachineConfig(machineConfigVersionMap, desiredConfig)
 
 		lns := mco.NewLayeredNodeState(&node)
 		isUnavailable := lns.IsUnavailable(&pool)
@@ -245,14 +233,19 @@ func assessNodesStatus(cv *configv1.ClusterVersion, pool mcfgv1.MachineConfigPoo
 	return nodesStatusData, insights
 }
 
-func getOpenShiftVersionOfMachineConfig(machineConfigs []mcfgv1.MachineConfig, name string) (string, bool) {
+func buildMachineConfigVersionMap(machineConfigs []mcfgv1.MachineConfig) map[string]string {
+	versionMap := make(map[string]string, len(machineConfigs))
 	for _, mc := range machineConfigs {
-		if mc.Name == name {
-			openshiftVersion := mc.Annotations[mco.ReleaseImageVersionAnnotationKey]
-			return openshiftVersion, openshiftVersion != ""
+		if version := mc.Annotations[mco.ReleaseImageVersionAnnotationKey]; version != "" {
+			versionMap[mc.Name] = version
 		}
 	}
-	return "", false
+	return versionMap
+}
+
+func getOpenShiftVersionOfMachineConfig(versionMap map[string]string, name string) (string, bool) {
+	version, found := versionMap[name]
+	return version, found
 }
 
 func isNodeDraining(node corev1.Node, isUpdating bool) bool {

pkg/cli/admin/upgrade/status/workerpool_test.go

@@ -836,7 +836,8 @@ func Test_assessNodesStatus(t *testing.T) {
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			nodeDisplayData, updateInsight := assessNodesStatus(tc.args.cv, tc.args.pool, tc.args.nodes, tc.args.machineConfigs, false)
+			machineConfigVersionMap := buildMachineConfigVersionMap(tc.args.machineConfigs)
+			nodeDisplayData, updateInsight := assessNodesStatus(tc.args.cv, tc.args.pool, tc.args.nodes, machineConfigVersionMap, false)
 			if diff := cmp.Diff(tc.expectedNodeDisplayData, nodeDisplayData, allowUnexportedWorkerPools); diff != "" {
 				t.Errorf("nodeDisplayData differ from expected:\n%s", diff)
 			}
@@ -951,7 +952,8 @@ func Test_assessNodesStatus_DisplayData_Sorting(t *testing.T) {
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			nodeDisplayData, _ := assessNodesStatus(tc.args.cv, tc.args.pool, tc.args.nodes, tc.args.machineConfigs, false)
+			machineConfigVersionMap := buildMachineConfigVersionMap(tc.args.machineConfigs)
+			nodeDisplayData, _ := assessNodesStatus(tc.args.cv, tc.args.pool, tc.args.nodes, machineConfigVersionMap, false)
 			if diff := cmp.Diff(tc.expectedNodeDisplayData, nodeDisplayData, allowUnexportedWorkerPools); diff != "" {
 				t.Errorf("nodeDisplayData differ from expected:\n%s", diff)
 			}