From e081e9a1c5efd14257f3396299fbc7c88f374f4c Mon Sep 17 00:00:00 2001 From: Yaroslav Borbat <86148689+yaroslavborbat@users.noreply.github.com> Date: Mon, 25 Nov 2024 11:13:23 +0300 Subject: [PATCH] fix(vm): fix generating wrong statistic (#414) fix generating wrong statistic --------- Signed-off-by: yaroslavborbat --- .../pkg/controller/vm/internal/statistic.go | 117 +++++-------- .../controller/vm/internal/statistic_test.go | 162 +----------------- 2 files changed, 41 insertions(+), 238 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/statistic.go b/images/virtualization-artifact/pkg/controller/vm/internal/statistic.go index 77890b11e..ebe80466b 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/statistic.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/statistic.go @@ -19,7 +19,6 @@ package internal import ( "context" "math" - "sort" "strconv" "time" @@ -200,44 +199,55 @@ func (h *StatisticHandler) syncStats(current, changed *virtv2.VirtualMachine, kv if current == nil || changed == nil { return } + phaseChanged := current.Status.Phase != changed.Status.Phase + var stats virtv2.VirtualMachineStats + if current.Status.Stats != nil { stats = *current.Status.Stats.DeepCopy() } pts := NewPhaseTransitions(stats.PhasesTransitions, current.Status.Phase, changed.Status.Phase) - pts.Sort() - stats.PhasesTransitions = pts.Items - launchTimeDuration := stats.LaunchTimeDuration + stats.PhasesTransitions = pts - var emptyOSInfo virtv1.VirtualMachineInstanceGuestOSInfo + launchTimeDuration := stats.LaunchTimeDuration switch changed.Status.Phase { - case virtv2.MachinePending: + case virtv2.MachinePending, virtv2.MachineStopped: launchTimeDuration.WaitingForDependencies = nil launchTimeDuration.VirtualMachineStarting = nil launchTimeDuration.GuestOSAgentStarting = nil case virtv2.MachineStarting: launchTimeDuration.VirtualMachineStarting = nil launchTimeDuration.GuestOSAgentStarting = nil + + if phaseChanged { + for i := len(pts) - 1; i > 0; i-- { + pt := pts[i] + ptPrev := pts[i-1] + if pt.Phase == virtv2.MachineStarting && ptPrev.Phase == virtv2.MachinePending { + launchTimeDuration.WaitingForDependencies = &metav1.Duration{Duration: pt.Timestamp.Sub(pts[i-1].Timestamp.Time)} + break + } + } + } case virtv2.MachineRunning: - if kvvmi != nil && emptyOSInfo == current.Status.GuestOSInfo { + if kvvmi != nil && osInfoIsEmpty(kvvmi.Status.GuestOSInfo) { launchTimeDuration.GuestOSAgentStarting = nil } - } - for i, pt := range pts.Items { - switch pt.Phase { - case virtv2.MachineStarting: - if i > 0 && pts.Items[i-1].Phase == phasePreviousPhase[pt.Phase] { - launchTimeDuration.WaitingForDependencies = &metav1.Duration{Duration: pt.Timestamp.Sub(pts.Items[i-1].Timestamp.Time)} - } - case virtv2.MachineRunning: - if i > 0 && pts.Items[i-1].Phase == phasePreviousPhase[pt.Phase] { - launchTimeDuration.VirtualMachineStarting = &metav1.Duration{Duration: pt.Timestamp.Sub(pts.Items[i-1].Timestamp.Time)} - } - if kvvmi != nil && emptyOSInfo == current.Status.GuestOSInfo && emptyOSInfo != kvvmi.Status.GuestOSInfo && !pt.Timestamp.IsZero() { - launchTimeDuration.GuestOSAgentStarting = &metav1.Duration{Duration: time.Now().Truncate(time.Second).Sub(pt.Timestamp.Time)} + for i := len(pts) - 1; i > 0; i-- { + pt := pts[i] + ptPrev := pts[i-1] + + if pt.Phase == virtv2.MachineRunning { + if phaseChanged && ptPrev.Phase == virtv2.MachineStarting { + launchTimeDuration.VirtualMachineStarting = &metav1.Duration{Duration: pt.Timestamp.Sub(pts[i-1].Timestamp.Time)} + } + if kvvmi != nil && osInfoIsEmpty(current.Status.GuestOSInfo) && !osInfoIsEmpty(kvvmi.Status.GuestOSInfo) && !pt.Timestamp.IsZero() { + launchTimeDuration.GuestOSAgentStarting = &metav1.Duration{Duration: time.Now().Truncate(time.Second).Sub(pt.Timestamp.Time)} + } + break } } } @@ -246,69 +256,22 @@ func (h *StatisticHandler) syncStats(current, changed *virtv2.VirtualMachine, kv changed.Status.Stats = &stats } -var phasePreviousPhase = map[virtv2.MachinePhase]virtv2.MachinePhase{ - virtv2.MachineRunning: virtv2.MachineStarting, - virtv2.MachineStarting: virtv2.MachinePending, - virtv2.MachineStopped: virtv2.MachineStopping, -} - -type PhaseTransitions struct { - Items []virtv2.VirtualMachinePhaseTransitionTimestamp +func osInfoIsEmpty(info virtv1.VirtualMachineInstanceGuestOSInfo) bool { + var emptyOSInfo virtv1.VirtualMachineInstanceGuestOSInfo + return emptyOSInfo == info } -func NewPhaseTransitions(phaseTransitions []virtv2.VirtualMachinePhaseTransitionTimestamp, oldPhase, newPhase virtv2.MachinePhase) PhaseTransitions { +func NewPhaseTransitions(phaseTransitions []virtv2.VirtualMachinePhaseTransitionTimestamp, oldPhase, newPhase virtv2.MachinePhase) []virtv2.VirtualMachinePhaseTransitionTimestamp { now := metav1.NewTime(time.Now().Truncate(time.Second)) - phasesTransitionsMap := make(map[virtv2.MachinePhase]virtv2.VirtualMachinePhaseTransitionTimestamp, len(phaseTransitions)) - for _, pt := range phaseTransitions { - phasesTransitionsMap[pt.Phase] = pt - } - if _, found := phasesTransitionsMap[newPhase]; !found || oldPhase != newPhase { - phasesTransitionsMap[newPhase] = virtv2.VirtualMachinePhaseTransitionTimestamp{ + if oldPhase != newPhase { + phaseTransitions = append(phaseTransitions, virtv2.VirtualMachinePhaseTransitionTimestamp{ Phase: newPhase, Timestamp: now, - } - } - p := newPhase - t := now.Add(-1 * time.Second) - // Since we are setting up phases based on kvvm, we may skip some of them. - // But we need to know some timestamps to generate statistics. - // Add the missing phases. - for { - if previousPhase, found := phasePreviousPhase[p]; found { - if _, found = phasesTransitionsMap[previousPhase]; !found { - phasesTransitionsMap[previousPhase] = virtv2.VirtualMachinePhaseTransitionTimestamp{ - Phase: previousPhase, - Timestamp: metav1.NewTime(t), - } - t = t.Add(-1 * time.Second) - } - p = previousPhase - continue - } - break + }) } - phasesTransitionsSlice := make([]virtv2.VirtualMachinePhaseTransitionTimestamp, len(phasesTransitionsMap)) - i := 0 - for _, p := range phasesTransitionsMap { - phasesTransitionsSlice[i] = p - i++ + if len(phaseTransitions) > 5 { + return phaseTransitions[len(phaseTransitions)-5:] } - return PhaseTransitions{Items: phasesTransitionsSlice} -} - -func (pt *PhaseTransitions) Sort() { - sort.Sort(pt) -} - -func (pt *PhaseTransitions) Len() int { - return len(pt.Items) -} - -func (pt *PhaseTransitions) Less(i, j int) bool { - return pt.Items[j].Timestamp.After(pt.Items[i].Timestamp.Time) -} - -func (pt *PhaseTransitions) Swap(i, j int) { - pt.Items[i], pt.Items[j] = pt.Items[j], pt.Items[i] + return phaseTransitions } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/statistic_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/statistic_test.go index 21a10575c..ac8617827 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/statistic_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/statistic_test.go @@ -20,7 +20,6 @@ import ( "context" "fmt" "testing" - "time" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -37,13 +36,6 @@ import ( virtv2 "github.com/deckhouse/virtualization/api/core/v1alpha2" ) -func oldTime() time.Time { - t, _ := time.Parse(time.RFC3339, "2006-01-02T15:04:05+07:00") - _, offset := time.Now().Zone() - diff := time.Duration(offset) * time.Second - return t.Add(-diff).In(time.Local) -} - func TestStatisticHandler(t *testing.T) { scheme := apiruntime.NewScheme() for _, f := range []func(*apiruntime.Scheme) error{ @@ -66,113 +58,7 @@ func TestStatisticHandler(t *testing.T) { expect func(vm *virtv2.VirtualMachine) error }{ { - name: "test1: virtualmachine running, if nil statistics", - getObjects: func() []client.Object { - return []client.Object{ - createPod(namespacedName), - createVM(namespacedName, virtv2.MachinePending, nil, virtv1.VirtualMachineInstanceGuestOSInfo{}), - createKVVMI(namespacedName, virtv1.Running, virtv1.VirtualMachineInstanceGuestOSInfo{Name: "test"}), - } - }, - mutateChanged: func(vm *virtv2.VirtualMachine) { - if vm != nil { - vm.Status.Phase = virtv2.MachineRunning - } - }, - expect: func(vm *virtv2.VirtualMachine) error { - if vm == nil || vm.Status.Stats == nil { - return fmt.Errorf("expected vm or stats to not be nil") - } - expectPhasesTransitions := []virtv2.MachinePhase{ - virtv2.MachinePending, - virtv2.MachineStarting, - virtv2.MachineRunning, - } - if err := checkPhasesTransitions(t, expectPhasesTransitions, vm); err != nil { - return err - } - require.NotNil(t, vm.Status.Stats) - require.NotNil(t, vm.Status.Stats.LaunchTimeDuration.WaitingForDependencies) - require.NotNil(t, vm.Status.Stats.LaunchTimeDuration.VirtualMachineStarting) - require.NotNil(t, vm.Status.Stats.LaunchTimeDuration.GuestOSAgentStarting) - return nil - }, - }, - { - name: "test2: virtualmachine running, statistic should no change", - getObjects: func() []client.Object { - info := virtv1.VirtualMachineInstanceGuestOSInfo{Name: "test"} - return []client.Object{ - createPod(namespacedName), - createVM(namespacedName, virtv2.MachineRunning, createStatisticNoChange(), info), - createKVVMI(namespacedName, virtv1.Running, info), - } - }, - mutateChanged: func(vm *virtv2.VirtualMachine) {}, - expect: func(vm *virtv2.VirtualMachine) error { - if vm == nil || vm.Status.Stats == nil { - return fmt.Errorf("expected vm or stats to not be nil") - } - stats := createStatisticNoChange() - require.Equal(t, stats.PhasesTransitions, vm.Status.Stats.PhasesTransitions) - require.Equal(t, stats.LaunchTimeDuration, vm.Status.Stats.LaunchTimeDuration) - return nil - }, - }, - { - name: "test3: .Stats.LaunchTimeDuration.WaitingForDependencies was should changed", - getObjects: func() []client.Object { - info := virtv1.VirtualMachineInstanceGuestOSInfo{} - return []client.Object{ - createPod(namespacedName), - createVM(namespacedName, - virtv2.MachinePending, - &virtv2.VirtualMachineStats{ - PhasesTransitions: []virtv2.VirtualMachinePhaseTransitionTimestamp{ - { - Phase: virtv2.MachinePending, - Timestamp: metav1.NewTime(oldTime()), - }, - { - Phase: virtv2.MachineStarting, - Timestamp: metav1.NewTime(oldTime().Add(10 * time.Second)), - }, - }, - LaunchTimeDuration: virtv2.VirtualMachineLaunchTimeDuration{ - WaitingForDependencies: &metav1.Duration{Duration: 10 * time.Second}, - }, - }, - info), - createKVVMI(namespacedName, virtv1.Scheduling, info), - } - }, - mutateChanged: func(vm *virtv2.VirtualMachine) { - if vm == nil { - return - } - vm.Status.Phase = virtv2.MachineStarting - }, - expect: func(vm *virtv2.VirtualMachine) error { - if vm == nil || vm.Status.Stats == nil { - return fmt.Errorf("expected vm or stats to not be nil") - } - expectPhasesTransitions := []virtv2.MachinePhase{ - virtv2.MachinePending, - virtv2.MachineStarting, - } - if err := checkPhasesTransitions(t, expectPhasesTransitions, vm); err != nil { - return err - } - require.NotNil(t, vm.Status.Stats) - wfd := vm.Status.Stats.LaunchTimeDuration.WaitingForDependencies - require.NotNil(t, wfd) - require.Greater(t, wfd.Duration, 10*time.Second) - - return nil - }, - }, - { - name: "test4: check generated .status.resources", + name: "test1: check generated .status.resources", getObjects: func() []client.Object { return []client.Object{ createPod(namespacedName), @@ -213,52 +99,6 @@ func TestStatisticHandler(t *testing.T) { } } -func checkPhasesTransitions(t *testing.T, expectPhasesTransitions []virtv2.MachinePhase, vm *virtv2.VirtualMachine) error { - if vm == nil || vm.Status.Stats == nil { - return fmt.Errorf("expected vm or stats to not be nil") - } - var pts []virtv2.MachinePhase - timestamp := oldTime().Add(-24 * time.Hour) - for _, pt := range vm.Status.Stats.PhasesTransitions { - if pt.Timestamp.After(timestamp) { - timestamp = pt.Timestamp.Time - } else { - return fmt.Errorf("wrong sort phases") - } - pts = append(pts, pt.Phase) - } - require.Equal(t, expectPhasesTransitions, pts) - return nil -} - -func createStatisticNoChange() *virtv2.VirtualMachineStats { - return &virtv2.VirtualMachineStats{ - PhasesTransitions: []virtv2.VirtualMachinePhaseTransitionTimestamp{ - { - Phase: virtv2.MachinePending, - Timestamp: metav1.Time{Time: oldTime()}, - }, - { - Phase: virtv2.MachineStarting, - Timestamp: metav1.Time{Time: oldTime().Add(1 * time.Second)}, - }, - { - Phase: virtv2.MachineRunning, - Timestamp: metav1.Time{Time: oldTime().Add(2 * time.Second)}, - }, - { - Phase: virtv2.MachineStopping, - Timestamp: metav1.Time{Time: oldTime().Add(3 * time.Second)}, - }, - }, - LaunchTimeDuration: virtv2.VirtualMachineLaunchTimeDuration{ - WaitingForDependencies: &metav1.Duration{Duration: 1 * time.Second}, - VirtualMachineStarting: &metav1.Duration{Duration: 1 * time.Second}, - GuestOSAgentStarting: &metav1.Duration{Duration: 1 * time.Second}, - }, - } -} - func createPod(vmKey types.NamespacedName) *corev1.Pod { return &corev1.Pod{ TypeMeta: metav1.TypeMeta{