test: v3 e2e upgrade #3910
test: v3 e2e upgrade #3910
Conversation
@evan-forbes @mojtaba-esk have either of you hit this? |
yes |
|
Sysrex added traefik API to the Robusta cluster so this is now unblocked. The test hangs for me though: Update: it didn't hang, it just took forever to complete. |
|
Sure! This PR isn't ready and #3911 is ready. |
|
I wasn't aware of this problem when testing. The last problem I experienced with this test is that the txsim wasn't able to execute the |
evan-forbes
added
WS: V3 3️⃣
|
Note to self: can see logs for txsim by looking in Lens admin@k8s-knuu-1 and then filtering to namespace like: txsim logs say: I'm considering adding a post-hook so that the validators can submit a signal message instead of requiring txSim to do that. Update: I don't think this test is using the new txSim binary b/c the |
which makes sense because it's trying to send authored blobs before the upgrade from v2 -> v3 happens. |
|
I hit the error @cmwaters described |
|
Some debug logs: Looks like validator keys don't get added. Validator instances get a volume |
|
Still didn't work: Update: I don't see any logic to copy the keys from the validators to the txSim nodes. |
rootulp
requested review from
rach-id and
ninabarbakadze
and removed request for
a team
|
For posterity the two issues I found and fixed were:
|
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve the introduction of a new file Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🔇 Additional comments (5)
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (14)
test/e2e/testnet/key_generator.go (2)
16-20: Consider adding an explanatory comment for using math/rand.The function correctly initializes the keyGenerator with a seeded random number generator. However, it might be beneficial to add a comment explaining why
math/randis used instead ofcrypto/rand. This would clarify the intentional use of a deterministic random number generator for testing purposes.Consider adding a comment like this:
// We use math/rand instead of crypto/rand to ensure deterministic key generation for testing purposes.
22-37: LGTM: Well-implemented Generate method with a minor suggestion.The Generate method is well-implemented, correctly handling the supported key types and including appropriate error handling. The use of panic for unsupported key types is acceptable in this context.
Consider adding a comment to document that an empty string for keyType defaults to ed25519. This could improve clarity for users of this method. For example:
// Generate creates a private key of the specified type. // If keyType is an empty string, it defaults to "ed25519". func (g *keyGenerator) Generate(keyType string) crypto.PrivKey { // ... existing implementation ... }test/e2e/minor_version_compatibility.go (1)
66-67: LGTM. Consider adding a comment for clarity.The addition of the
upgradeScheduleparameter to theCreateTxClientmethod call is a good improvement, allowing for future flexibility in upgrade testing.Consider adding a brief comment explaining the purpose of the
upgradeScheduleparameter, especially since it's currently initialized as an empty map. This would improve code readability and maintainability. For example:+ // Initialize an empty upgrade schedule. This can be populated in the future to test specific upgrade scenarios. upgradeSchedule := map[int64]uint64{}test/e2e/major_upgrade_v2.go (1)
55-56: Approve the addition ofupgradeSchedule, but suggest improvementsThe addition of the
upgradeScheduleparameter toCreateTxClientis a good step towards implementing upgrade tests. However, there are a few points to consider:
The
upgradeSchedulemap is currently empty. Consider populating it with relevant upgrade information for v2 and v3.The function name
MajorUpgradeToV2suggests this test is for v2, but the PR objectives mention implementing a v3 upgrade test. Consider either:
a) Renaming this function toMajorUpgradeToV3if it's meant to test v3 upgrades, or
b) Creating a new functionMajorUpgradeToV3that builds upon this one for v3 testing.To fully address the PR objectives, expand this test (or create a new one) to cover v3 upgrades and future signaling-based upgrades. This may involve adding logic to handle different upgrade versions and incorporating signaling mechanisms.
Would you like assistance in implementing these suggestions?
test/e2e/benchmark/benchmark.go (1)
59-68: Improved readability and added upgrade schedule support. LGTM!The restructuring of the
CreateTxClientsmethod call enhances code readability. The addition of themap[int64]uint64{}parameter for the upgrade schedule is a good preparation for future upgrade testing.Consider adding a comment explaining the purpose of the empty upgrade schedule map for better clarity:
b.manifest.TxClientsResource, gRPCEndpoints, - map[int64]uint64{}, // upgrade schedule + map[int64]uint64{}, // upgrade schedule (empty for now, can be populated for future upgrade tests) )test/cmd/txsim/cli.go (1)
132-137: LGTM: Enhanced blob sequence creation with share version support.The changes allow for optional specification of a share version for blob sequences, which is a valuable addition. The condition
blobShareVersion >= 0ensures that the share version is only set when explicitly specified.Consider adding a check to ensure that
blobShareVersiondoesn't exceed the maximum allowed value for auint8. For example:if blobShareVersion >= 0 { + if blobShareVersion > 255 { + return fmt.Errorf("blob share version must be between 0 and 255, got %d", blobShareVersion) + } sequence.WithShareVersion(uint8(blobShareVersion)) }This would prevent potential issues with overflow when converting to
uint8.test/e2e/testnet/node.go (2)
100-107: LGTM: Improved function signature with a suggestionThe reorganization of parameters and the renaming of
upgradeHeighttoupgradeHeightV2enhance code clarity and specificity. These changes improve the overall readability of the function signature.Consider adding a comment to explain the purpose of
upgradeHeightV2for better documentation.
163-164: LGTM: Updated upgrade height conditionThe condition has been correctly updated to use the renamed
upgradeHeightV2parameter. This change maintains consistency with the function signature update.Consider adding a comment to explain the significance of the V2 upgrade and why this condition is necessary.
test/e2e/testnet/txsimNode.go (1)
89-93: Review argument logging for sensitive informationWhile logging the
argsarray can be helpful for debugging, ensure that it does not contain sensitive information such as secrets or private keys. Currently, the arguments seem safe, but it's good practice to audit logged data for potential security risks.test/txsim/upgrade.go (1)
83-83: Update comment to reflect 'signalled' instead of 'voted'The comment refers to validators having voted, but the code now tracks whether validators have signalled. Update the comment for consistency and clarity.
Apply this diff to correct the comment:
-// if all validators have voted, we can now try to upgrade. +// if all validators have signalled, we can now try to upgrade.test/e2e/major_upgrade_v3.go (3)
27-29: Replace 'HACKHACK' with standard comment markers.The use of
// HACKHACK:is unconventional and may be confusing to other developers. Consider using standard comment markers likeTODO,FIXME, orNOTEto indicate temporary code or important notes.Update the comment as follows:
// TODO: Use the main branch version after the PR is merged.
31-31: Format the log message for clarity.Passing multiple arguments to
logger.Printlnwill concatenate them with spaces, which might not produce the intended log message structure.Consider formatting the log message to clearly display the version information:
logger.Println(fmt.Sprintf("Running major upgrade to v3 test, version: %s", version))Or, if using a structured logger that supports key-value pairs:
logger.Println("Running major upgrade to v3 test", "version", version)Ensure that the logging library correctly handles key-value pairs if you choose the latter option.
87-87: Uselogger.Printlninstead offmt.Printlnfor consistency.Using
fmt.Printlnfor logging deviates from the logging pattern used throughout the file. For consistent logging and potentially better log management, uselogger.Println.Update the line to:
logger.Println("height", resp.Header.Height)test/e2e/testnet/testnet.go (1)
181-182: Use structured logging instead offmt.PrintfThe line
fmt.Printf("nodeName %v\n", nodeName)uses standard output for logging, which may not be consistent with the rest of the application's logging strategy. For consistency and better log management, consider using thelogpackage.Replace with:
log.Info().Str("nodeName", nodeName).Msg("Processing node")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
- pkg/appconsts/global_consts.go (1 hunks)
- test/cmd/txsim/cli.go (4 hunks)
- test/e2e/benchmark/benchmark.go (1 hunks)
- test/e2e/main.go (1 hunks)
- test/e2e/major_upgrade_v2.go (1 hunks)
- test/e2e/major_upgrade_v3.go (1 hunks)
- test/e2e/minor_version_compatibility.go (1 hunks)
- test/e2e/simple.go (1 hunks)
- test/e2e/testnet/key_generator.go (1 hunks)
- test/e2e/testnet/node.go (4 hunks)
- test/e2e/testnet/testnet.go (8 hunks)
- test/e2e/testnet/txsimNode.go (3 hunks)
- test/e2e/testnet/util.go (0 hunks)
- test/txsim/upgrade.go (2 hunks)
💤 Files with no reviewable changes (1)
- test/e2e/testnet/util.go
🧰 Additional context used
🔇 Additional comments (19)
test/e2e/testnet/key_generator.go (2)
3-10: LGTM: Imports are appropriate and necessary.The imported packages are relevant to the functionality of the key generator. No unused imports are present.
12-14: LGTM: keyGenerator struct is well-defined.The struct is simple and focused, containing only the necessary field for its purpose. Using a custom random number generator allows for deterministic key generation, which is beneficial for testing scenarios.
test/e2e/main.go (2)
Line range hint
1-71: Summary: Successful integration of new test case.The addition of the "MajorUpgradeToV3" test case to the
testsslice is the only change in this file. This modification aligns well with the PR objectives of implementing an e2e upgrade test for v3. The existing structure for running tests, including specific test selection and error handling, remains intact. This change effectively expands the testing capabilities without disrupting the current functionality.To ensure comprehensive testing, make sure that:
- The
MajorUpgradeToV3function is properly implemented in the appropriate file.- Any necessary setup or teardown procedures for this new test are in place.
- The new test is included in any relevant CI/CD pipelines or testing scripts.
26-26: LGTM: New test case added correctly.The addition of the "MajorUpgradeToV3" test case is consistent with the existing structure and follows the pattern of other test cases. This is a good step towards implementing the e2e upgrade test for v3 as outlined in the PR objectives.
To ensure the
MajorUpgradeToV3function is properly defined, please run the following script:Consider updating any relevant documentation or README files to reflect the addition of this new test case, if not already done.
✅ Verification successful
Verification Successful:
MajorUpgradeToV3function exists.The
MajorUpgradeToV3function is properly defined intest/e2e/major_upgrade_v3.go, confirming that the new test case integrates correctly with the codebase.Consider updating any relevant documentation or README files to reflect the addition of this new test case, if not already done.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the MajorUpgradeToV3 function # Test: Search for the MajorUpgradeToV3 function definition rg --type go "func MajorUpgradeToV3\(" -A 5Length of output: 388
test/e2e/simple.go (1)
38-39: LGTM! VerifyCreateTxClientusage across the codebase.The addition of the
upgradeScheduleparameter to theCreateTxClientmethod call is appropriate for implementing upgrade testing capabilities. The empty map is suitable for this simple E2E test where no specific upgrade is scheduled.To ensure consistency, please verify that all other calls to
CreateTxClientin the codebase have been updated with the new parameter. Run the following script to check for any inconsistencies:✅ Verification successful
Verified: All
CreateTxClientusages include theupgradeScheduleparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for inconsistent usage of CreateTxClient across the codebase # Search for CreateTxClient calls rg --type go -A 5 'CreateTxClient\('Length of output: 3556
pkg/appconsts/global_consts.go (1)
33-36:⚠️ Potential issueRevert temporary override and implement a configurable solution
The current change overrides
DefaultUpgradeHeightDelaywith a value of 1 for e2e testing purposes. This approach is risky as it could accidentally be merged into production, potentially causing issues with the upgrade process.As suggested in a previous review, please consider the following approach:
- Revert the temporary override.
- Implement a configurable solution using build flags or environment variables.
Here's a potential implementation:
var DefaultUpgradeHeightDelay int64 func init() { DefaultUpgradeHeightDelay = 7 * 24 * 60 * 60 / 12 // Default 7-day delay if delayStr := os.Getenv("UPGRADE_HEIGHT_DELAY"); delayStr != "" { if delay, err := strconv.ParseInt(delayStr, 10, 64); err == nil { DefaultUpgradeHeightDelay = delay } } }This approach allows you to easily override the delay for testing purposes without risking accidental merges of test-specific values into production code.
To ensure this constant isn't being overridden elsewhere, run:
#!/bin/bash # Search for other occurrences of DefaultUpgradeHeightDelay assignment rg --type go 'DefaultUpgradeHeightDelay\s*=' -g '!pkg/appconsts/global_consts.go'test/cmd/txsim/cli.go (4)
44-44: LGTM: New variable for blob share version.The addition of
blobShareVersionas a global variable is appropriate for CLI flags and enhances the flexibility of blob sequence configuration.
106-107: LGTM: Updated error message for clarity.The error message now correctly includes
--upgrade-scheduleas a valid option, providing users with complete information about available sequence types.
219-219: LGTM: New CLI flag for blob share version.The addition of the
--blob-share-versionflag is well-implemented. The default value of -1 aligns with the logic in the blob sequence creation, and the description clearly explains its purpose.
Line range hint
1-270: Overall assessment: Well-implemented enhancements to the transaction simulator.The changes in this file successfully enhance the functionality of the transaction simulator by allowing more detailed configuration of blob sequences, particularly through the addition of the blob share version feature. These modifications align well with the PR objectives of improving testing capabilities for major upgrades.
Key improvements:
- Introduction of the
blobShareVersionvariable and corresponding CLI flag.- Enhanced blob sequence creation logic to incorporate the share version.
- Updated error messages for better user feedback.
The changes are well-implemented, with only a minor suggestion for additional error handling in the blob share version setting. These enhancements contribute positively to the testing framework's robustness and flexibility.
test/e2e/testnet/node.go (3)
62-62: LGTM: Improved method signatureThe removal of the trailing comma in the method signature is a good cleanup. It improves code consistency and adheres to Go's syntax conventions.
75-75: LGTM: Improved method signatureSimilar to the previous change, the removal of the trailing comma in this method signature enhances code consistency and adheres to Go's syntax conventions.
Line range hint
462-462: Please clarify the change in stake calculationThe
Stakefield is now set to half of theSelfDelegationamount (n.SelfDelegation / 2). This is a significant change that could affect the validator's voting power and potentially impact network security.Could you please provide more context on:
- The rationale behind this change?
- Any potential impacts on the network's security or consensus?
- Whether this change is part of a broader update to the staking mechanism?
To assess the impact of this change, let's check for other occurrences of stake calculations in the codebase:
✅ Verification successful
Verified Stake Calculation Update
The modification to set
Staketon.SelfDelegation / 2is consistent with existing implementations, such as intest/util/genesis/accounts.go, whereStakeis set toDefaultInitialBalance / 2to reserve tokens for fees. This indicates a deliberate strategy to allocate half of the tokens for staking purposes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for stake calculations in Go files rg --type go 'Stake\s*[:=]' -C 3Length of output: 1883
test/e2e/testnet/txsimNode.go (1)
69-69: Confirm correct UID for volume ownershipWhen adding the volume with
AddVolumeWithOwner, the UID10001is specified. Verify that this UID matches the user inside thetxsimcontainer that needs access to the volume. Incorrect ownership may lead to permission issues during runtime.You can check the UID of the user inside the container by inspecting the Dockerfile or running the following command on a running container:
#!/bin/bash # Description: Check the UID of the user inside the container. # Expected: The UID should match the one specified in `AddVolumeWithOwner`. # Replace <container_name> with the actual container name or ID. docker exec <container_name> id -utest/e2e/testnet/testnet.go (5)
476-479: Addition ofGenesis()method toTestnetstructThe new method
Genesis()provides access to thegenesisfield of theTestnetstruct. This enhances encapsulation and allows other components to access genesis-related data more cleanly.
83-86: Ensure all invocations ofCreateGenesisNodeuseupgradeHeightV2The parameter
upgradeHeighthas been renamed toupgradeHeightV2in theCreateGenesisNodemethod. Verify that all calls to this method have been updated accordingly to prevent incorrect parameter assignment.Use the following script to detect any outdated calls:
#!/bin/bash # Description: Find calls to CreateGenesisNode using the old parameter name rg --type go 'CreateGenesisNode\([^\)]*upgradeHeight[^\)]*\)'
113-117: Update calls toCreateTxClientswith the newupgradeScheduleparameterThe
CreateTxClientsmethod now includes an additional parameterupgradeSchedule map[int64]uint64. Ensure that all calls to this method provide the new parameter to maintain expected functionality.Run the following script to find and review calls to
CreateTxClients:#!/bin/bash # Description: Find all instances of CreateTxClients and display surrounding lines for context rg --type go -A 5 'CreateTxClients\('
97-99: Ensure all invocations ofCreateGenesisNodesuseupgradeHeightV2The parameter
upgradeHeighthas been renamed toupgradeHeightV2in theCreateGenesisNodesmethod. Verify that all calls to this method have been updated to match the new parameter name.Use the following script to detect any outdated calls:
#!/bin/bash # Description: Find calls to CreateGenesisNodes using the old parameter name rg --type go 'CreateGenesisNodes\([^\)]*upgradeHeight[^\)]*\)'
132-149: UpdateCreateTxClientmethod calls with new parametersThe
CreateTxClientmethod signature has changed:
sequencesis renamed toblobSequences- Added a new parameter
upgradeSchedule map[int64]uint64Ensure all calls to
CreateTxClientare updated to reflect these changes.Run the following script to identify outdated calls:
#!/bin/bash # Description: Find all calls to CreateTxClient and display surrounding lines for context rg --type go -A 10 'CreateTxClient\('
| name string, | ||
| version string, | ||
| endpoint string, | ||
| seed int64, | ||
| sequences int, | ||
| blobSequences int, | ||
| blobRange string, | ||
| blobsPerSeq int, | ||
| pollTime int, | ||
| resources Resources, | ||
| volumePath string, | ||
| knuu *knuu.Knuu, | ||
| upgradeSchedule map[int64]uint64, |
There was a problem hiding this comment.
💡 Codebase verification
Update Required for CreateTxClient Invocation in testnet.go
test/e2e/testnet/testnet.go:117uses the outdated parametersequences. It should be renamed toblobSequencesto match the updated function signature.
🔗 Analysis chain
Ensure all callers of CreateTxClient are updated to match the new signature
The function CreateTxClient has been updated with a new parameter upgradeSchedule and the parameter sequences has been renamed to blobSequences. To maintain consistency and prevent compilation errors, verify that all invocations of CreateTxClient across the codebase are updated to reflect these changes.
Run the following script to locate all usages of CreateTxClient and check for outdated parameter lists:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to `CreateTxClient` and display their parameter lists.
# Expected: All calls should use the updated parameters.
rg --type go 'CreateTxClient\(' -A 5
Length of output: 106
Script:
#!/bin/bash
# Description: Find all calls to `CreateTxClient` and display their parameter lists.
# This script searches through Go files to locate `CreateTxClient` invocations.
rg --type go 'CreateTxClient\(([^)]*)\)' --with-filename --line-number
Length of output: 1123
| testnet.NoError("failed to get header", err) | ||
| if resp.Header.Version.App == v3.Version { |
There was a problem hiding this comment.
Ensure error handling after retrieving the header.
When calling client.Header(ctx, nil), if an error occurs, accessing resp.Header may result in a nil pointer dereference. This can cause the program to panic.
Modify the code to handle the error appropriately:
resp, err := client.Header(ctx, nil)
if err != nil {
testnet.NoError("failed to get header", err)
continue
}Make sure that resp is not nil before accessing its fields.
test/e2e/major_upgrade_v3.go
Outdated
| logger.Println("waiting for upgrade") | ||
| for _, node := range testNet.Nodes() { | ||
| client, err := node.Client() | ||
| testnet.NoError("failed to get client", err) | ||
|
|
||
| upgradeComplete := false | ||
| lastHeight := int64(0) | ||
| for !upgradeComplete { | ||
| select { | ||
| case <-timer.C: | ||
| return fmt.Errorf("failed to upgrade to v3, last height: %d", lastHeight) | ||
| case <-ticker.C: | ||
| resp, err := client.Header(ctx, nil) | ||
| testnet.NoError("failed to get header", err) | ||
| if resp.Header.Version.App == v3.Version { | ||
| upgradeComplete = true | ||
| } | ||
| fmt.Println("height", resp.Header.Height) | ||
| lastHeight = resp.Header.Height | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle timeouts and errors for each node individually.
The current implementation shares the same timer across all nodes. If one node delays the upgrade, it could cause the entire test to fail, even if others have succeeded.
Consider refactoring the code to handle timeouts and errors on a per-node basis to isolate issues and provide more detailed feedback.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
test/e2e/testnet/testnet.go (1)
474-477: Add Documentation for the NewGenesisMethodThe
Genesismethod provides access to thegenesisfield but lacks a documentation comment. Adding a comment will improve code readability and help other developers understand its purpose.Apply this diff to add the documentation:
+// Genesis returns the genesis configuration for the testnet. func (t *Testnet) Genesis() *genesis.Genesis { return t.genesis }
There was a problem hiding this comment.
When I try to run the test make test-e2e MajorUpgradeToV3, validators spun up just fine but the txCleint fails with this log:
Starting txsim with command:
/bin/txsim --key-path /var/folders/gl/8f7g64t91rxc6hsz_gj0mqfh0000gn/T/txsim --grpc-endpoint 10.38.154.62:9090 --poll-time 1s --seed 42 --blob 1 --blob-amounts 100 --blob-sizes 100-2000 --upgrade-schedule 20:3 --blob-share-version 0
Error: no accounts found in keyring
Did you encounter a similar issue?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
test/e2e/testnet/testnet.go (1)
174-174: Consider Making Account Balance ConfigurableThe account balance is hardcoded as
1e16. To improve flexibility and maintainability, consider making the balance a configurable parameter or defining it as a constant.For example, define a constant for the balance:
const defaultAccountBalance = 1e16And update the code:
err = t.genesis.AddAccount(genesis.Account{ PubKey: pk, - Balance: 1e16, + Balance: defaultAccountBalance, Name: name, })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- test/e2e/testnet/testnet.go (8 hunks)
🧰 Additional context used
🔇 Additional comments (2)
test/e2e/testnet/testnet.go (2)
132-143: Well-Documented Function ParametersThe
CreateTxClientfunction now includes clear and detailed parameter documentation in the preceding comment block. This enhances readability and adheres to Go's documentation standards.
479-481: Well-ImplementedGenesisAccessor MethodThe addition of the
Genesis()method provides controlled access to thegenesisfield of theTestnetstruct. This adheres to good encapsulation practices and enhances the modularity of the code.
I encountered that on an earlier commit of this PR but thought it was fixed after subsequent commits. Hmm I can repro on the most recent commit though. |
rootulp
requested review from
rootulp,
staheri14 and
evan-forbes
and removed request for
rootulp
Closes #3772
Opens #3947
Testing