Skip to content

Conversation

@kavirajk
Copy link
Contributor

@kavirajk kavirajk commented Aug 8, 2022

The main rationale is for "choosing" random unbinded port for testing.

It's one of the Go's idiom to pass port number 0 to enforce Go's net's package to bind to some random port that is free.

example:

package main

import (
	"log"
	"net"
	"net/http"
	"time"

	"google.golang.org/grpc"
)

func main() {
	httpListener, err := net.Listen("tcp", "0.0.0.0:0")
	if err != nil {
		panic(err)
	}

	grpcListener, err := net.Listen("tcp", "0.0.0.0:0")
	if err != nil {
		panic(err)
	}

	h := http.Server{}
	go func() {
		log.Println("http serving at", httpListener.Addr())
		h.Serve(httpListener)
	}()

	g := grpc.Server{}
	go func() {
		log.Println("grpc serving at", grpcListener.Addr())
		g.Serve(grpcListener)
	}()

	time.Sleep(20 * time.Second)
}

And that will bind random "unassigned" port.

2022/08/08 16:51:49 grpc serving at [::]:35359
2022/08/08 16:51:49 http serving at [::]:44671

The problem is, currently there is no way to know what those ports are when using server.Server.

This PR exposes those addresses so that, we can start the test server easily and pass on the listen addresses to all it's clients.

Signed-off-by: Kaviraj kavirajkanagaraj@gmail.com

The main rationale is for "choosing" random unbinded port for testing.

It's one of the Go's idiom to pass port number `0` to enforce Go's net's package to bind to some random port that is free.

```
func main() {
    httpListener, err := net.Listen("tcp", "0.0.0.0:0")
    if err != nil {
        panic(err)
    }

    grpcListener, err := net.Listen("tcp", "0.0.0.0:0")
    if err != nil {
        panic(err)
    }

    h := http.Server{}
    go func() {
        log.Println("http serving at", httpListener.Addr())
        h.Serve(httpListener)
    }()

    g := grpc.Server{}
    go func() {
        log.Println("grpc serving at", grpcListener.Addr())
        g.Serve(grpcListener)
    }()

    time.Sleep(20 * time.Second)
}
```

And that will bind random "unassigned" port.

```
2022/08/08 16:51:49 grpc serving at [::]:35359
2022/08/08 16:51:49 http serving at [::]:44671
```

The problem is, currently there is no way to know what those ports are.

This PR exposes those addresses so that, we can test server easily and pass on the listen addresses to all it's clients.
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Copy link
Collaborator

@bboreham bboreham 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 this PR; it is very well done.

Small point; would you consider using the new methods here?

log.WithField("http", httpListener.Addr()).WithField("grpc", grpcListener.Addr()).Infof("server listening on addresses")

@bboreham
Copy link
Collaborator

bboreham commented Aug 9, 2022

Ah, never mind, the Server object isn't constructed till much later.

@kavirajk
Copy link
Contributor Author

kavirajk commented Aug 9, 2022

Thanks @bboreham for the review :)

can you also merge it? (I can happily vendor it on Loki then :))

@bboreham bboreham merged commit 72ba250 into weaveworks:master Aug 9, 2022
kavirajk added a commit to grafana/loki that referenced this pull request Aug 9, 2022
You cannot run promtail tests if there is already running promtail in your local.
e.g:
```
$ go test -run TestPromtail ./clients/pkg/promtail/
```

It gives you error
```
--- FAIL: TestPromtail (0.02s)
    promtail_test.go:112: error creating promtail listen tcp 127.0.0.1:9080: bind: address already in use
FAIL
FAIL	github.com/grafana/loki/clients/pkg/promtail	0.043s
```
Cause for the error is, test uses static default port (the same we use for promtail normally) in tests.

This is not **too** annonying if seen in isolation. But mainly annoying in two cases particularly
1. Hard to run tests parallely in general with these kind of static ports
2. These are causing some real problems in some integeration tests on GEL which starts docker-compose to run some test suites and
even stopping containers won't release ports soon enough making CI fail randomly.

This PR takes advantage of Go idiom of passing `port=0` to http (and also grpc) servers enforce Go's `net` package to bind some unused random ports. Making each test run choose random ports
enabling easy running theses tests in parallel.

NOTE: This PR also contains upgrade in `common/server` package that made this fix possible.
weaveworks/common#249

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
kavirajk added a commit to grafana/loki that referenced this pull request Aug 10, 2022
* test(promtail): Fix flaky test with promtail bind port.

You cannot run promtail tests if there is already running promtail in your local.
e.g:
```
$ go test -run TestPromtail ./clients/pkg/promtail/
```

It gives you error
```
--- FAIL: TestPromtail (0.02s)
    promtail_test.go:112: error creating promtail listen tcp 127.0.0.1:9080: bind: address already in use
FAIL
FAIL	github.com/grafana/loki/clients/pkg/promtail	0.043s
```
Cause for the error is, test uses static default port (the same we use for promtail normally) in tests.

This is not **too** annonying if seen in isolation. But mainly annoying in two cases particularly
1. Hard to run tests parallely in general with these kind of static ports
2. These are causing some real problems in some integeration tests on GEL which starts docker-compose to run some test suites and
even stopping containers won't release ports soon enough making CI fail randomly.

This PR takes advantage of Go idiom of passing `port=0` to http (and also grpc) servers enforce Go's `net` package to bind some unused random ports. Making each test run choose random ports
enabling easy running theses tests in parallel.

NOTE: This PR also contains upgrade in `common/server` package that made this fix possible.
weaveworks/common#249

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

* go mod tidy

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
lxwzy pushed a commit to lxwzy/loki that referenced this pull request Nov 7, 2022
* test(promtail): Fix flaky test with promtail bind port.

You cannot run promtail tests if there is already running promtail in your local.
e.g:
```
$ go test -run TestPromtail ./clients/pkg/promtail/
```

It gives you error
```
--- FAIL: TestPromtail (0.02s)
    promtail_test.go:112: error creating promtail listen tcp 127.0.0.1:9080: bind: address already in use
FAIL
FAIL	github.com/grafana/loki/clients/pkg/promtail	0.043s
```
Cause for the error is, test uses static default port (the same we use for promtail normally) in tests.

This is not **too** annonying if seen in isolation. But mainly annoying in two cases particularly
1. Hard to run tests parallely in general with these kind of static ports
2. These are causing some real problems in some integeration tests on GEL which starts docker-compose to run some test suites and
even stopping containers won't release ports soon enough making CI fail randomly.

This PR takes advantage of Go idiom of passing `port=0` to http (and also grpc) servers enforce Go's `net` package to bind some unused random ports. Making each test run choose random ports
enabling easy running theses tests in parallel.

NOTE: This PR also contains upgrade in `common/server` package that made this fix possible.
weaveworks/common#249

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

* go mod tidy

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants