containers / aardvark-dns

Authoritative dns server for A/AAAA container records. Forwards other request to host's /etc/resolv.conf
Apache License 2.0
176 stars 31 forks source link

config: ignore enoent errors while reading configs #489

Closed Luap99 closed 1 month ago

Luap99 commented 1 month ago

When we list files in directory there is a race where the file might have been removed in the meantime. So ignore the ENOENT case as this is no real error and we still want to parse the other files.

openshift-ci[bot] commented 1 month ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/containers/aardvark-dns/blob/main/OWNERS)~~ [Luap99] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
Luap99 commented 1 month ago

@mheon @baude PTAL

packit-as-a-service[bot] commented 1 month ago

Unit tests failed. @containers/packit-build please check.

packit-as-a-service[bot] commented 1 month ago

Unit tests failed. @containers/packit-build please check.

packit-as-a-service[bot] commented 1 month ago

Integration tests failed. @containers/packit-build please check.

mheon commented 1 month ago

LGTM

baude commented 1 month ago

awesome /lgtm

packit-as-a-service[bot] commented 1 month ago

Tests failed. @containers/packit-build please check.

Luap99 commented 1 month ago

fixed the failing unit test, should be good to

Luap99 commented 1 month ago

@lsm5 look slike something is broken with testing on cetons/rhel

1..19
ok 1 basic container - dns itself (custom bad dns server)
ok 2 basic container - dns itself (custom good dns server)
ok 3 basic container - dns itself custom
ok 4 basic container - ndots incomplete bad entry must NXDOMAIN instead of forwarding and timing out
ok 6 basic container - dns itself with long network name
ok 7 two containers on the same network
ok 8 basic container - internal network has no DNS
ok 9 basic container - internal network has no DNS - ipv6
ok 10 two containers on different networks
ok 11 two subnets with isolated container and one shared
ok 12 three networks with a connect
ok 13 three subnets, one container on two of the subnets, network connect
ok 14 three subnets two ipaddress v6 and one ipaddress v4, one container on two of the subnets, network connect
ok 15 two containers on the same network with aliases
ok 16 check reverse lookups
ok 17 check reverse lookups on ipaddress v6
ok 18 aardvark-dns should fail when udp port is already bound
ok 19 aardvark-dns should fail when tcp port is already bound
# bats warning: Executed 18 instead of expected 19 tests
make: *** [Makefile:108: integration] Error 1

Look super strange, fedora seems fine

Luap99 commented 1 month ago

ok 4 basic container - ndots incomplete bad entry must NXDOMAIN instead of forwarding and timing out ok 6 basic container - dns itself with long network name

ok 5 is missing?! On fedora it is there.

cc @edsantiago in case you have seen this before

edsantiago commented 1 month ago

No... BATS does not just skip tests without notification. That's really weird, and I see nothing in the test source that could explain it.

lsm5 commented 1 month ago

ok 5 is missing?! On fedora it is there.

is this the first such occurrence ?

Luap99 commented 1 month ago

ok 5 is missing?! On fedora it is there.

is this the first such occurrence ?

First time but I never looked before as tmt test were broken, after https://github.com/containers/aardvark-dns/pull/465 I assumed we should have working tmt tests now. Given your PR was green I think it might be caused by recent changes in the file. I did change/add new tests in there last week.

lsm5 commented 1 month ago

ok 5 is missing?! On fedora it is there.

is this the first such occurrence ?

First time but I never looked before as tmt test were broken, after #465 I assumed we should have working tmt tests now. Given your PR was green I think it might be caused by recent changes in the file. I did change/add new tests in there last week.

So skipping of 5 seems to be on c10s only, also seen on #491 . But there are other failures on rhel9 / c9s

baude commented 1 month ago

/lgtm