Merge pull request #28299 from dgoodwin/node-pod-monitor-intervals

Port Node and Pod State to Structured Intervals

Author: openshift-ci[bot]

pkg/monitor/monitorapi/construction.go

@@ -62,8 +62,22 @@ func (b *IntervalBuilder) Build(from, to time.Time) Interval {
 	return ret
 }
 
+// BuildNow creates the final interval with a from/to timestamp of now.
+func (b *IntervalBuilder) BuildNow() Interval {
+	now := time.Now()
+	ret := Interval{
+		Condition: b.BuildCondition(),
+		Display:   b.display,
+		Source:    b.source,
+		From:      now,
+		To:        now,
+	}
+
+	return ret
+}
+
 func (b *IntervalBuilder) Message(mb *MessageBuilder) *IntervalBuilder {
-	b.structuredMessage = mb.build()
+	b.structuredMessage = mb.Build()
 	return b
 }
 
@@ -417,8 +431,8 @@ func (m *MessageBuilder) HumanMessagef(messageFormat string, args ...interface{}
 	return m.HumanMessage(fmt.Sprintf(messageFormat, args...))
 }
 
-// build creates the final StructuredMessage with all data assembled by this builder.
-func (m *MessageBuilder) build() Message {
+// Build creates the final StructuredMessage with all data assembled by this builder.
+func (m *MessageBuilder) Build() Message {
 	ret := Message{
 		Annotations: map[AnnotationKey]string{},
 	}

pkg/monitor/monitorapi/identification_pod.go

@@ -13,10 +13,6 @@ func LocatePod(pod *corev1.Pod) string {
 	return fmt.Sprintf("ns/%s pod/%s node/%s uid/%s", pod.Namespace, pod.Name, pod.Spec.NodeName, pod.UID)
 }
 
-func LocatePodContainer(pod *corev1.Pod, containerName string) string {
-	return fmt.Sprintf("ns/%s pod/%s node/%s uid/%s container/%s", pod.Namespace, pod.Name, pod.Spec.NodeName, pod.UID, containerName)
-}
-
 // NonUniquePodLocatorFrom produces an inexact locator based on namespace and name.  This is useful when dealing with events
 // that are produced that do not contain UIDs.  Ultimately, we should use UIDs everywhere, but this is will keep some our
 // matching working until then.
@@ -26,6 +22,7 @@ func NonUniquePodLocatorFrom(locator string) string {
 	return fmt.Sprintf("ns/%s pod/%s", namespace, parts["pod"])
 }
 
+// TODO:  all callers should eventuall be using structured locator variant below:
 func PodFrom(locator string) PodReference {
 	parts := LocatorParts(locator)
 	namespace := NamespaceFrom(parts)
@@ -43,10 +40,27 @@ func PodFrom(locator string) PodReference {
 	}
 }
 
-func ContainerFrom(locator string) ContainerReference {
-	pod := PodFrom(locator)
-	parts := LocatorParts(locator)
-	name := parts["container"]
+// PodFromLocator is used to strip down a locator to just a pod. (as it may contain additional keys like container or node)
+// that we do not want for some uses.
+func PodFromLocator(locator Locator) PodReference {
+	namespace := locator.Keys[LocatorNamespaceKey]
+	name := locator.Keys[LocatorPodKey]
+	uid := locator.Keys[LocatorUIDKey]
+	if len(namespace) == 0 || len(name) == 0 {
+		return PodReference{}
+	}
+	return PodReference{
+		NamespacedReference: NamespacedReference{
+			Namespace: namespace,
+			Name:      name,
+			UID:       uid,
+		},
+	}
+}
+
+func ContainerFrom(locator Locator) ContainerReference {
+	pod := PodFrom(locator.OldLocator())
+	name := locator.Keys[LocatorContainerKey]
 	if len(name) == 0 || len(pod.UID) == 0 {
 		return ContainerReference{}
 	}
@@ -61,17 +75,17 @@ type PodReference struct {
 	NamespacedReference
 }
 
-func (r PodReference) ToLocator() string {
-	return fmt.Sprintf("ns/%s pod/%s uid/%s", r.Namespace, r.Name, r.UID)
+func (r PodReference) ToLocator() Locator {
+	return NewLocator().PodFromNames(r.Namespace, r.Name, r.UID)
 }
 
 type ContainerReference struct {
 	Pod           PodReference
 	ContainerName string
 }
 
-func (r ContainerReference) ToLocator() string {
-	return fmt.Sprintf("ns/%s pod/%s uid/%s container/%s", r.Pod.Namespace, r.Pod.Name, r.Pod.UID, r.ContainerName)
+func (r ContainerReference) ToLocator() Locator {
+	return NewLocator().ContainerFromNames(r.Pod.Namespace, r.Pod.Name, r.Pod.UID, r.ContainerName)
 }
 
 func AnnotationsFromMessage(message string) map[AnnotationKey]string {

pkg/monitor/monitorapi/types.go

@@ -165,6 +165,9 @@ const (
 	PodReasonForceDelete           IntervalReason = "ForceDelete"
 	PodReasonDeleted               IntervalReason = "Deleted"
 	PodReasonScheduled             IntervalReason = "Scheduled"
+	PodReasonEvicted               IntervalReason = "Evicted"
+	PodReasonPreempted             IntervalReason = "Preempted"
+	PodReasonFailed                IntervalReason = "Failed"
 
 	ContainerReasonContainerExit      IntervalReason = "ContainerExit"
 	ContainerReasonContainerStart     IntervalReason = "ContainerStart"
@@ -184,6 +187,9 @@ const (
 	NodeNotReadyReason IntervalReason = "NotReady"
 	NodeFailedLease    IntervalReason = "FailedToUpdateLease"
 
+	MachineConfigChangeReason  IntervalReason = "MachineConfigChange"
+	MachineConfigReachedReason IntervalReason = "MachineConfigReached"
+
 	Timeout IntervalReason = "Timeout"
 
 	E2ETestStarted  IntervalReason = "E2ETestStarted"
@@ -196,6 +202,7 @@ const (
 	AnnotationReason             AnnotationKey = "reason"
 	AnnotationContainerExitCode  AnnotationKey = "code"
 	AnnotationCause              AnnotationKey = "cause"
+	AnnotationConfig             AnnotationKey = "config"
 	AnnotationNode               AnnotationKey = "node"
 	AnnotationEtcdLocalMember    AnnotationKey = "local-member-id"
 	AnnotationEtcdTerm           AnnotationKey = "term"
@@ -207,10 +214,18 @@ const (
 	// TODO this looks wrong. seems like it ought to be set in the to/from
 	AnnotationDuration       AnnotationKey = "duration"
 	AnnotationRequestAuditID AnnotationKey = "request-audit-id"
+	AnnotationRoles          AnnotationKey = "roles"
 	AnnotationStatus         AnnotationKey = "status"
 	AnnotationCondition      AnnotationKey = "condition"
 )
 
+// ConstructionOwner was originally meant to signify that an interval was derived from other intervals.
+// This allowed for the possibility of testing interval generation by feeding in only source intervals,
+// and checking what was generated.
+// TODO: likely want to drop this concept in favor of Source, plus a flag automatically applied to any
+// intervals coming back from the monitor test call to generate calculated intervals. Source
+// will replace the use of what constructed the interval, and the flag will allow us to see what is derived
+// and what isn't.
 type ConstructionOwner string
 
 const (
@@ -229,6 +244,9 @@ type Message struct {
 	Annotations map[AnnotationKey]string `json:"annotations"`
 }
 
+// IntervalSource is used to type/categorize all intervals based on what created them.
+// This is intended to be used to group, and when combined with the display flag, signal that
+// they should be visible by default in the UIs that render interval charts.
 type IntervalSource string
 
 const (
@@ -238,13 +256,16 @@ const (
 	SourceE2ETest                 IntervalSource = "E2ETest"
 	SourceNetworkManagerLog       IntervalSource = "NetworkMangerLog"
 	SourceNodeMonitor             IntervalSource = "NodeMonitor"
+	SourceSystemJournalScanner    IntervalSource = "KubeletLogScanner"
 	SourcePodLog                  IntervalSource = "PodLog"
 	SourcePodMonitor              IntervalSource = "PodMonitor"
 	SourceKubeEvent               IntervalSource = "KubeEvent"
 	SourceTestData                IntervalSource = "TestData"                // some tests have no real source to assign
 	SourcePathologicalEventMarker IntervalSource = "PathologicalEventMarker" // not sure if this is really helpful since the events all have a different origin
 	SourceClusterOperatorMonitor  IntervalSource = "ClusterOperatorMonitor"
 	SourceOperatorState           IntervalSource = "OperatorState"
+	SourceNodeState                              = "NodeState"
+	SourcePodState                               = "PodState"
 )
 
 type Interval struct {
@@ -303,25 +324,73 @@ func (i Message) OldMessage() string {
 }
 
 func (i Locator) OldLocator() string {
-	keys := sets.NewString()
+	keys := []string{}
 	for k := range i.Keys {
-		keys.Insert(string(k))
+		keys = append(keys, string(k))
 	}
 
+	keys = sortKeys(keys)
+
 	annotations := []string{}
-	for _, k := range keys.List() {
+	for _, k := range keys {
 		v := i.Keys[LocatorKey(k)]
 		if LocatorKey(k) == LocatorE2ETestKey {
 			annotations = append(annotations, fmt.Sprintf("%v/%q", k, v))
 		} else {
 			annotations = append(annotations, fmt.Sprintf("%v/%v", k, v))
 		}
 	}
+
 	annotationString := strings.Join(annotations, " ")
 
 	return annotationString
 }
 
+// sortKeys ensures that some keys appear in the order we require (least specific to most), so rows with locators
+// are grouped together. (i.e. keeping containers within the same pod together, or rows for a specific container)
+// Blindly going through the keys results in alphabetical ordering, container comes first, and then we've
+// got container events separated from their pod events on the intervals chart.
+// This will hopefully eventually go away but for now we need it.
+// Courtesy of ChatGPT but unit tested.
+func sortKeys(keys []string) []string {
+
+	// Ensure these keys appear in this order. Other keys can be mixed in and will appear at the end in alphabetical
+	// order.
+	orderedKeys := []string{"namespace", "node", "pod", "uid", "server", "container", "shutdown"}
+
+	// Create a map to store the indices of keys in the orderedKeys array.
+	// This will allow us to efficiently check if a key is in orderedKeys and find its position.
+	orderedKeyIndices := make(map[string]int)
+
+	for i, key := range orderedKeys {
+		orderedKeyIndices[key] = i
+	}
+
+	// Define a custom sorting function that orders the keys based on the orderedKeys array.
+	sort.Slice(keys, func(i, j int) bool {
+		// Get the indices of keys i and j in orderedKeys.
+		indexI, existsI := orderedKeyIndices[keys[i]]
+		indexJ, existsJ := orderedKeyIndices[keys[j]]
+
+		// If both keys exist in orderedKeys, sort them based on their order.
+		if existsI && existsJ {
+			return indexI < indexJ
+		}
+
+		// If only one of the keys exists in orderedKeys, move it to the front.
+		if existsI {
+			return true
+		} else if existsJ {
+			return false
+		}
+
+		// If neither key is in orderedKeys, sort alphabetically so we have predictable ordering
+		return keys[i] < keys[j]
+	})
+
+	return keys
+}
+
 type IntervalFilter func(i Interval) bool
 
 type IntervalFilters []IntervalFilter

pkg/monitor/monitorapi/types_test.go

@@ -3,6 +3,8 @@ package monitorapi
 import (
 	"testing"
 	"time"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestIntervals_Duration(t *testing.T) {
@@ -106,3 +108,45 @@ func TestIntervals_Duration(t *testing.T) {
 		})
 	}
 }
+
+//	Not sure this test needs to live forever, but while working through the move
+//
+// to structured locators, it would be best if the legacy one kept coming out with
+// keys in the same order they were before, as the intervals chart sorts on these and
+// some are expected to be grouped together.
+func TestLocatorOldLocator(t *testing.T) {
+	tests := []struct {
+		name     string
+		locator  Locator
+		expected string // the legacy locator
+	}{
+		{
+			name:     "container locator",
+			locator:  NewLocator().ContainerFromNames("mynamespace", "mypod", "fakeuid", "mycontainer"),
+			expected: "namespace/mynamespace pod/mypod uid/fakeuid container/mycontainer",
+		},
+		{
+			name:     "pod locator",
+			locator:  NewLocator().PodFromNames("mynamespace", "mypod", "fakeuid"),
+			expected: "namespace/mynamespace pod/mypod uid/fakeuid",
+		},
+		{
+			name: "container locator with keys mixed in", // not sure if this can happen but make sure what we expect occurs
+			locator: Locator{Keys: map[LocatorKey]string{
+				"a":         "b",
+				"container": "mycontainer",
+				"foo":       "bar",
+				"namespace": "mynamespace",
+				"pod":       "mypod",
+				"uid":       "fakeuid",
+				"zzz":       "foobar",
+			}},
+			expected: "namespace/mynamespace pod/mypod uid/fakeuid container/mycontainer a/b foo/bar zzz/foobar",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, tt.locator.OldLocator())
+		})
+	}
+}

pkg/monitor/serialization/serialize.go

@@ -66,9 +66,11 @@ func EventsFromJSON(data []byte) (monitorapi.Intervals, error) {
 		events = append(events, monitorapi.Interval{
 			Source: monitorapi.IntervalSource(interval.Source),
 			Condition: monitorapi.Condition{
-				Level:   level,
-				Locator: interval.Locator,
-				Message: interval.Message,
+				Level:             level,
+				Locator:           interval.Locator,
+				Message:           interval.Message,
+				StructuredLocator: interval.StructuredLocator,
+				StructuredMessage: interval.StructuredMessage,
 			},
 
 			From: interval.From.Time,
@@ -176,8 +178,16 @@ func (intervals byTime) Less(i, j int) bool {
 	case d > 0:
 		return false
 	}
-	return intervals[i].Locator < intervals[j].Locator
+
+	switch d := intervals[i].To.Sub(intervals[j].To.Time); {
+	case d < 0:
+		return true
+	case d > 0:
+		return false
+	}
+	return intervals[i].Message < intervals[j].Message
 }
+
 func (intervals byTime) Len() int { return len(intervals) }
 func (intervals byTime) Swap(i, j int) {
 	intervals[i], intervals[j] = intervals[j], intervals[i]