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

tests: simplify monitoringdashboard test case, add to tests-e2e-direct #1861

Conversation

justinsb
Copy link
Collaborator

@justinsb justinsb commented May 21, 2024

Changing to a random display name is not very intuitive (and doesn't
get us much).

Also add this to tests-e2e-direct, so we verify the golden output

@@ -38,7 +38,7 @@ KCC_USE_DIRECT_RECONCILERS=LoggingLogMetric \
GOLDEN_OBJECT_CHECKS=1 \
GOLDEN_REQUEST_CHECKS=1 \
E2E_KUBE_TARGET=envtest RUN_E2E=1 E2E_GCP_TARGET=mock \
go test -test.count=1 -timeout 600s -v ./tests/e2e -run 'TestAllInSeries/fixtures/explicitlogmetric|TestAllInSeries/fixtures/exponentiallogmetric|TestAllInSeries/fixtures/linearlogmetric'
go test -test.count=1 -timeout 600s -v ./tests/e2e -run 'TestAllInSeries/fixtures/explicitlogmetric|TestAllInSeries/fixtures/exponentiallogmetric|TestAllInSeries/fixtures/linearlogmetric|TestAllInSeries/fixtures/monitoringdashboard'
Copy link
Collaborator

Choose a reason for hiding this comment

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

two quick points:

  • this is not using the direct controller for monitoring yet
  • we probably want to ignore the s-i-s: merge

I don't think we can do the same trick as the script.yaml annotations for absent since these are fixtures but will leave that up to you if you want to modify the yaml or add a code check (specifically for monitoring I think) for now.

( these are probably all items I could've done better 😛 )

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you share more context about the "script.yaml and the trick" (just in case others may hit a problem)?

I thought this go test is a short path for LLM only: the KCC_USE_DIRECT_RECONCILERS specifies to LLM and the run only lists LLM related tests.
If we want to run tests for different scifi controllers, maybe we shall separate it from here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we can do the same trick as the script.yaml annotations for absent since these are fixtures

Oh I see, I think this comment was a bit confusing and I should've linked the code I had in mind 🤦🏼 Here's an example of using state-into-spec: absent inscript.yaml:

apiVersion: logging.cnrm.cloud.google.com/v1beta1
kind: LoggingLogMetric
metadata:
name: logginglogmetric-${uniqueId}
annotations:
cnrm.cloud.google.com/state-into-spec: absent

But again, I don't think we can use that here because the fixture tests are shared in test-e2e-fixtures today, and more importantly, in post submits.

I thought this go test is a short path for LLM only: the KCC_USE_DIRECT_RECONCILERS specifies to LLM and the run only lists LLM related tests.

Yes I meant to highlight that in with this point:

  • this is not using the direct controller for monitoring yet

So today, adding that would be a "no-op" and yes we would require a KCC_USE_DIRECT_RECONCILERS flag when we actually want to use the direct controller. Moreover, I'd recommend we split out the LLM and other resources 😛

@justinsb justinsb force-pushed the monitoring_dashboard_tests branch from 8d9503b to 1bdf4c4 Compare May 21, 2024 20:28
@justinsb
Copy link
Collaborator Author

Actually looks like I did most of this better in #1806, so let's get that one in first, then I'll rebase

/hold

@justinsb justinsb force-pushed the monitoring_dashboard_tests branch 3 times, most recently from 204ca9c to 1e03b41 Compare May 31, 2024 12:08
@justinsb justinsb changed the title chore: update monitoringdashboard test data tests: simplify monitoringdashboard test case, add to tests-e2e-direct May 31, 2024
@@ -38,7 +38,7 @@ KCC_USE_DIRECT_RECONCILERS=LoggingLogMetric \
GOLDEN_OBJECT_CHECKS=1 \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to double check, do we want to run monitoringdashboard direct controller on the test "monitoringdashboard"? If so, I think we want to include the Kind name in KCC_USE_DIRECT_RECONCILERS or using a different command (so you can  echo the steps more granularly)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I wanted to start by adding this to the "strict" tests where we check the golden output. Then I would add it to KCC_USE_DIRECT_RECONCILERS, and we would be able to see the changes in behaviour (if any) at the http and at the KRM level.

I think what's going wrong is that we've turned off golden tests with KCC_USE_DIRECT_RECONCILERS, and I don't think we should have done that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good. With #1910 , those LLM tests should no longer need the KCC_USE_DIRECT_RECONCILERS flag (and thanks for the cleanup in #1919).

When the monitoring dashboard is ready for the direct controller, you can simply remove the dcl2crd label (KCC_USE_DIRECT_RECONCILERS should also work but I think it less preferred for beta resource)

@justinsb justinsb force-pushed the monitoring_dashboard_tests branch from 1e03b41 to ced16e3 Compare June 5, 2024 01:30
@yuwenma
Copy link
Collaborator

yuwenma commented Jun 5, 2024

PR looks good. could you capture the latest log and I think this PR is ready to go!

@justinsb justinsb force-pushed the monitoring_dashboard_tests branch from ced16e3 to 69a1716 Compare June 5, 2024 02:04
@justinsb
Copy link
Collaborator Author

justinsb commented Jun 5, 2024

I think the flakes are due to races in the SLEEP approach, which was always intended as a temporary hack to be fixed in #1800. I updated that PR.

@justinsb justinsb force-pushed the monitoring_dashboard_tests branch from 69a1716 to 066aac1 Compare June 5, 2024 10:20
@justinsb
Copy link
Collaborator Author

justinsb commented Jun 7, 2024

#1800 has merged, so I think this is unblocked

/assign @yuwenma

/hold cancel

(Because that hold is very old!)

@yuwenma
Copy link
Collaborator

yuwenma commented Jun 7, 2024

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Jun 7, 2024
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yuwenma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 87b178d into GoogleCloudPlatform:master Jun 7, 2024
13 checks passed
@yuwenma yuwenma added this to the 1.119 milestone Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants