test: make nodes also restart after upgrading in knuu #3556
Conversation
|
Currently dealing with this error: |
|
would there be any tradeoffs in running this in testnode? |
Outside of these three, I would think it's relatively similar |
cmwaters
added
the
warn:blocked
|
I may be mistaken, but I think this is blocked on #3505 and potential other changes that can avoid the port forwarding errors that arise |
cmwaters
requested review from
ramin and
staheri14
and removed request for
a team
WalkthroughThis update modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Logger
participant Application
User ->> Application: Calls MajorUpgradeToV2(logger)
Application ->> Logger: Logs Upgrade Initiation
Application ->> Application: Set upgradeHeight to 10
loop Upgrade Process
Application ->> Application: Perform upgrade steps
Application ->> Logger: Log Progress
end
Application ->> Application: Restart Nodes
Application ->> Logger: Log restart verification
Application ->> User: Returns upgrade completion status
Assessment against linked issues
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
This is passing for me locally now |
cmwaters
removed
the
warn:blocked
| @@ -60,6 +60,7 @@ func MajorUpgradeToV2(logger *log.Logger) error { | |||
|
|
|||
| heightBefore := upgradeHeight - 1 | |||
| for i := 0; i < numNodes; i++ { | |||
|
|
|||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // make all nodes in the network restart and ensure that progress is still made | ||
| for _, node := range testNet.Nodes() { | ||
| client, err := node.Client() | ||
| testnet.NoError("failed to get client", err) | ||
|
|
||
| height, err := getHeight(ctx, client, time.Minute) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get height: %w", err) | ||
| } | ||
|
|
||
| if err := node.Upgrade(latestVersion); err != nil { | ||
| return fmt.Errorf("failed to restart node: %w", err) | ||
| } | ||
|
|
||
| if err := waitForHeight(ctx, client, height+3, time.Minute); err != nil { | ||
| return fmt.Errorf("failed to wait for height: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider simplifying the loop for node restarts. Currently, it checks heights and restarts nodes sequentially, which could be parallelized for efficiency.
- for _, node := range testNet.Nodes() {
- client, err := node.Client()
- testnet.NoError("failed to get client", err)
- height, err := getHeight(ctx, client, time.Minute)
- if err != nil {
- return fmt.Errorf("failed to get height: %w", err)
- }
- if err := node.Upgrade(latestVersion); err != nil {
- return fmt.Errorf("failed to restart node: %w", err)
- }
- if err := waitForHeight(ctx, client, height+3, time.Minute); err != nil {
- return fmt.Errorf("failed to wait for height: %w", err)
- }
- }
+ // Parallelize node restarts for efficiency
+ errGroup := new(errgroup.Group)
+ for _, node := range testNet.Nodes() {
+ node := node // capture range variable
+ errGroup.Go(func() error {
+ client, err := node.Client()
+ if err != nil {
+ return fmt.Errorf("failed to get client: %w", err)
+ }
+ height, err := getHeight(ctx, client, time.Minute)
+ if err != nil {
+ return err
+ }
+ if err := node.Upgrade(latestVersion); err != nil {
+ return fmt.Errorf("failed to restart node: %w", err)
+ }
+ return waitForHeight(ctx, client, height+3, time.Minute)
+ })
+ }
+ if err := errGroup.Wait(); err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // make all nodes in the network restart and ensure that progress is still made | |
| for _, node := range testNet.Nodes() { | |
| client, err := node.Client() | |
| testnet.NoError("failed to get client", err) | |
| height, err := getHeight(ctx, client, time.Minute) | |
| if err != nil { | |
| return fmt.Errorf("failed to get height: %w", err) | |
| } | |
| if err := node.Upgrade(latestVersion); err != nil { | |
| return fmt.Errorf("failed to restart node: %w", err) | |
| } | |
| if err := waitForHeight(ctx, client, height+3, time.Minute); err != nil { | |
| return fmt.Errorf("failed to wait for height: %w", err) | |
| } | |
| } | |
| // Parallelize node restarts for efficiency | |
| errGroup := new(errgroup.Group) | |
| for _, node := range testNet.Nodes() { | |
| node := node // capture range variable | |
| errGroup.Go(func() error { | |
| client, err := node.Client() | |
| if err != nil { | |
| return fmt.Errorf("failed to get client: %w", err) | |
| } | |
| height, err := getHeight(ctx, client, time.Minute) | |
| if err != nil { | |
| return err | |
| } | |
| if err := node.Upgrade(latestVersion); err != nil { | |
| return fmt.Errorf("failed to restart node: %w", err) | |
| } | |
| return waitForHeight(ctx, client, height+3, time.Minute) | |
| }) | |
| } | |
| if err := errGroup.Wait(); err != nil { | |
| return err | |
| } |
| } | ||
|
|
||
| if err := node.Upgrade(latestVersion); err != nil { | ||
| return fmt.Errorf("failed to restart node: %w", err) |
There was a problem hiding this comment.
[optional]
| return fmt.Errorf("failed to restart node: %w", err) | |
| return fmt.Errorf("failed to upgrade node: %w", err) |
There was a problem hiding this comment.
I know the function says upgrade, but it's actually upgrading to the same version, effectively a restart. It could be cleaner if we just have a Restart method in testnet that does the same thing
There was a problem hiding this comment.
+1 restart would be clearer in that case
Closes: #3337