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(e2e): ignore error log about failed storage update #19060

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ghouscht
Copy link
Contributor

Should fix #18978 as suggested by @ahrtr

tests/e2e/logging_test.go Outdated Show resolved Hide resolved
"setting up serving from embedded etcd failed.": true,
// See https://github.com/etcd-io/etcd/pull/19040#issuecomment-2539173800
// TODO: Remove with etcd 3.7
"failed to update storage version": true,
Copy link
Member

Choose a reason for hiding this comment

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

We only ignore the specific error message.

Suggested change
"failed to update storage version": true,
"cannot detect storage schema version: missing confstate information": true,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test seems to check the msg not error object in the logged JSON: https://github.com/ghouscht/etcd/blob/ignore-missing-err-confstate/tests/e2e/logging_test.go#L136-L139

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to check both the msg and error. In this case, it makes more sense to ignore the specific error message cannot detect storage schema version: missing confstate information.

We can add one more field "Error" into logEntry.

type logEntry struct {
	Level     string `json:"level"`
	Timestamp string `json:"ts"`
	Caller    string `json:"caller"`
	Message   string `json:"msg"`
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adapted the code to check the Message and Error for a match. If one of the two matches exactly the error is ignored. Hope that is ok

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.78%. Comparing base (25dfc82) to head (07fd512).

Additional details and impacted files

see 19 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19060      +/-   ##
==========================================
- Coverage   68.83%   68.78%   -0.05%     
==========================================
  Files         420      420              
  Lines       35626    35626              
==========================================
- Hits        24523    24507      -16     
- Misses       9679     9692      +13     
- Partials     1424     1427       +3     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25dfc82...07fd512. Read the comment docs.

@ghouscht ghouscht force-pushed the ignore-missing-err-confstate branch from 27a885e to 07fd512 Compare December 13, 2024 15:28
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you!

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, ghouscht

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

@ahrtr
Copy link
Member

ahrtr commented Dec 13, 2024

cc @ivanvc @jmhbnz @serathius

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

Successfully merging this pull request may close these issues.

Etcd writing error in tests about missing confstate
3 participants