test(cosmovisor): fix typo daemon + fix flaky test #21487

chillyvee · 2024-08-30T18:03:14Z

Update spelling of deamon to daemon
Attempt to fix #21086

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

included the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
targeted the correct branch (see PR Targeting)
provided a link to the relevant issue or specification
reviewed "Files changed" and left comments if necessary
included the necessary unit and integration tests
added a changelog entry to CHANGELOG.md
updated the relevant documentation or specification, including comments for documenting Go code
confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

I have...

confirmed the correct type prefix in the PR title
confirmed all author checklist items have been addressed
reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

Documentation
- Added a new entry in the changelog to document recent improvements, including a typo fix in the daemon reference.
Improvements
- Corrected a typographical error from "deamonHome" to "daemonHome" for better clarity and consistency.
- Enhanced timing logic in shutdown grace test for improved reliability.

coderabbitai · 2024-08-30T18:03:21Z

Walkthrough

The changes involve updating the CHANGELOG.md to include new entries about improvements, specifically a typo fix in the cosmovisor module and a correction in the upgrade-info.json TestLaunchProcess case. Additionally, the scanner.go file has been modified to correct the spelling of the variable deamonHome to daemonHome in the fileWatcher struct and its related methods. The process_test.go file has also been updated to adjust the PollInterval for tests. These updates focus on documentation and code clarity without altering any functionality.

Changes

File(s)	Change Summary
CHANGELOG.md	Added new entries under "Improvements" for a typo fix in the `cosmovisor` module and the `upgrade-info.json` TestLaunchProcess case.
tools/cosmovisor/process_test.go	Decreased `PollInterval` from 20 to 15 in `TestLaunchProcess` and `TestLaunchProcessWithRestartDelay`.
tools/cosmovisor/scanner.go	Corrected the spelling of `deamonHome` to `daemonHome` in the `fileWatcher` struct and methods.

Assessment against linked issues

Objective	Addressed	Explanation
Fix flaky test `TestLaunchProcess` (#21086)	✅

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

chillyvee · 2024-08-30T18:30:16Z

appears failing test existed in previous PR
https://github.com/cosmos/cosmos-sdk/actions/runs/10338915901/job/28617647938?pr=20585

julienrbrt

Thanks!

CHANGELOG.md

chillyvee · 2024-08-30T18:42:23Z

FYI previous PR does not close:
#21086 (comment)

Test still randomly failing

needs a wait for file to be filled

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ce8f9d4 and 0adc6333ba2c0bee4b74a6d28e407190e8ea3c34.

Files selected for processing (2)

CHANGELOG.md (1 hunks)
tools/cosmovisor/scanner.go (3 hunks)

Files skipped from review due to trivial changes (2)

CHANGELOG.md
tools/cosmovisor/scanner.go

julienrbrt · 2024-08-30T18:59:27Z

FYI previous PR does not close:

#21086 (comment)

Test still randomly failing

needs a wait for file to be filled

Yes, I tried that but it will break other tests as the delay will still be too large, that's why it was commented.

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 0adc6333ba2c0bee4b74a6d28e407190e8ea3c34 and f05b361a0604ee0bbd9de2f4b284f565753e04f1.

Files selected for processing (2)

CHANGELOG.md (1 hunks)
tools/cosmovisor/scanner.go (4 hunks)

Files skipped from review due to trivial changes (1)

tools/cosmovisor/scanner.go

Files skipped from review as they are similar to previous changes (1)

CHANGELOG.md

chillyvee · 2024-08-30T19:02:48Z

All tests pass. Do you know which test the 2ms wait breaks?

chillyvee · 2024-08-30T19:18:49Z

Removing wait if file is filled. Will that avoid breaking other tests?

julienrbrt · 2024-08-30T19:20:05Z

Removing wait if file is filled. Will that avoid breaking other tests?

Yeah that's a good one 👏

CHANGELOG.md

julienrbrt · 2024-08-30T19:22:00Z

Ah, I see it still fails with the same error it did without the file size check :(

chillyvee · 2024-08-30T19:28:25Z

Need a faster poll? We only give the chain 1 second to stop.
Not a problem if chain has gov halt.
But a problem if halting without gov prop on a fast chain. In that case, running "daemon status" might not be fast enough?

chillyvee · 2024-08-30T19:47:58Z

ShutdownGrace test can have 1 more second sleep before failing. The use case is the chain has a governance upgrade halt and needs to flush pebbledb to disk before kill. Not urgent to kill the daemon.

chillyvee · 2024-08-30T20:04:43Z

Only sonarcloud failing now

tools/cosmovisor/scanner.go

… filled

…after flush

chillyvee · 2024-08-31T15:07:05Z

added retry loop
Only sonarcloud failing now

chillyvee · 2024-08-31T17:21:03Z

@raynaudoe Off topic: Are you maintaining cosmos/keyring? cosmos/keyring#3

raynaudoe · 2024-08-31T17:38:12Z

@raynaudoe Off topic: Are you maintaining cosmos/keyring? cosmos/keyring#3

Yep happy to review the PR! will do later today, thanks

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f05b361a0604ee0bbd9de2f4b284f565753e04f1 and c7df382.

Files selected for processing (3)

tools/cosmovisor/process_test.go (2 hunks)
tools/cosmovisor/scanner.go (5 hunks)
tools/cosmovisor/testdata/dontdie/cosmovisor/genesis/bin/dummyd (1 hunks)

Files skipped from review as they are similar to previous changes (1)

tools/cosmovisor/scanner.go

Additional context used

Path-based instructions (1)

tools/cosmovisor/process_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

Additional comments not posted (4)

tools/cosmovisor/testdata/dontdie/cosmovisor/genesis/bin/dummyd (2)

14-15: LGTM!

The added comments provide clarity about the purpose and timing expectations of the sleep commands.

18-19: LGTM!

The added comment provides clarity about the state of the chain during the sleep. The increased sleep duration from 1 second to 2 seconds ensures adequate time for the shutdown process.

tools/cosmovisor/process_test.go (2)

148-148: Verify the impact of the polling interval change on the test behavior.

The PollInterval field in the cosmovisor.Config structure has been decreased from 20 to 15 in the TestPlanShutdownGrace function. Please ensure that this change does not introduce any unintended consequences or negatively impact the test behavior, particularly in scenarios where timing is critical.

28-28: Verify the impact of the polling interval change on the test behavior.

The PollInterval field in the cosmovisor.Config structure has been decreased from 20 to 15 in the TestLaunchProcess function. Please ensure that this change does not introduce any unintended consequences or negatively impact the test behavior, particularly in scenarios where timing is critical.

facundomedica · 2024-09-03T10:08:16Z

tools/cosmovisor/scanner.go

+	// check https://github.com/cosmos/cosmos-sdk/issues/21086
+	// If new file is still empty, wait a small amount of time for write to complete
+	if stat.Size() == 0 {
+		for range 10 {


nit: do you think this could be improved with exponential backoff? I think that this is too arbitrary and might cause again flakyness in the test.

for i range 10 { ... time.Sleep(math.Pow(2, float64(i)) * time.Millisecond)

If it were to fail all 10 times, we would have waited a total of ~2046ms. Let's maybe reduce the amount of retries to 5 (62ms).

We can modify in a follow up PR. In practice the loop never runs more than a few times since the file is quickly finalized. The failing test was related more to 1) not waiting at all (fixed here) and 2) test case delay being too similar to timeout (already fixed).

In production, retry or waiting some amount of time is necessary to avoid reading empty files (race condition).

chillyvee requested a review from a team as a code owner August 30, 2024 18:03

github-actions bot added the C:Cosmovisor Issues and PR related to Cosmovisor label Aug 30, 2024

chillyvee force-pushed the cvdeamon branch 2 times, most recently from b3e1da4 to 0adc633 Compare August 30, 2024 18:16

chillyvee changed the title ~~fix(cosmovisor) typo daemon~~ fix(cosmovisor): typo daemon Aug 30, 2024

julienrbrt approved these changes Aug 30, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

julienrbrt changed the title ~~fix(cosmovisor): typo daemon~~ chore(cosmovisor): typo daemon Aug 30, 2024

coderabbitai bot reviewed Aug 30, 2024

View reviewed changes

julienrbrt approved these changes Aug 30, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

julienrbrt changed the title ~~chore(cosmovisor): typo daemon~~ test(cosmovisor): fix typo daemon + fix flaky test Aug 30, 2024

chillyvee force-pushed the cvdeamon branch 3 times, most recently from 68c0bbc to 59b56cd Compare August 30, 2024 19:36

julienrbrt assigned raynaudoe, julienrbrt and cool-develope Aug 30, 2024

julienrbrt enabled auto-merge August 30, 2024 21:46

raynaudoe reviewed Aug 31, 2024

View reviewed changes

tools/cosmovisor/scanner.go Outdated Show resolved Hide resolved

auto-merge was automatically disabled August 31, 2024 14:32
Head branch was pushed to by a user without write access

chillyvee requested a review from facundomedica as a code owner August 31, 2024 14:32

chillyvee force-pushed the cvdeamon branch 2 times, most recently from e651ac7 to 6db87b9 Compare August 31, 2024 14:35

chillyvee added 6 commits August 31, 2024 14:51

fix(cosmovisor): CV typo daemon

e218a2f

fix(cosmovisor): CV fix TestLaunchProcess

da62de7

fix(cosmovisor): CV do not delay reading upgrade-info file if file is…

d0d57d1

… filled

fix(cosmovisor): CV reduce PollInterval for fast dummyd halt

1fd9985

fix(cosmovisor): CV ShutdownGrace needs 1 second buffer to pass test …

a3faec5

…after flush

fix(cosmovisor): CV retry loop for empty upgrade-info.json

46b6405

chillyvee force-pushed the cvdeamon branch from 6db87b9 to 46b6405 Compare August 31, 2024 14:51

julienrbrt approved these changes Aug 31, 2024

View reviewed changes

julienrbrt enabled auto-merge August 31, 2024 21:15

Merge branch 'main' into cvdeamon

c7df382

coderabbitai bot reviewed Sep 1, 2024

View reviewed changes

julienrbrt requested a review from raynaudoe September 2, 2024 18:23

facundomedica reviewed Sep 3, 2024

View reviewed changes

facundomedica approved these changes Sep 3, 2024

View reviewed changes

julienrbrt added this pull request to the merge queue Sep 3, 2024

Merged via the queue into cosmos:main with commit e3d8378 Sep 3, 2024
74 of 75 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(cosmovisor): fix typo daemon + fix flaky test #21487

test(cosmovisor): fix typo daemon + fix flaky test #21487

chillyvee commented Aug 30, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Aug 30, 2024 •

edited

Loading

Walkthrough

Changes

Assessment against linked issues

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

chillyvee commented Aug 30, 2024

julienrbrt left a comment

chillyvee commented Aug 30, 2024 •

edited

Loading

coderabbitai bot left a comment

julienrbrt commented Aug 30, 2024

coderabbitai bot left a comment

chillyvee commented Aug 30, 2024

chillyvee commented Aug 30, 2024

julienrbrt commented Aug 30, 2024

julienrbrt commented Aug 30, 2024

chillyvee commented Aug 30, 2024

chillyvee commented Aug 30, 2024

chillyvee commented Aug 30, 2024

chillyvee commented Aug 31, 2024

chillyvee commented Aug 31, 2024

raynaudoe commented Aug 31, 2024

coderabbitai bot left a comment

facundomedica Sep 3, 2024

chillyvee Sep 3, 2024

test(cosmovisor): fix typo daemon + fix flaky test #21487

test(cosmovisor): fix typo daemon + fix flaky test #21487

Conversation

chillyvee commented Aug 30, 2024 • edited by coderabbitai bot Loading

Author Checklist

Reviewers Checklist

Summary by CodeRabbit

Summary by CodeRabbit

coderabbitai bot commented Aug 30, 2024 • edited Loading

Walkthrough

Changes

Assessment against linked issues

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

chillyvee commented Aug 30, 2024

julienrbrt left a comment

Choose a reason for hiding this comment

chillyvee commented Aug 30, 2024 • edited Loading

coderabbitai bot left a comment

Choose a reason for hiding this comment

julienrbrt commented Aug 30, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

chillyvee commented Aug 30, 2024

chillyvee commented Aug 30, 2024

julienrbrt commented Aug 30, 2024

julienrbrt commented Aug 30, 2024

chillyvee commented Aug 30, 2024

chillyvee commented Aug 30, 2024

chillyvee commented Aug 30, 2024

chillyvee commented Aug 31, 2024

chillyvee commented Aug 31, 2024

raynaudoe commented Aug 31, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

facundomedica Sep 3, 2024

Choose a reason for hiding this comment

chillyvee Sep 3, 2024

Choose a reason for hiding this comment

chillyvee commented Aug 30, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Aug 30, 2024 •

edited

Loading

chillyvee commented Aug 30, 2024 •

edited

Loading