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

Use the port file and dynamic port generation in client/server tests. #10604

Merged
merged 4 commits into from
Aug 18, 2021
Merged

Use the port file and dynamic port generation in client/server tests. #10604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6754283
Use the port file and dynamic port generation in client/server tests.
SamirTalwar Aug 17, 2021
c994961
Simplify the runner_with_port_file considerably.
SamirTalwar Aug 18, 2021
a3eb1b4
Simplify port file generation in the new client-server runner.
SamirTalwar Aug 18, 2021
a2fcb24
Simplify the runner_with_port_file further.
SamirTalwar Aug 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions bazel_tools/client_server/runner_with_port_file/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Copyright (c) 2021 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
# SPDX-License-Identifier: Apache-2.0

load("//bazel_tools:haskell.bzl", "da_haskell_binary")

da_haskell_binary(
name = "runner_with_port_file",
srcs = ["Main.hs"],
hackage_deps = [
"base",
"directory",
"extra",
"filepath",
"process",
"temporary",
],
visibility = ["//visibility:public"],
deps = ["//libs-haskell/da-hs-base"],
)
34 changes: 34 additions & 0 deletions bazel_tools/client_server/runner_with_port_file/Main.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
-- Copyright (c) 2021 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
-- SPDX-License-Identifier: Apache-2.0

module Main (main) where

import Control.Monad (unless)
import Data.List.Extra (replace, splitOn, stripInfix)
import Data.Maybe (isJust)
import System.Environment (getArgs)
import System.FilePath ((</>))
import System.Process (callProcess, proc, withCreateProcess)
import System.Exit (exitFailure)
import System.IO (hPutStrLn, stderr)
import System.IO.Temp (withSystemTempDirectory)

import DA.PortFile

main :: IO ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Taking the other feedback into account, about only waiting on the port-file, but not also the connection, how significantly does this runner still differ from //bazel_tools/client_server/runner?
That other runner also supports port 0 and a port-file, it also waits for the creation of that port-file.
One difference that stands out is that the other runner hard-codes the shape of the --port-file and --port arguments. If that's the only remaining difference, perhaps the other runner could be generalized to make these args configurable instead of adding an additional runner?

Copy link
Author

Choose a reason for hiding this comment

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

You're right, they're similar. My goal is to eventually merge all three runners together. However, I wanted to start small and just change a single target.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying. 👍 Makes sense.

main = do
[clientExe, clientArgs, serverExe, serverArgs, _] <- getArgs
let splitArgs = filter (/= "") . splitOn " "
let splitServerArgs = splitArgs serverArgs
let splitClientArgs = splitArgs clientArgs
unless (any (isJust . stripInfix "%PORT_FILE%") splitServerArgs) $ do
hPutStrLn stderr "No server parameters accept a port file."
exitFailure
withSystemTempDirectory "runner" $ \tempDir -> do
let portFile = tempDir </> "portfile"
let interpolatedServerArgs = map (replace "%PORT_FILE%" portFile) splitServerArgs
let serverProc = proc serverExe interpolatedServerArgs
withCreateProcess serverProc $ \_stdin _stdout _stderr _ph -> do
port <- readPortFile maxRetries portFile
let interpolatedClientArgs = map (replace "%PORT%" (show port)) splitClientArgs
callProcess clientExe interpolatedClientArgs
10 changes: 4 additions & 6 deletions ledger/participant-state/kvutils/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,6 @@ proto_jars(

REFERENCE_LEDGER_EXPORT_NAME = "reference-ledger-export"

REFERENCE_LEDGER_EXPORT_PORT = 65102
Copy link

Choose a reason for hiding this comment

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

Why not just move this out of emphemeral range and be done? Want to "solve it for once and for all" to avoid chance of conflict with other fixed ports?

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not sure what you’re suggesting? In this PR, @SamirTalwar-DA switched to using port 0 which choses a free open port so there is no chance of it colliding with anything.

Copy link

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but the collision observed in practice was likely due to the 65102 being taken by an outgoing connection, whereas if it were out of the ephemeral range (i.e., in 1025-32767) this, at least, would not happen. I just wanted to understand the motivation for going the extra mile.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t know how Samir ended up with the collision but generally just making sure that our own tests don’t conflict with each other is already painful and we have a lot of tests that cannot run in parallel with other tests exactly because they hardcode the same port number. I’m absolutely in favor of using port 0 everywhere and I think the complexity for this is justified.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I started by changing the port and then decided to go overboard. I am happy to go with the simpler change but my real intent with this is to use it across the conformance tests so they don't accidentally collide.

It really bugs me when I have a ledger running locally for testing and then try running some conformance tests, only to watch them fail because they picked the same default port.

Copy link

Choose a reason for hiding this comment

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

Right.. local running, parallel tests, oh my – thanks for the explanations. Now I understand a bit about the differences between testing here and in LR. :-)


# Generates a ledger export by running the test tool against a kvutils-based ledger.
client_server_build(
name = REFERENCE_LEDGER_EXPORT_NAME,
Expand All @@ -249,15 +247,15 @@ client_server_build(
client_args = [
"--concurrent-test-runs=4",
"--timeout-scale-factor=20",
"localhost:%d" % REFERENCE_LEDGER_EXPORT_PORT,
"localhost:%PORT%",
],
output_env = "KVUTILS_LEDGER_EXPORT",
runner = "@//bazel_tools/client_server/runner_with_port_check:runner",
runner_args = [str(REFERENCE_LEDGER_EXPORT_PORT)],
runner = "@//bazel_tools/client_server/runner_with_port_file",
runner_args = [],
server = "//ledger/ledger-on-memory:app",
server_args = [
"--contract-id-seeding=testing-weak",
"--participant=participant-id=%s,port=%d" % (REFERENCE_LEDGER_EXPORT_NAME, REFERENCE_LEDGER_EXPORT_PORT),
"--participant=participant-id=export,port=0,port-file=%PORT_FILE%",
],
visibility = [":__subpackages__"],
) if not is_windows else None
Expand Down