From 79cdca26a1dc5f773c13be4d86cefeb8e6297c9b Mon Sep 17 00:00:00 2001 From: Jack Ottofaro Date: Fri, 17 Jun 2022 11:42:28 -0400 Subject: [PATCH] pkg/cvo: retain initial completed update history entry --- pkg/cvo/status.go | 25 ++++++++------- pkg/cvo/status_test.go | 71 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 84 insertions(+), 12 deletions(-) diff --git a/pkg/cvo/status.go b/pkg/cvo/status.go index 0fdb83fdae..0f3684cb68 100644 --- a/pkg/cvo/status.go +++ b/pkg/cvo/status.go @@ -29,6 +29,10 @@ const ( // cannot reach the desired state. It is considered more serious than Degraded // and indicates the cluster is not healthy. ClusterStatusFailing = configv1.ClusterStatusConditionType("Failing") + + // MaxHistory is the maximum size of ClusterVersion history. Once exceeded + // ClusterVersion history will be pruned. + MaxHistory = 50 ) func mergeEqualVersions(current *configv1.UpdateHistory, desired configv1.Release) bool { @@ -128,26 +132,25 @@ func mergeOperatorHistory(config *configv1.ClusterVersion, desired configv1.Rele } // TODO: prune Z versions over transitions to Y versions, keep initial installed version - pruneStatusHistory(config, 50) + pruneStatusHistory(config, MaxHistory) config.Status.Desired = desired } +// pruneStatusHistory maintains history size at MaxHistory by removing entry at index MaxHistory +// unless that entry is a completed update in which case entry at MaxHistory-1 is removed thereby +// retaining the initial completed version. func pruneStatusHistory(config *configv1.ClusterVersion, maxHistory int) { if len(config.Status.History) <= maxHistory { return } - for i, item := range config.Status.History { - if item.State != configv1.CompletedUpdate { - continue - } - // guarantee the last position in the history is always a completed item - if i >= maxHistory { - config.Status.History[maxHistory-1] = item - } - break + if config.Status.History[maxHistory].State == configv1.CompletedUpdate { + item := config.Status.History[maxHistory] + config.Status.History = config.Status.History[0 : maxHistory-1] + config.Status.History = append(config.Status.History, item) + } else { + config.Status.History = config.Status.History[:maxHistory] } - config.Status.History = config.Status.History[:maxHistory] } // ClusterVersionInvalid indicates that the cluster version has an error that prevents the server from diff --git a/pkg/cvo/status_test.go b/pkg/cvo/status_test.go index e0b57bd915..1eb0308f2f 100644 --- a/pkg/cvo/status_test.go +++ b/pkg/cvo/status_test.go @@ -5,6 +5,7 @@ import ( "fmt" "reflect" "testing" + "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/diff" @@ -80,14 +81,16 @@ func Test_pruneStatusHistory(t *testing.T) { want []configv1.UpdateHistory }{ { + name: "max history 2", config: obj.DeepCopy(), maxHistory: 2, want: []configv1.UpdateHistory{ {State: configv1.PartialUpdate, Version: "0.0.10"}, - {State: configv1.CompletedUpdate, Version: "0.0.7"}, + {State: configv1.PartialUpdate, Version: "0.0.9"}, }, }, { + name: "max history 3", config: obj.DeepCopy(), maxHistory: 3, want: []configv1.UpdateHistory{ @@ -97,6 +100,7 @@ func Test_pruneStatusHistory(t *testing.T) { }, }, { + name: "max history 4", config: obj.DeepCopy(), maxHistory: 4, want: []configv1.UpdateHistory{ @@ -107,6 +111,7 @@ func Test_pruneStatusHistory(t *testing.T) { }, }, { + name: "max history 5", config: obj.DeepCopy(), maxHistory: 5, want: []configv1.UpdateHistory{ @@ -118,6 +123,7 @@ func Test_pruneStatusHistory(t *testing.T) { }, }, { + name: "max history 6", config: obj.DeepCopy(), maxHistory: 6, want: []configv1.UpdateHistory{ @@ -134,12 +140,75 @@ func Test_pruneStatusHistory(t *testing.T) { config := tt.config.DeepCopy() pruneStatusHistory(config, tt.maxHistory) if !reflect.DeepEqual(tt.want, config.Status.History) { + t.Logf("%v", config.Status.History) t.Fatalf("%s", diff.ObjectReflectDiff(tt.want, config.Status.History)) } }) } } +func Test_mergeOperatorHistory(t *testing.T) { + obj := &configv1.ClusterVersion{} + var nowT time.Time + now := metav1.NewTime(nowT) + tests := []struct { + name string + config *configv1.ClusterVersion + first configv1.UpdateHistory + second configv1.UpdateHistory + want configv1.UpdateHistory + }{ + { + name: "drop partial", + config: obj.DeepCopy(), + first: configv1.UpdateHistory{State: configv1.PartialUpdate, CompletionTime: &now, Version: "0.0.7", Image: "test:0"}, + second: configv1.UpdateHistory{State: configv1.CompletedUpdate, CompletionTime: &now, Version: "0.0.6", Image: "test:1"}, + want: configv1.UpdateHistory{State: configv1.CompletedUpdate, CompletionTime: &now, Version: "0.0.6", Image: "test:1"}, + }, + { + name: "keep completed", + config: obj.DeepCopy(), + first: configv1.UpdateHistory{State: configv1.CompletedUpdate, CompletionTime: &now, Version: "0.0.7", Image: "test:0"}, + second: configv1.UpdateHistory{State: configv1.PartialUpdate, CompletionTime: &now, Version: "0.0.6", Image: "test:1"}, + want: configv1.UpdateHistory{State: configv1.CompletedUpdate, CompletionTime: &now, Version: "0.0.7", Image: "test:0"}, + }, + { + name: "neither completed", + config: obj.DeepCopy(), + first: configv1.UpdateHistory{State: configv1.PartialUpdate, CompletionTime: &now, Version: "0.0.7", Image: "test:0"}, + second: configv1.UpdateHistory{State: configv1.PartialUpdate, CompletionTime: &now, Version: "0.0.6", Image: "test:1"}, + want: configv1.UpdateHistory{State: configv1.PartialUpdate, CompletionTime: &now, Version: "0.0.6", Image: "test:1"}, + }, + { + name: "keep first completed", + config: obj.DeepCopy(), + first: configv1.UpdateHistory{State: configv1.CompletedUpdate, CompletionTime: &now, Version: "0.0.7", Image: "test:0"}, + second: configv1.UpdateHistory{State: configv1.CompletedUpdate, CompletionTime: &now, Version: "0.0.6", Image: "test:1"}, + want: configv1.UpdateHistory{State: configv1.CompletedUpdate, CompletionTime: &now, Version: "0.0.7", Image: "test:0"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for i := 0; i <= MaxHistory; i++ { + image := fmt.Sprintf("test:%d", i) + if i == 0 { + mergeOperatorHistory(tt.config, configv1.Release{Image: image, Version: tt.first.Version}, false, now, + (tt.first.State == configv1.CompletedUpdate)) + } else if i == 1 { + mergeOperatorHistory(tt.config, configv1.Release{Image: image, Version: tt.second.Version}, false, now, + (tt.second.State == configv1.CompletedUpdate)) + } else { + version := fmt.Sprintf("%d", i) + mergeOperatorHistory(tt.config, configv1.Release{Image: image, Version: version}, false, now, true) + } + } + if !reflect.DeepEqual(tt.want, tt.config.Status.History[MaxHistory-1]) { + t.Fatalf("%s", diff.ObjectReflectDiff(tt.want, tt.config.Status.History[MaxHistory-1])) + } + }) + } +} + func TestOperator_syncFailingStatus(t *testing.T) { ctx := context.Background() tests := []struct {