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

itest: small fixes for tapd load test #869

Merged
merged 1 commit into from
Apr 9, 2024
Merged

itest: small fixes for tapd load test #869

merged 1 commit into from
Apr 9, 2024

Conversation

calvinrzachman
Copy link
Contributor

When building images off the main branch, I found that the load test pod was incapable of correctly determining the bitcoind backend version:

➜  ✗ kubectl logs -f tapd-loadtest-orchestrator-job-11-zm76d
=== RUN   TestPerformance
=== RUN   TestPerformance/mint
=== NAME  TestPerformance
    utils.go:149:
        	Error Trace:	/app/itest/utils.go:149
        	            				/app/itest/loadtest/utils.go:121
        	            				/app/itest/loadtest/mint_batch_test.go:28
        	            				/app/itest/loadtest/load_test.go:57
        	Error:      	unknown chain backend: %v
        	Test:       	TestPerformance
        	Messages:   	bitcoind v25.0.0 and above
=== NAME  TestPerformance/mint
    testing.go:1490: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test
--- FAIL: TestPerformance (0.18s)
    --- FAIL: TestPerformance/mint (0.18s)
FAIL

Also, encountered the following error:

Error:      	Error "rpc error: code = Unknown desc = cannot add ourselves as a federation member" does not contain "universe server already added"

We used to run the following in the mint test:

_, err = bob.AddFederationServer(
    ctx, &unirpc.AddFederationServerRequest{
        Servers: []*unirpc.UniverseFederationServer{
            {
                Host: aliceHost,
            },
        },
    },
)

So we can just update the itest.SyncUniverses(...) call to maintain this behavior.

jharveyb
jharveyb reviewed Apr 3, 2024
View reviewed changes
itest/loadtest/mint_batch_test.go
@@ -164,5 +164,5 @@ func mintTest(t *testing.T, ctx context.Context, cfg *Config) {
})
require.True(t, correctOp)

itest.SyncUniverses(ctx, t, alice, bob, aliceHost, cfg.TestTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should actually be bobHost and not aliceHost? IIUC alice is the client trying to sync to bob.

Copy link
Member

Choose a reason for hiding this comment

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

No, I think this is correct. It was wrong before. Bob should be the client that syncs to Alice and for that it needs Alice's host address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There may be more than one correct way to set it up. The old mint test used to have bob add alice as a federation server using bob.AddFederationServer(... something <aliceHost> ...), so I was just reverting to that approach. We could probably also get away with having alice add bob as the federation server.

Does the directionality of who adds whom matter? Alice is the node doing all the minting the way the tests are setup now.

Copy link
Member

Choose a reason for hiding this comment

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

Directionality matters, yes. Because a sync currently is only pull. So Bob will fetch everything that Alice has. But if Bob has something that Alice doesn't I don't think that's currently pushed in the other direction.

@guggero guggero self-requested a review April 4, 2024 09:23
guggero
guggero reviewed Apr 4, 2024
View reviewed changes
Copy link
Member

@guggero guggero 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 the fixes. Tested them both locally and with a slight adjustment (see inline comment), this seems to work.

itest/utils.go Outdated
@@ -158,7 +158,7 @@ func MineBlocks(t *testing.T, client *rpcclient.Client,
)
require.NoError(t, err)

case rpcclient.BtcdVersion:
case *rpcclient.BtcdVersion:
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this is NOT a pointer, while the BitcoindVersion is (see itest that's currently broken). This is very very weird, no idea why. But I tested it locally with the load test and integration test, and it looks like that's really the case...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to log the type to see that it was a pointer in the bitcoind case. Just assumed it would also be for the btcd case. That is strange.

itest/loadtest/mint_batch_test.go
@@ -164,5 +164,5 @@ func mintTest(t *testing.T, ctx context.Context, cfg *Config) {
})
require.True(t, correctOp)

itest.SyncUniverses(ctx, t, alice, bob, aliceHost, cfg.TestTimeout)
Copy link
Member

Choose a reason for hiding this comment

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

No, I think this is correct. It was wrong before. Bob should be the client that syncs to Alice and for that it needs Alice's host address.

guggero
guggero approved these changes Apr 8, 2024
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM 🎉

itest/utils.go
@@ -147,7 +147,7 @@ func MineBlocks(t *testing.T, client *rpcclient.Client,
var blockHashes []*chainhash.Hash

switch backend.(type) {
case rpcclient.BitcoindVersion:
case *rpcclient.BitcoindVersion:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add a comment here? Otherwise someone looking at this will definitely think one of them is wrong...

Copy link
Member

Choose a reason for hiding this comment

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

aren't both meant to be pointers?

Copy link
Member

Choose a reason for hiding this comment

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

Nope, apparently they're not, which is very weird...

GeorgeTsagk
GeorgeTsagk approved these changes Apr 9, 2024
View reviewed changes
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

LGTM, tACK

@guggero guggero added this pull request to the merge queue Apr 9, 2024
Merged via the queue into lightninglabs:main with commit 39bbc18 Apr 9, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeorgeTsagk GeorgeTsagk GeorgeTsagk approved these changes

@jharveyb jharveyb jharveyb left review comments

@guggero guggero guggero approved these changes

Assignees
No one assigned
Labels
performance testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@calvinrzachman @guggero @jharveyb @GeorgeTsagk @dstadulis