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.

Sign up for GitHub

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

Jump to bottom

WINC-983: [e2e] Ensure required log files are non-empty #1421

Merged
merged 1 commit into from
Feb 27, 2023
Merged

WINC-983: [e2e] Ensure required log files are non-empty #1421

merged 1 commit into from
Feb 27, 2023

Conversation

mansikulkarni96
Copy link
Member

This commit adds a check to the logs tests to check that log files have non-zero size, ensuring they are actually being written to by the services.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 18, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 18, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@mansikulkarni96 mansikulkarni96 changed the title [e2e] Ensure required log files are non-emptyi WINC-983: [e2e] Ensure required log files are non-empty Feb 18, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 18, 2023 •
edited by openshift-ci bot

@mansikulkarni96: This pull request references WINC-983 which is a valid jira issue.

In response to this:

This commit adds a check to the logs tests to check that log files have non-zero size, ensuring they are actually being written to by the services.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 18, 2023
@mansikulkarni96
Copy link
Member Author

/test aws-e2e-operator

saifshaikh48
saifshaikh48 reviewed Feb 22, 2023
View reviewed changes
test/e2e/logs_test.go Outdated
Comment on lines 70 to 73
logInfo, err := os.Stat(outputFile)
if err != nil || logInfo.Size() == 0 {
return errors.Wrap(err, "log file should not be empty")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should be done at the retrieveLogs() call site to avoid giving too much responsibility to this function. Also we're retrying on error at the call site -- when we get a log size of zero, we don't need to retry

@alinaryan
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2023
aravindhp
aravindhp suggested changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@aravindhp aravindhp left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @mansikulkarni96

test/e2e/logs_test.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2023
@mansikulkarni96 mansikulkarni96 requested review from aravindhp and removed request for saifshaikh48 February 23, 2023 19:43
saifshaikh48
saifshaikh48 reviewed Feb 23, 2023
View reviewed changes
Copy link
Contributor

@saifshaikh48 saifshaikh48 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aravindhp, mansikulkarni96

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 23, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2023
@mansikulkarni96 mansikulkarni96 marked this pull request as ready for review February 23, 2023 20:58
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 23, 2023
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 4d372cb and 2 for PR HEAD fcc609d in total

@mansikulkarni96
Copy link
Member Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 23, 2023
@mansikulkarni96 mansikulkarni96 marked this pull request as draft February 23, 2023 23:08
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 23, 2023
@mansikulkarni96
[e2e] Ensure required log files are non-empty
24d3311 
This commit  adds a check to the logs tests to check that
log files have non-zero size, ensuring they are actually being
written to by the services.
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2023
@mansikulkarni96
Copy link
Member Author

/test aws-e2e-operator

@mansikulkarni96 mansikulkarni96 marked this pull request as ready for review February 24, 2023 16:44
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 24, 2023
@saifshaikh48
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2023
@mansikulkarni96
Copy link
Member Author

/retest

@mansikulkarni96
Copy link
Member Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2023
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 4d372cb and 2 for PR HEAD 24d3311 in total

@aravindhp
Copy link
Contributor

/retest-required. azure-e2e-ccm-install failed with:

 ERRO[2023-02-27T16:23:11Z] Some steps failed:                           
ERRO[2023-02-27T16:23:11Z] 
  * could not run steps: step [release-inputs:latest] failed: could not create snapshot imagestream ocp/4.13 for release latest: Timeout: request did not complete within requested timeout - context deadline exceeded

@aravindhp
Copy link
Contributor

/retest-required

@mansikulkarni96
Copy link
Member Author

/retest-required
There was an outage reported in prow, status is fixed now retrying.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 27, 2023

@mansikulkarni96: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit c348eff into openshift:master Feb 27, 2023
@mansikulkarni96
Copy link
Member Author

/cherry-pick release-4.12

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Mar 5, 2023
Merged
@openshift-cherrypick-robot
Copy link

@mansikulkarni96: new pull request created: #1458

In response to this:

/cherry-pick release-4.12

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mansikulkarni96
Copy link
Member Author

/cherry-pick release-4.11

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Mar 7, 2023
Merged
@openshift-cherrypick-robot
Copy link

@mansikulkarni96: new pull request created: #1463

In response to this:

/cherry-pick release-4.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mansikulkarni96
Copy link
Member Author

/cherry-pick release-4.10

@openshift-cherrypick-robot
Copy link

@mansikulkarni96: #1421 failed to apply on top of branch "release-4.10":

Applying: Ensure required log files are non-empty
Using index info to reconstruct a base tree...
M	test/e2e/logs_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/logs_test.go
CONFLICT (content): Merge conflict in test/e2e/logs_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Ensure required log files are non-empty
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mansikulkarni96
Copy link
Member Author

/cherry-pick release-4.10

@openshift-cherrypick-robot
Copy link

@mansikulkarni96: #1421 failed to apply on top of branch "release-4.10":

Applying: Ensure required log files are non-empty
Using index info to reconstruct a base tree...
M	test/e2e/logs_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/logs_test.go
CONFLICT (content): Merge conflict in test/e2e/logs_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Ensure required log files are non-empty
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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

@saifshaikh48 saifshaikh48 saifshaikh48 left review comments

@aravindhp aravindhp Awaiting requested review from aravindhp

@sebsoto sebsoto Awaiting requested review from sebsoto

Assignees

@saifshaikh48 saifshaikh48

@alinaryan alinaryan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@mansikulkarni96 @openshift-ci-robot @alinaryan @saifshaikh48 @aravindhp @openshift-cherrypick-robot @openshift-merge-robot