Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: test case multi-namespace_support #312

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

lihongyan1
Copy link
Contributor

Fix issue #311

Prometheus pod need more resources and has complicated configuration, replace it with prometheus example app pod

@lihongyan1 lihongyan1 requested a review from a team as a code owner July 14, 2023 06:33
@lihongyan1 lihongyan1 force-pushed the fix-case-multi-namespace branch 3 times, most recently from d9c07d0 to a3862f7 Compare July 14, 2023 06:51
@lihongyan1 lihongyan1 changed the title fix:(test)TestMonitoringStackController/Verify_multi-namespace_support fix: TestMonitoringStackController/Verify_multi-namespace_support Jul 14, 2023
@lihongyan1 lihongyan1 changed the title fix: TestMonitoringStackController/Verify_multi-namespace_support fix: test case multi-namespace_support Jul 14, 2023
@lihongyan1 lihongyan1 force-pushed the fix-case-multi-namespace branch from a3862f7 to d7400d5 Compare July 14, 2023 06:57
Name: "prometheus",
Image: "quay.io/prometheus/prometheus:v2.39.1",
Name: "prometheus-example-app",
Image: "quay.io/openshifttest/prometheus-example-app@sha256:382dc349f82d730b834515e402b48a9c7e2965d0efbc42388bd254f424f6193e",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should instead rely on what upstream Prometheus Operator is using ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to use this one, for I run the upstream test cases on OCP with all kinds of profiles share between QE team during ObO release, the image will be mirrored by default and the test cases can get the image and run on disconnected or private OCP clusters, other wise, the case will failed on private and disconnected OCP cluster.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lihongyan1 would you mind leaving a comment that describes this and a link to the upstream that is mirrored? Just for future us when someone asks why we don't use the upstream version :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments

Name: "prometheus-example-app",
Image: "quay.io/openshifttest/prometheus-example-app@sha256:382dc349f82d730b834515e402b48a9c7e2965d0efbc42388bd254f424f6193e",
SecurityContext: &corev1.SecurityContext{
AllowPrivilegeEscalation: &bF,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: consider pointer.Bool - https://pkg.go.dev/k8s.io/utils/pointer#Bool

Name: "prometheus-example-app",
Image: "quay.io/openshifttest/prometheus-example-app@sha256:382dc349f82d730b834515e402b48a9c7e2965d0efbc42388bd254f424f6193e",
SecurityContext: &corev1.SecurityContext{
AllowPrivilegeEscalation: &bF,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: consider pointer.Bool - https://pkg.go.dev/k8s.io/utils/pointer#Bool

@@ -861,7 +861,7 @@ func namespaceSelectorTest(t *testing.T) {

promClient := framework.NewPrometheusClient("http://localhost:9090")
if pollErr := wait.Poll(5*time.Second, 5*time.Minute, func() (bool, error) {
query := `prometheus_build_info{namespace=~"test-ns-.*"}`
query := `version{namespace=~"test-ns-.*"}`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a different metric that we can reply on that is specific to prometheus_example ?

Copy link
Contributor Author

@lihongyan1 lihongyan1 Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there are the following metrics, for detail you can refer web site, none of them are specific

version - of type gauge - containing the app version - as a constant metric value 1 and label version, representing this app version
http_requests_total - of type counter - representing the total numbere of incoming HTTP requests
http_request_duration_seconds - of type histogram, representing duration of all HTTP requests
http_request_duration_seconds_count- total count of all incoming HTTP requeests
http_request_duration_seconds_sum - total duration in seconds of all incoming HTTP requests
http_request_duration_seconds_bucket

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know what you mean, I changed the metric as version{pod="prometheus-example-app",namespace=~"test-ns-.*"}, thanks for the comments

Copy link
Collaborator

@sthaha sthaha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the patch @lihongyan1. The approach looks good to me. Some comments that needs addressing.

@lihongyan1 lihongyan1 force-pushed the fix-case-multi-namespace branch 2 times, most recently from 39e0e60 to c5cbc06 Compare July 20, 2023 06:33
@lihongyan1 lihongyan1 requested a review from sthaha July 20, 2023 07:51
@lihongyan1 lihongyan1 force-pushed the fix-case-multi-namespace branch 2 times, most recently from 2131315 to 569c89e Compare July 24, 2023 02:41
@lihongyan1 lihongyan1 force-pushed the fix-case-multi-namespace branch from 569c89e to db37e9b Compare July 31, 2023 01:43
Copy link
Collaborator

@sthaha sthaha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jan--f jan--f merged commit 6c09f46 into rhobs:main Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants