(feat): Add Metrics Exporter Sidecar to ValkeyCluster Pods#49
(feat): Add Metrics Exporter Sidecar to ValkeyCluster Pods#49sandeepkunusoth merged 5 commits intovalkey-io:mainfrom
Conversation
Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
jdheyburn
left a comment
jdheyburn
left a comment
There was a problem hiding this comment.
Overall looks great! Just had a few minor comments
Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
bjosv
left a comment
bjosv
left a comment
There was a problem hiding this comment.
Nice. Added some comments/questions.
| Affinity *corev1.Affinity `json:"affinity,omitempty"` | ||
|
|
||
| // Metrics exporter options | ||
| // +kubebuilder:default:={enabled:true} |
There was a problem hiding this comment.
Is this change needed? And if yes; can we remove // +kubebuilder:default=true on line 88 in the ExporterSpec type now?
There was a problem hiding this comment.
Yes this change is needed otherwise by default its becoming false. I will remove other change in next PR.
| `, valkeyName) | ||
|
|
||
| // Create temporary YAML file | ||
| manifestFile := filepath.Join(os.TempDir(), fmt.Sprintf("%s-%d.yaml", valkeyName, time.Now().UnixNano())) |
There was a problem hiding this comment.
Do we need a unique name here?
We don't do this in the degraded state test where we use valkeycluster-degraded.yaml directly.
| out, err := utils.Run(cmd) | ||
| g.Expect(err).NotTo(HaveOccurred(), "Deployment not found") | ||
| g.Expect(out).To(ContainSubstring(valkeyName)) | ||
| }, 2*time.Minute, 5*time.Second).Should(Succeed()) |
There was a problem hiding this comment.
The default is 2 minutes, so we could remove all these extra timeouts to get rid of clutter
| }, 2*time.Minute, 5*time.Second).Should(Succeed()) | |
| }).Should(Succeed()) |
There was a problem hiding this comment.
yes forgot about this suggestion from past PRs. will update this in next PR.
| `, valkeyName) | ||
|
|
||
| // Create temporary YAML file | ||
| manifestFile := filepath.Join(os.TempDir(), fmt.Sprintf("%s-%d.yaml", valkeyName, time.Now().UnixNano())) |
There was a problem hiding this comment.
same comment here regarding including time in the filename.
If the reason is to enable it to run in parallel, maybe https://pkg.go.dev/os#CreateTemp is a better option to get uniqueness.
|
|
||
| By("verifying common Valkey metrics are exposed") | ||
| Eventually(func(g Gomega) { | ||
| cmd := exec.Command("curl", "-s", "http://localhost:9121/metrics") |
There was a problem hiding this comment.
This test now requires curl to be installed on the system.
An option that is to use a curl pod, that how the kubebuilder scaffolding gets the metrics from the controller.
) This PR implements valkey-io#28 by adding redis_exporter sidecar to ValkeyCluster pods (configurable via spec.exporter) so each pod exposes Valkey metrics on port 9121. also includes e2e tests to validate redis exporter metrics and small refactors to the existing e2e test suite(moved some common code from e2e_test.go to e2e_suite_test.go). --------- Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
) This PR implements valkey-io#28 by adding redis_exporter sidecar to ValkeyCluster pods (configurable via spec.exporter) so each pod exposes Valkey metrics on port 9121. also includes e2e tests to validate redis exporter metrics and small refactors to the existing e2e test suite(moved some common code from e2e_test.go to e2e_suite_test.go). --------- Signed-off-by: Sandeep Kunusoth <31273507+sandeepkunusoth@users.noreply.github.com>
…#54) This PR fixes e2e test failing due to #43 temporarily by deleting replica deployment in e2e tests instead of deleting random deployment (either replica or shard previously). We need to add back e2e test to delete shard and allow it to recover once above issue is fixed. Also addressed review comments from metrics exporter PR @bjosv #49 --------- Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
3 participants
This PR implements #28 by adding redis_exporter sidecar to ValkeyCluster pods (configurable via spec.exporter) so each pod exposes Valkey metrics on port 9121. also includes e2e tests to validate redis exporter metrics and small refactors to the existing e2e test suite(moved some common code from e2e_test.go to e2e_suite_test.go).