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

ci: fix reload config for empty pinned image test #8399

Merged
merged 2 commits into from
Sep 24, 2024
Merged

ci: fix reload config for empty pinned image test #8399

merged 2 commits into from
Sep 24, 2024

Conversation

littlejawa
Copy link
Contributor

What type of PR is this?

/kind failing-test

What this PR does / why we need it:

Modify the "wait_for_log" function in helpers.bash to allow it to catch multiple occurrences of the same message.
Use that in the failing test to synchronize and wait for config reload to be finished before checking the content of the configuration.

Which issue(s) this PR fixes:

Fixes #8324

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

@littlejawa littlejawa requested a review from mrunalp as a code owner July 18, 2024 12:46
@openshift-ci openshift-ci bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. labels Jul 18, 2024
@openshift-ci openshift-ci bot requested review from hasan4791 and klihub July 18, 2024 12:46
@codecov Codecov
Copy link

codecov bot commented Jul 18, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.85%. Comparing base (44e0241) to head (eda8023).
Report is 67 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8399      +/-   ##
==========================================
- Coverage   49.25%   48.85%   -0.40%     
==========================================
  Files         153      153              
  Lines       17254    17371     +117     
==========================================
- Hits         8498     8487      -11     
- Misses       7688     7819     +131     
+ Partials     1068     1065       -3

@littlejawa
Copy link
Contributor Author

/test ci-fedora-critest

@littlejawa
Copy link
Contributor Author

@sohankunkerkar, @kwilczynski - as you reviewed the original PR, could you get a look at this one?

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2024
@littlejawa littlejawa force-pushed the ci/fix_wait_for_log branch from 4cd05d8 to 7f02fda Compare July 26, 2024 07:01
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2024
@littlejawa littlejawa force-pushed the ci/fix_wait_for_log branch from 7f02fda to 5df18a5 Compare August 20, 2024 09:35
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2024
@haircommander
Copy link
Member

/approve
/retest

@cri-o/cri-o-maintainers PTAL

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2024
@littlejawa
Copy link
Contributor Author

@cri-o/cri-o-maintainers - can we merge this one ?

@haircommander
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2024
@sohankunkerkar
Copy link
Member

@littlejawa could you address https://github.com/cri-o/cri-o/actions/runs/10469089567/job/28991329813?pr=8399?

@sohankunkerkar
Copy link
Member

/hold
Feel free to unhold when the test is fixed.

@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 Sep 19, 2024
kwilczynski
kwilczynski reviewed Sep 20, 2024
View reviewed changes
test/helpers.bash Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2024
@roman-kiselenko
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2024
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2024
@sohankunkerkar
Copy link
Member

@littlejawa, it looks like CI is almost green! Could you squash the last four commits and prepare this PR for merge?

@littlejawa
tests: improve wait_for_log to allow multiple calls for the same message
c02f9bb 
wait_for_log looks at crio logs and waits for the given message to appear.
If the message is already in the logs, it gets out immediately.

To allow waiting for another occurence of the same message, we're making
it output the timestamp of the first occurence that it finds.
The caller can keep that information and provide it as a second parameter
to wait_for_log, in which case wait_for_log will start waiting for
the message after the given timestamp.

Signed-off-by: Julien Ropé <jrope@redhat.com>
@littlejawa
test: fix empty pinned_images test
eda8023 
As the test is doing multiple reloads, it was hard to use wait_for_log to synchronize
on the end of the reload process.
With the modification on wait_for_log to allow waiting for multiple occurences of a
log message, we can simplify this test, and make sure it's actually in sync.

Keeping only one "wait_for_log" calls should be enough then - no need for multiple
synchronization points, and the config validation is done by the calls to "crictl images".

Fixes: cri-o#8324

Signed-off-by: Julien Ropé <jrope@redhat.com>
@littlejawa
Copy link
Contributor Author

@littlejawa, it looks like CI is almost green! Could you squash the last four commits and prepare this PR for merge?

Done. Thanks !

@haircommander
Copy link
Member

/retest

2 similar comments
@sohankunkerkar
Copy link
Member

/retest

@kwilczynski
Copy link
Member

/retest

sohankunkerkar
sohankunkerkar approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@sohankunkerkar sohankunkerkar 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 openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Sep 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, littlejawa, sohankunkerkar

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

@sohankunkerkar
Copy link
Member

/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 Sep 24, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 64b00ec into cri-o:main Sep 24, 2024
81 of 82 checks passed
@kwilczynski kwilczynski mentioned this pull request Sep 27, 2024
Closed
@kwilczynski
Copy link
Member

/cherry-pick release-1.31

@kwilczynski
Copy link
Member

/cherry-pick release-1.30

@openshift-cherrypick-robot
Copy link

@kwilczynski: new pull request created: #8632

In response to this:

/cherry-pick release-1.31

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-sigs/prow repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Sep 30, 2024
Merged
@openshift-cherrypick-robot
Copy link

@kwilczynski: #8399 failed to apply on top of branch "release-1.30":

Applying: tests: improve wait_for_log to allow multiple calls for the same message
Applying: test: fix empty pinned_images test
Using index info to reconstruct a base tree...
M	contrib/test/ci/integration.yml
M	contrib/test/ci/vars.yml
M	test/reload_config.bats
Falling back to patching base and 3-way merge...
Auto-merging test/reload_config.bats
Auto-merging contrib/test/ci/vars.yml
CONFLICT (content): Merge conflict in contrib/test/ci/vars.yml
Auto-merging contrib/test/ci/integration.yml
CONFLICT (content): Merge conflict in contrib/test/ci/integration.yml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 test: fix empty pinned_images test
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-1.30

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-sigs/prow repository.

@kwilczynski
Copy link
Member

OK. Will cherry-pick manually.

@kwilczynski kwilczynski mentioned this pull request Oct 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwilczynski kwilczynski kwilczynski left review comments

@sohankunkerkar sohankunkerkar sohankunkerkar approved these changes

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner

@hasan4791 hasan4791 Awaiting requested review from hasan4791

@klihub klihub Awaiting requested review from klihub

Assignees

@roman-kiselenko roman-kiselenko

@haircommander haircommander

@sohankunkerkar sohankunkerkar

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Flaky test on Kata runtime (reload_config)
7 participants
@littlejawa @haircommander @sohankunkerkar @roman-kiselenko @kwilczynski @openshift-cherrypick-robot @openshift-merge-robot