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.

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

Revert "chore: updated to HAproxy 3.0 and forced running as root" #136

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

proudier
Copy link
Contributor

@proudier proudier commented Aug 21, 2024

Reverts #130

Given the issue #132 and the lack of activity on haproxy/haproxy#2684, let's revert this PR

@pedrobaeza @josep-tecnativa

@josep-tecnativa
Copy link
Contributor

Perhaps it needs to be updated with the command poetry update so that the checks pass successfully.

@josep-tecnativa
Copy link
Contributor

ping @proudier

@proudier
Copy link
Contributor Author

proudier commented Sep 9, 2024

(sorry for the delay)

Issue with the test suite

I think there is an issue with the test suite. I ran it 40 times on my machine and all the runs passed (see script below). Also, this MR reverts 100% of the original MR (#130), so anything #130 might have broken would be unbroken...

I ran the same script against v1.2.0: 10 out of 40 runs passed. It should be either 0 or 40. Moreover, it's not always the same test that fails, and sometimes more than one test fails. This randomness tends to confirm we have an issue with the test suite.

Error analysis

The errors reported by the failed tests are of two types, similar to those:

  • error during connect: Get "http://localhost:49350/v1.47/version": read tcp [::1]:62552->[::1]:49350: read: connection reset by peer
  • error during connect: Get "http://localhost:49364/v1.47/version": EOF

My guess is that the "docker create" (in proxy_factory) returns before the container is actually available on the network. After all, many software components are involved: docker daemon, {ip,nft}tables, linux bridge, ...

Proposal

I ran the script against v1.2.0 with a change in conftest.py: a pause after the creation of the container -> All runs passed (used to be 10 out of 40)!

Im aware a hardcoded pause in a test suite is not an elegant approach but

  • the test suite is still prety fast (4.6 sec on average with the pause)
  • having a flaky test suite is horrible
  • fixing the docker daemon is waaaay out of scope
  • it's easy to revert if/when someone has a better approach

Next steps

What do you think about all this?

I could send a new MR with the sleep fix (+poetry deps update while at it) and rebase this MR once it's former is merged

Test script

# gitref=7caf185
# 7caf185 is the revert MR, with no poetry update

# Preparation
# !!!! BEWARE of the `docker system prune` !!!!
git reset --hard $gitref
poetry install --no-root
docker rmi test:docker-socket-proxy
docker system prune -f

# Run 40 test passes
for i in {1..40}; do
    poetry run pytest --prebuild > "$gitref-test-output-$i"
    sleep 2
done

successes=$(grep '4 passed' $gitref-test-output-* | wc -l)

@josep-tecnativa
Copy link
Contributor

It's OK for me.

@josep-tecnativa
Copy link
Contributor

Now we can merge this @pedrobaeza

@pedrobaeza pedrobaeza merged commit 393a99c into Tecnativa:master Sep 9, 2024
3 checks passed
jamesdkelly88 pushed a commit to jamesdkelly88/db-lab that referenced this pull request Sep 9, 2024
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
|
[ghcr.io/tecnativa/docker-socket-proxy](https://redirect.github.com/Tecnativa/docker-socket-proxy)
| minor | `0.2.0` -> `0.3.0` |

---

### Release Notes

<details>
<summary>Tecnativa/docker-socket-proxy
(ghcr.io/tecnativa/docker-socket-proxy)</summary>

###
[`v0.3.0`](https://redirect.github.com/Tecnativa/docker-socket-proxy/releases/tag/v0.3.0)

[Compare
Source](https://redirect.github.com/Tecnativa/docker-socket-proxy/compare/v0.2.0...v0.3.0)

#### What's Changed

- test: pause after container creation + poetry deps update by
[@&#8203;proudier](https://redirect.github.com/proudier) in
[Tecnativa/docker-socket-proxy#138
- Revert "chore: updated to HAproxy 3.0 and forced running as root" by
[@&#8203;proudier](https://redirect.github.com/proudier) in
[Tecnativa/docker-socket-proxy#136

**Full Changelog**:
Tecnativa/docker-socket-proxy@v0.2.0...v0.3.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/jamesdkelly88/db-lab).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC41OS4yIiwidXBkYXRlZEluVmVyIjoiMzguNTkuMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@proudier proudier deleted the revert-130-haproxy3.0 branch September 9, 2024 20:29
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.

3 participants