Expose time range options for log collection in VerticaScrutinize CR #885

qindotguan · 2024-07-31T03:59:55Z

This PR defines 3 new parameters in the VerticaScrutinize CR which allow the collection of an arbitrary time range of logs:

logAgeHours: maximum age in hours of logs
logAgeOldestTime: maximum timestamp for logs
logAgeNewestTime: minimum timestamp for logs

Webhook added to validate:

logAgeHours cannot be set alongside logAgeOldestTime and logAgeNewestTime.
logAgeOldestTime should not be ahead of the current time.
logAgeOldestTime should be ahead of logAgeNewestTime.
logAgeOldestTime and logAgeNewestTime should be formatted as YYYY-MM-DD HH [+/-XX]

… arbitrary time range of logs

roypaulin · 2024-07-31T04:15:14Z

api/v1beta1/verticascrutinize_types.go

+	// +kubebuilder:validation:Optional
+	// +operator-sdk:csv:customresourcedefinitions:type=spec
+	// In order to facilitate diagnosing less recent problems, scrutinize should be able to collect an arbitrary time range of logs.
+	// With no options set, archived logs gathered for each node should be no older than 24 hours, and as recent as now.
+	// With the oldest time param or log age set, no archives of vertica.log should be older than that time.
+	// Timestamps should be formatted as: YYYY-MM-DD HH [+/-X], where [] is optional and +X represents X hours ahead of UTC.
+	LogAgeOldestTime string `json:"logAgeOldestTime,omitempty"`
+
+	// +kubebuilder:validation:Optional
+	// +operator-sdk:csv:customresourcedefinitions:type=spec
+	// In order to facilitate diagnosing less recent problems, scrutinize should be able to collect an arbitrary time range of logs.
+	// With no options set, archived logs gathered for each node should be no older than 24 hours, and as recent as now.
+	// With the newest time param set, no archives of vertica.log should be newer than that time.
+	// Timestamps should be formatted as: YYYY-MM-DD HH [+/-X], where [] is optional and +X represents X hours ahead of UTC.
+	LogAgeNewestTime string `json:"logAgeNewestTime,omitempty"`
+
+	// +kubebuilder:validation:Optional
+	// +operator-sdk:csv:customresourcedefinitions:type=spec
+	// In order to facilitate diagnosing less recent problems, scrutinize should be able to collect an arbitrary time range of logs.
+	// With no options set, archived logs gathered for each node should be no older than 24 hours, and as recent as now.
+	// The hours param cannot be set alongside the Time options, and if attempted, should issue an error indicating so.
+	LogAgeHours int `json:"logAgeHours,omitempty"`
+


Let's use annotations:

vertica.com/scrutinize-log-age-oldest-time

vertica.com/scrutinize-log-age-newest-time

vertica.com/scrutinize-log-age-hours

see package/meta/annotations.go

Thanks. Moved these parameters from spec to annotations.

roypaulin · 2024-07-31T04:15:44Z

api/v1beta1/verticascrutinize_types.go

+	// +operator-sdk:csv:customresourcedefinitions:type=status
+	// +optional
+	// minimum timestamp for logs (default 24 hours ago)
+	LogAgeOldestTime string `json:"logAgeOldestTime,omitempty"`
+
+	// +operator-sdk:csv:customresourcedefinitions:type=status
+	// +optional
+	// maximum timestamp for logs (default none)
+	LogAgeNewestTime string `json:"logAgeNewestTime,omitempty"`
+
+	// +operator-sdk:csv:customresourcedefinitions:type=status
+	// +optional
+	// maximum timestamp for logs (default none)
+	LogAgeHours int `json:"logAgeHours,omitempty"`
+


We don't need this.

roypaulin · 2024-07-31T04:38:10Z

api/v1beta1/helpers.go

@@ -118,6 +121,37 @@ func (vscr *VerticaScrutinize) CopyAnnotations() map[string]string {
 	return annotations
 }

+// GenerateLogAgeTime returns a string in the format of YYYY-MM-DD HH [+/-XX]
+func (vscr *VerticaScrutinize) GenerateLogAgeTime(hourOffset time.Duration, timeZone string) string {


This does not need to be exported and does not need a receiver. It can just be a simple function because it is only used for testing.

Removed the receiver, but it needs to be exported as it's called in pkg/controllers/vscr/scrutinizepod_reconciler_test.go.

roypaulin · 2024-07-31T04:43:45Z

api/v1beta1/helpers.go

+}
+
+// ParseLogAgeTime converts YYYY-MM-DD HH [+/-XX] into time format in UTC
+func (vscr *VerticaScrutinize) ParseLogAgeTime(logAgeTime string) time.Time {


This should return an error if time could not be parsed.

Also, this function doesn't need a receiver.

Thanks. Added error handling and moved it to api/v1beta1/verticascrutinize_webhook.go as a simple function.

roypaulin · 2024-07-31T04:52:47Z

api/v1beta1/verticascrutinize_webhook.go

+		logAgeOldestTime := twentyFourHoursAgo
+
+		if vscr.Spec.LogAgeOldestTime != "" {
+			logAgeOldestTime = vscr.ParseLogAgeTime(vscr.Spec.LogAgeOldestTime)


Add a test specifically for this function. You need to return value is as expected.

roypaulin · 2024-07-31T04:55:29Z

pkg/controllers/vscr/scrutinizepod_reconciler.go

+	if vscr.Spec.LogAgeHours != 0 && (vscr.Spec.LogAgeOldestTime != "" || vscr.Spec.LogAgeNewestTime != "") {
+		s.Log.Info("--log-age-hours cannot be set alongside the *-time options")
+		return cmd


Don't need this, there is already a webhook rule.

roypaulin · 2024-07-31T04:59:25Z

pkg/controllers/vscr/scrutinizepod_reconciler.go

+	if vscr.Spec.LogAgeHours != 0 {
+		cmd = append(cmd, "--log-age-hours", strconv.Itoa(vscr.Spec.LogAgeHours))
+	} else {
+		if vscr.Spec.LogAgeOldestTime != "" {
+			cmd = append(cmd, "--log-age-oldest-time", vscr.Spec.LogAgeOldestTime)
+		}
+		if vscr.Spec.LogAgeNewestTime != "" {
+			cmd = append(cmd, "--log-age-newest-time", vscr.Spec.LogAgeNewestTime)
+		}
+	}
+


If any of these flags was added in 24.3.0+, we should check that the server version is 24.3.0+ before adding them to the command.

Added supported version 24.2 in version.go and check in pkg/controllers/vscr/vdbverify_reconciler.go.

roypaulin · 2024-07-31T05:00:34Z

pkg/controllers/vscr/scrutinizepod_reconciler.go

@@ -164,6 +187,7 @@ func (s *ScrutinizeCmdArgs) buildScrutinizeCmdArgs(vdb *v1.VerticaDB) []string {
 		// container and have scrutinize read the password from the mounted file
 		cmd = append(cmd, "--password-file", paths.ScrutinizeDBPasswordFile)
 	}
+


Suggested change

roypaulin · 2024-07-31T05:03:59Z

pkg/controllers/vscr/scrutinizepod_reconciler.go

@@ -147,13 +149,34 @@ func (s *ScrutinizePodReconciler) getHostList(pods []corev1.Pod) []string {
 }

 // buildScrutinizeCmdArgs returns the arguments of vcluster scrutinize command
-func (s *ScrutinizeCmdArgs) buildScrutinizeCmdArgs(vdb *v1.VerticaDB) []string {
+func (s *ScrutinizePodReconciler) buildScrutinizeCmdArgs(vdb *v1.VerticaDB, vscr *v1beta1.VerticaScrutinize) []string {


You don't need to pass vscr. You can use s.Vscr

Thanks. Updated.

roypaulin · 2024-07-31T05:06:34Z

tests/e2e-leg-6/collect-scrutinize/30-create-vscr.yaml

Add a separate test case(folder) and leave collect-scrutinize unchanged.

A new test folder collect-scrutinize-log-age added for the log age related tests.

roypaulin

add 06-assert to the test
Any way to know the specified time range was applied from the collected files?

roypaulin · 2024-08-05T18:26:36Z

pkg/controllers/vscr/scrutinizepod_reconciler.go

+		cmd = append(cmd, "--log-age-hours", strconv.Itoa(vmeta.GetScrutinizeLogAgeHours(s.Vscr.Annotations)))
+	} else {
+		if vmeta.GetScrutinizeLogAgeOldestTime(s.Vscr.Annotations) != "" {
+			cmd = append(cmd, "--log-age-oldest-time", s.Vscr.Annotations[vmeta.ScrutinizeLogAgeOldestTime])


Suggested change

cmd = append(cmd, "--log-age-oldest-time", s.Vscr.Annotations[vmeta.ScrutinizeLogAgeOldestTime])

cmd = append(cmd, "--log-age-oldest-time", vmeta.GetScrutinizeLogAgeOldestTime(s.Vscr.Annotations))

We can manually rotate and check the time range of generated vertica.log files. But it's too long to wait because the minimum supported time range of these 3 annotations is 1 hour.

pkg/controllers/vscr/scrutinizepod_reconciler.go

roypaulin · 2024-08-06T20:25:01Z

Add a changie entry. Ask Cai what it means.

roypaulin · 2024-08-29T11:05:47Z

api/v1beta1/helpers.go

+	timeOffsetFormatted := timeOffset.Format("2006-01-02") + " " + strconv.Itoa(timeOffset.Hour())
+
+	if timeZone != "" {
+		timeOffsetFormatted = timeOffsetFormatted + " " + timeZone


Suggested change

timeOffsetFormatted = timeOffsetFormatted + " " + timeZone

timeOffsetFormatted = fmt.Sprintf("%s %s", timeOffsetFormatted, timeZone)

roypaulin · 2024-08-29T11:07:12Z

api/v1beta1/helpers.go

+// GenerateLogAgeTime returns a string in the format of YYYY-MM-DD HH [+/-XX]
+func GenerateLogAgeTime(hourOffset time.Duration, timeZone string) string {
+	timeOffset := time.Now().Add(hourOffset * time.Hour)
+	timeOffsetFormatted := timeOffset.Format("2006-01-02") + " " + strconv.Itoa(timeOffset.Hour())


Suggested change

timeOffsetFormatted := timeOffset.Format("2006-01-02") + " " + strconv.Itoa(timeOffset.Hour())

timeOffsetFormatted := fmt.Sprintf("%s %s", timeOffset.Format("2006-01-02"), strconv.Itoa(timeOffset.Hour()))

roypaulin · 2024-08-29T11:29:31Z

pkg/controllers/vscr/vdbverify_reconciler.go

+	if vinf.IsOlder(v1.ScrutinizeLogAgeVersion) {
+		ver, _ := s.Vdb.GetVerticaVersionStr()
+		s.VRec.Eventf(s.Vscr, corev1.EventTypeWarning, events.VclusterOpsScrutinizeNotSupported,
+			"The server version %s does not support scrutinize with a log time range. The minimum server version it supports is %s.",
+			ver, v1.ScrutinizeLogAgeVersion)
+		return s.updateStateAndScrutinizeReadyCondition(ctx, metav1.ConditionFalse,
+			events.VclusterOpsScrutinizeNotSupported, "NotReady:IncompatibleDB")
+	}
+


This block makes sense only when at least one of the logAge annotations is set. Otherwise we can skip this check.
Don't you think?

roypaulin · 2024-09-03T15:09:06Z

tests/e2e-leg-6/collect-scrutinize-log-age/40-wait-for-scrutinize.yaml

This step fails in the e2e tests. Have you tried it locally?

The test pass locally but failed in workflow due to insufficient cpu:
logger.go:42: 20:05:53 | collect-scrutinize-log-age | 2024-09-04 19:55:52 +0000 UTC Warning Pod vertica-scrutinize-log-time FailedScheduling 0/1 nodes are available: 1 Insufficient CPU.

Added 26-delete-vscr.yaml and 46-delete-vscr.yaml to remove old vscr to release resources.

roypaulin · 2024-09-05T16:10:59Z

Looks good! will approve once all tests pass

qindotguan added 7 commits July 30, 2024 23:29

add GenerateLogAgeTime and ParseLogAgeTime in helpers

9cdedf6

add LogAgeHours, LogAgeOldestTime, and LogAgeOldestTime to collect an…

7bfd040

… arbitrary time range of logs

add --log-age-hours and --log-age-*-time as arguments

4ae9728

add verticascrutinize webhook and test

5e3b00c

test logAgeHours is set in the arguments

5f50a07

test --log-age-*-time is set in the arguments

b5c3ace

no log age set if all three are not empty

60bce3c

qindotguan requested review from roypaulin and cchen-vertica as code owners July 31, 2024 03:59

qindotguan mentioned this pull request Jul 31, 2024

Rebased: expose time range options for log collection in VerticaScrutinize CR #876

Closed

roypaulin reviewed Jul 31, 2024

View reviewed changes

qindotguan added 14 commits August 4, 2024 06:36

move ParseLogAgeTime to verticascrutinize_webhook

cbdc286

add version check event for scrutinize with log time range

06db1fb

add scrutinize log time in annotations

9987490

support scrutinize log time range parameters in annotations

3283584

use annotations to validate scrutinize log time webhook

36ac134

add seperate test for scrutinize test with log time range

3d44501

restore scrutinize test in collect-scrutinize

3e851e2

test scrutinize log age hours

3a53fd7

move scrutinize log times test to collect-scrutinize-log-age

486f64c

move scrutinize log times negative test to collect-scrutinize-log-age

b7cbb88

move scrutinize log time range test to collect-scrutinize-log-age

91258a5

move scrutinize log time parameters to annotations

25ce3c5

update vscr webhook error message

b06658c

test log age time with annotations

f50490b

roypaulin reviewed Aug 5, 2024

View reviewed changes

qindotguan added 4 commits August 5, 2024 16:01

add webhook test to parse invalid times

4317e79

call get functions to retrieve times

b0bb098

add vscr webhook Mutating and Validating configurations

69df563

change the last test to test default values

e6a3983

add 06-assert.yaml back

7ec00b9

add change to expose log time range in vscr

e445e92

roypaulin reviewed Aug 29, 2024

View reviewed changes

qindotguan added 4 commits August 31, 2024 10:49

use fmt.Sprint to concatenate strings

df25fd1

check version only if lgoAge annotation is set

75bdb46

merge from main

d430ccc

remove duplicated vrpq.api/v1beta1/helpers.go

b10f8f7

roypaulin reviewed Sep 3, 2024

View reviewed changes

qindotguan added 2 commits September 4, 2024 13:45

Merge branch 'main' into qguan/vscr-expose-time-options

9b2a334

remove olds vscr to release resources

531bf81

roypaulin approved these changes Sep 6, 2024

View reviewed changes

qindotguan merged commit 7e73016 into main Sep 6, 2024
33 checks passed

qindotguan deleted the qguan/vscr-expose-time-options branch September 6, 2024 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose time range options for log collection in VerticaScrutinize CR #885

Expose time range options for log collection in VerticaScrutinize CR #885

qindotguan commented Jul 31, 2024

roypaulin Jul 31, 2024 •

edited

Loading

qindotguan Aug 5, 2024

roypaulin Jul 31, 2024

qindotguan Aug 5, 2024

roypaulin Jul 31, 2024

qindotguan Aug 5, 2024

roypaulin Jul 31, 2024

qindotguan Aug 5, 2024

roypaulin Jul 31, 2024

roypaulin Jul 31, 2024

qindotguan Aug 5, 2024

roypaulin Jul 31, 2024

qindotguan Aug 5, 2024

roypaulin Jul 31, 2024

roypaulin Jul 31, 2024

qindotguan Aug 5, 2024

roypaulin Jul 31, 2024

qindotguan Aug 5, 2024

roypaulin left a comment

roypaulin Aug 5, 2024

qindotguan Aug 5, 2024

roypaulin commented Aug 6, 2024

roypaulin Aug 29, 2024

roypaulin Aug 29, 2024

roypaulin Aug 29, 2024

roypaulin Sep 3, 2024

qindotguan Sep 5, 2024

roypaulin commented Sep 5, 2024

	cmd = append(cmd, "--log-age-oldest-time", s.Vscr.Annotations[vmeta.ScrutinizeLogAgeOldestTime])
	cmd = append(cmd, "--log-age-oldest-time", vmeta.GetScrutinizeLogAgeOldestTime(s.Vscr.Annotations))

	timeOffsetFormatted = timeOffsetFormatted + " " + timeZone
	timeOffsetFormatted = fmt.Sprintf("%s %s", timeOffsetFormatted, timeZone)

	timeOffsetFormatted := timeOffset.Format("2006-01-02") + " " + strconv.Itoa(timeOffset.Hour())
	timeOffsetFormatted := fmt.Sprintf("%s %s", timeOffset.Format("2006-01-02"), strconv.Itoa(timeOffset.Hour()))

Expose time range options for log collection in VerticaScrutinize CR #885

Expose time range options for log collection in VerticaScrutinize CR #885

Conversation

qindotguan commented Jul 31, 2024

roypaulin Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roypaulin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roypaulin commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roypaulin commented Sep 5, 2024

roypaulin Jul 31, 2024 •

edited

Loading