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

Add an e2e tests that detects error logs during etcd bootstrap #17329

Open
serathius opened this issue Jan 26, 2024 · 14 comments · Fixed by #17423
Open

Add an e2e tests that detects error logs during etcd bootstrap #17329

serathius opened this issue Jan 26, 2024 · 14 comments · Fixed by #17423

Comments

@serathius
Copy link
Member

What would you like to be added?

Etcd should not write any error logs during normal operation. We could add a test that starts etcd cluster, does some simple operations and validates if there is any error level log written.

This could be further improved by enabling this check in all e2e tests when cluster/server is being closed. As some test intentionally inject errors, we could make it configurable option that is enabled by default and disabled in all tests we expect error to be written.

Why is this needed?

Logs are used to provide useful debug information to users, however if mislabeled they could cause user to lose trust and ignore errors. For error logs to be useful they should only be emitted where there is something wrong.

We should prevent issues like #17245 where a change in code started generating a large number of error logs.

Proposed in #17249 (comment) with agreement between maintainers.

@nitishfy
Copy link
Contributor

Hi, I'd like to take this up!

@serathius
Copy link
Member Author

Thanks, feel free to reach out to me on slack (get an invite) if you need any help
/assign @nitishfy

@serathius
Copy link
Member Author

#17423 covered a single node cluster, we should also cover:

  • 3 node cluster
  • different TLS configurations.

@serathius serathius reopened this Feb 14, 2024
@nitishfy
Copy link
Contributor

#17423 covered a single node cluster, we should also cover:

  • 3 node cluster
  • different TLS configurations.

we can raise a new PR for it. WDYT?

@serathius
Copy link
Member Author

SG, thanks

@serathius
Copy link
Member Author

Also, when we are done, we should consider backporting the test.

@jmhbnz jmhbnz changed the title Add an E2e tests that detects error logs during etcd boorstrap Add an e2e tests that detects error logs during etcd bootstrap Aug 12, 2024
@serathius
Copy link
Member Author

Still remaining to be completed:

  • Extend the existing test to also validate 3 node cluster.
  • Backport the test to release-3.5 and release-3.4 branches to prevent regressions there.

@ghouscht
Copy link
Member

I think I can take this.

/assign

@ghouscht
Copy link
Member

ghouscht commented Oct 31, 2024

@serathius
Copy link
Member Author

Awesome, thanks @ghouscht !

@ahrtr
Copy link
Member

ahrtr commented Oct 31, 2024

It looks like the test doesn't enable TLS by default. Should we also extend the test to support the case of enabling TLS?

@serathius
Copy link
Member Author

Good point, I missed #17329 (comment).

@serathius serathius reopened this Oct 31, 2024
@ghouscht
Copy link
Member

Oh I missed that as well, sorry! I'll have a look later on and will prepare another PR.

@ghouscht
Copy link
Member

ghouscht commented Nov 1, 2024

Something like this #18819? Or did you have something different in your mind?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment