BUG/MINOR: prevents port conflicts on startup when using hostNetwork #568
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fabianonunes
force-pushed
the
avoid-port-conflicts-host-mode
branch
3 times, most recently
from
a51e3c7 to
4c7efe3
Compare
|
Hi @fabianonunes , I was able to make the controller start even with a 8080 already occupied just by simply removing the bindings from the two frontends. Can you elaborate on the further changes in your PR ? For example, why removing the peer ? Why replacing the edit function by their creation counterpart ? For semantic consistency ? |
|
Hi, Ivan!
Well spotted. It seems that localpeer isn't activated during the initial HAProxy startup, so there's no need for concern. I'll remove these changes from PR. We only need remove from the frontends. Since we can have other service listening to the ports 1042 (healthz) and 1024 (stats), these should be removed too from initial config. We can change these ports with the CLI flags To reproduce this conflict, run a process on host listening on port 1042. Then, run haproxy-controller with the flag |
fabianonunes
force-pushed
the
avoid-port-conflicts-host-mode
branch
from
4c7efe3 to
fd4bd0f
Compare
|
Can you recheck your PR because it has a failed test:
|
|
@ivanmatmati, can you run the test again? I couldn't reproduce this bug. On my fork, on the same commit, I've repeated the tests twice without errors.
|
|
I couldn't reproduce it locally either: (these sections are collapsible. click on the title to expand) $ deploy/tests/create.sh$ go clean -testcache$ go test ./... -v --tags=e2e_parallel --tags=e2e_https$ echo $?$ go test ./... -v -p 1 --tags=e2e_sequential$ echo $? |
fabianonunes
force-pushed
the
avoid-port-conflicts-host-mode
branch
2 times, most recently
from
306c496 to
de65bf9
Compare
|
Hi, @ivanmatmati. I've identified the error. Between my fork and my PR, a commit 247199b added a new haproxyConfig in base-suite.go for use in other tests. This explains why my fork was passing while it's failing here. Sorry. |
Creates new binds (instead of editing them) of the healthz and stats frontends to avoid port conflicts at the first start of the HAProxy when using hostNetwork: true. Since these bind directives are not required, the first start of HAProxy is not affected.
fabianonunes
force-pushed
the
avoid-port-conflicts-host-mode
branch
from
de65bf9 to
85445b3
Compare
|
Good, we can proceed now. |
|
Hi, @ivanmatmati. Is there anything I can do to help you with this issue? |
|
No, it's ok we're about to merge your PR. Thanks. |
|
@fabianonunes thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes #566.
Creates new binds (instead of editing them) of the
healthzandstatsfrontends to avoid port conflicts at the first start of the HAProxy when usinghostNetwork: true.Since these bind directives are not required, the first start of HAProxy is not affected.
The
httpandhttpsfrontends were already being created before this PR.