fix(test): configuration changes and fixes needed to scale-test #1085

alexcastilio · 2024-11-29T16:02:08Z

Description

Changes:

move env var name OUTPUT_FILEPATH to common/common.go
adjust timeout for diferent steps
enable operator in retina legacy helm chart installation
add retries to kube api requests to make it more robust to eventual failures in responses
add podStatus to metrics exported
add retina operator to get metrics from
other small fixes and improvements

Related Issue

If this pull request is related to any issue, please mention it here. Additionally, make sure that the issue is assigned to you before submitting this pull request.

Checklist

I have read the contributing documentation.
I signed and signed-off the commits (git commit -S -s ...). See this documentation on signing commits.
I have correctly attributed the author(s) of the code.
I have tested the changes locally.
I have followed the project's style guidelines.
I have updated the documentation, if necessary.
I have added tests, if applicable.

Screenshots (if applicable) or Testing Completed

Please add any relevant screenshots or GIFs to showcase the changes made.

Additional Notes

Add any additional notes or context about the pull request here.

Please refer to the CONTRIBUTING.md file for more information on how to contribute to this project.

github-actions · 2024-12-30T00:23:25Z

This PR will be closed in 7 days due to inactivity.

matmerr

looks like linter has some feedback though

timraymond · 2025-01-07T17:48:32Z

test/e2e/framework/kubernetes/delete-namespace.go

@@ -30,7 +34,7 @@ func (d *DeleteNamespace) Run() error {
 		return fmt.Errorf("error creating Kubernetes client: %w", err)
 	}

-	ctx, cancel := context.WithTimeout(context.Background(), defaultTimeoutSeconds*time.Second)
+	ctx, cancel := context.WithTimeout(context.Background(), deleteNamespaceTimeoutSeconds*time.Second)


Honestly, just inline this. The const makes this less readable (mute the linter if it complains). Also, (even though we're not doing this) the best practice with Go durations is const deleteNamespaceTimeout = 1200 * time.Second. Embedding a unit like Seconds in the name is an antipattern.

Suggested change

ctx, cancel := context.WithTimeout(context.Background(), deleteNamespaceTimeoutSeconds*time.Second)

ctx, cancel := context.WithTimeout(context.Background(), 1200 * time.Second)

timraymond · 2025-01-07T17:49:40Z

test/e2e/framework/kubernetes/delete-namespace.go

+	numberOfSteps := 9
+
 	backoff := wait.Backoff{
-		Steps:    6,
+		Steps:    numberOfSteps,


Same thing here, the constant just obscures what's going on more than it helps.

Steps: 9

timraymond · 2025-01-07T17:55:55Z

test/e2e/framework/scaletest/add-shared-labels.go

+		metav1.PatchOptions{},
+	)
+	if err != nil {
+		return fmt.Errorf("error patching pod: %w", err)


When I wrap errors, the word "error" is typically redundant (it's an error, so it will be logged as such). It's sufficient to just write what you were trying to do when the error occurred, as you've done:

Suggested change

return fmt.Errorf("error patching pod: %w", err)

return fmt.Errorf("patching pod: %w", err)

agree that "error" is redundant, but I prefer to write <what happened> and think that that reads easier when scanning logs.
something like

Suggested change

return fmt.Errorf("error patching pod: %w", err)

return fmt.Errorf("failed to patch Pod: %w", err)

timraymond · 2025-01-07T17:57:41Z

test/e2e/framework/scaletest/add-unique-labels.go

+			Path:  "/metadata/labels/uni-lab-" + fmt.Sprintf("%05d", *counter),
+			Value: "val",
+		})
+		(*counter)++


Why not just return this? Or let the caller deal with JSON marshalling so they can just look at the len?

timraymond · 2025-01-07T18:02:23Z

test/e2e/framework/scaletest/create-resources.go

@@ -48,11 +53,18 @@ func (c *CreateResources) Run() error {
 		return fmt.Errorf("error creating Kubernetes client: %w", err)
 	}

-	ctx, cancel := context.WithTimeout(context.Background(), defaultTimeoutSeconds*time.Second)
+	ctx, cancel := context.WithTimeout(context.Background(), timeoutCreateResourcesSeconds*time.Second)


I can't find the other instance, but I also want to point out that value * time.Second is dangerous math to do--you should eliminate it every chance you get. The value can easily overflow as you can see here if someone tries to give you a duration: https://go.dev/play/p/tZhE9KQPdju . time.Duration is a first-class type and it should be passed around and used.

+1000000
please use time.Duration for time instead of the primitive numerical types, and use it as soon as possible: at the interface of parsing config; in the func args; whenever you declare a time var or const

@timraymond @rbtr This and other places that is following this kind of pattern (variable to store a single number) was requested by the linter, e.g.:

Error: test/e2e/framework/kubernetes/delete-namespace.go:48:13: Magic number: 9, in <assign> detected (mnd) Steps: 9, ^

Should we disable this mnd linter?

timraymond · 2025-01-07T18:03:33Z

test/e2e/framework/scaletest/get-publish-metrics.go

 	g.stop = make(chan struct{})
 	g.wg.Add(1)

 	go func() {

-		t := time.NewTicker(5 * time.Minute)
+		t := time.NewTicker(defaultInterval)


This leaks a goroutine.

defer t.Stop()

Not a huge deal in tests, but it's a habit you should drill.

timraymond · 2025-01-07T18:04:26Z

test/e2e/framework/scaletest/get-publish-metrics.go

+			log.Fatalf("error getting and publishing number of restarts: %v", err)
+			return


There's no point in returning since log.Fatalf will panic. That said, you should not panic :)

timraymond · 2025-01-07T18:05:48Z

test/e2e/framework/scaletest/get-publish-metrics.go

@@ -88,53 +125,61 @@ func (g *GetAndPublishMetrics) Prevalidate() error {
 	if _, ok := g.AdditionalTelemetryProperty["retinaVersion"]; !ok {
 		return fmt.Errorf("retinaVersion is required in AdditionalTelemetryProperty")
 	}
+
+	if os.Getenv(common.OutputFilePathEnv) == "" {


I'm not crazy about ad-hoc os.Getenv. The trouble is that it gets difficult to reason about "what is the entire configuration for this process" if you do it often enough. Is there some place where you can put this where it will get loaded in a config struct you can pass around?

timraymond · 2025-01-07T18:06:31Z

test/e2e/framework/scaletest/get-publish-metrics.go

+		permissions := 0o644
+		file, err := os.OpenFile(g.outputFilePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, fs.FileMode(permissions))


Inline permissions

timraymond · 2025-01-07T18:11:52Z

test/e2e/framework/scaletest/add-shared-labels.go

@@ -12,6 +13,10 @@ import (
 	"k8s.io/client-go/tools/clientcmd"
 )

+const (
+	timeoutToLabelAllPodsMinutes = 120


Again, please use time.Duration:

Suggested change

timeoutToLabelAllPodsMinutes = 120

podLabelTimeout = 120 * time.Minute

Signed-off-by: Alex Castilio dos Santos <[email protected]>

alexcastilio requested a review from a team as a code owner November 29, 2024 16:02

alexcastilio requested review from rbtr and jpayne3506 November 29, 2024 16:02

github-actions bot added the meta/waiting-for-author Blocked and waiting on the author label Dec 30, 2024

alexcastilio removed the meta/waiting-for-author Blocked and waiting on the author label Jan 2, 2025

alexcastilio force-pushed the alexcastilio/fix-scale-test branch from 9791c43 to 419dc4d Compare January 2, 2025 16:16

matmerr previously approved these changes Jan 6, 2025

View reviewed changes

alexcastilio force-pushed the alexcastilio/fix-scale-test branch from 419dc4d to c2ef8e9 Compare January 7, 2025 08:54

alexcastilio dismissed matmerr’s stale review via a41e79d January 7, 2025 09:23

alexcastilio force-pushed the alexcastilio/fix-scale-test branch 8 times, most recently from bf79ef1 to c57c17a Compare January 7, 2025 15:50

timraymond requested changes Jan 7, 2025

View reviewed changes

alexcastilio force-pushed the alexcastilio/fix-scale-test branch 3 times, most recently from 2a7c193 to bb3a29f Compare January 8, 2025 16:06

fix(test): configuration changes and fixes needed to scale-test

bcb2d2d

Signed-off-by: Alex Castilio dos Santos <[email protected]>

alexcastilio force-pushed the alexcastilio/fix-scale-test branch from bb3a29f to bcb2d2d Compare January 8, 2025 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(test): configuration changes and fixes needed to scale-test #1085

fix(test): configuration changes and fixes needed to scale-test #1085

alexcastilio commented Nov 29, 2024 •

edited

Loading

github-actions bot commented Dec 30, 2024

matmerr left a comment

timraymond Jan 7, 2025

timraymond Jan 7, 2025

timraymond Jan 7, 2025

rbtr Jan 7, 2025 •

edited

Loading

timraymond Jan 7, 2025

timraymond Jan 7, 2025

rbtr Jan 7, 2025 •

edited

Loading

alexcastilio Jan 7, 2025 •

edited

Loading

timraymond Jan 7, 2025

timraymond Jan 7, 2025

timraymond Jan 7, 2025

timraymond Jan 7, 2025

timraymond Jan 7, 2025

	ctx, cancel := context.WithTimeout(context.Background(), deleteNamespaceTimeoutSeconds*time.Second)
	ctx, cancel := context.WithTimeout(context.Background(), 1200 * time.Second)

	return fmt.Errorf("error patching pod: %w", err)
	return fmt.Errorf("patching pod: %w", err)

	return fmt.Errorf("error patching pod: %w", err)
	return fmt.Errorf("failed to patch Pod: %w", err)

		log.Fatalf("error getting and publishing number of restarts: %v", err)
		return

		permissions := 0o644
		file, err := os.OpenFile(g.outputFilePath, os.O_APPEND\|os.O_CREATE\|os.O_WRONLY, fs.FileMode(permissions))

	timeoutToLabelAllPodsMinutes = 120
	podLabelTimeout = 120 * time.Minute

fix(test): configuration changes and fixes needed to scale-test #1085

Are you sure you want to change the base?

fix(test): configuration changes and fixes needed to scale-test #1085

Conversation

alexcastilio commented Nov 29, 2024 • edited Loading

Description

Related Issue

Checklist

Screenshots (if applicable) or Testing Completed

Additional Notes

github-actions bot commented Dec 30, 2024

matmerr left a 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

rbtr Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtr Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

alexcastilio Jan 7, 2025 • 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

alexcastilio commented Nov 29, 2024 •

edited

Loading

rbtr Jan 7, 2025 •

edited

Loading

rbtr Jan 7, 2025 •

edited

Loading

alexcastilio Jan 7, 2025 •

edited

Loading