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
test: fix udp-multicast-join tests #2185
Conversation
Unfortunately I've not been able to make the ipv6 tests to pass on |
cc @libuv/smartos and @mhdawson for the other three platforms. |
82d8dae
to
e6d7aff
Compare
Just an update: CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1242/ cc @libuv/aix |
@gireeshpunathil would you be able to take a look at the aix issue? |
@miladfarca could you look at the rhel 390 issue? If you need access to the specific machine just let me know. |
Thanks @mhdawson |
@mhdawson @santigimeno On
|
@miladfarca thanks for looking into it! |
We could do that... The question is; is it better to patch the system or the test? AFAIK we do minimal changes to iptables so our setup should be a good canary for typical LinuxONE setups... I don't have a good answer, on the one hand we want to cover the code, on the other, we want to be aware of possible regressions/issues with typical systems. I defer to the IBM team. |
IMO, it would be better to change the test if that's possible. Otherwise, we'll probably end up with users opening issues about the test. |
@refack I think the ansible config for linuxOne may already be making some IPTable additions in which case this would be a natural extension. @sam-github can you look to see if I'm remembering correctly and if so create a PR to add the additional configuration? |
PR-URL: #1808 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> See: libuv/libuv#2185 (comment)
e6d7aff
to
a233801
Compare
I just run the CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1452/ and it does not look good.
/cc @libuv/aix @libuv/smartos @mhdawson @miladfarca
The tests themselves are quite simple. TBH I don't know what could I change. Maybe disabling them for the platforms we can't make them pass is an option 🤷♂️ . |
I'll take a look at AIX later this week. @cjihrig Perhaps you can do smartos? |
I just ran the test manually on
|
@sam-github can you add @miladfarca's key? |
Key added to test-linuxonecc-rhel72-s390x-1 |
@sam-github Need access to |
@miladfarca test-linuxonecc-rhel72-s390x-2 has your keys now |
thanks @sam-github , issue is solved and tests are passing on both of our |
Great, thanks. I can't close this, maybe someone else here can. |
@miladfarca what needed to be updated? Just want to confirm that it is covered by the current Ansible scripts. |
@mhdawson
Seems like Sam had opened a card for this: nodejs/build#1808 |
@santigimeno I think the IPv6 test might need some additional skipping logic. At least on SmartOS, I think the test requires at least one interface that is UP, MULTICAST, IPv6, not LOOPBACK, and not POINTOPOINT. By passing the TL;DR - I think if we can get the IPv6 test passing by adding just adding an |
I've been looking into the failures in the To make it work I've had to make the following changes to the firewalld configuration: firewall-cmd --permanent --direct --add-rule ipv4 filter INPUT 0 -m pkttype --pkt-type multicast -j ACCEPT
firewall-cmd --permanent --direct --add-rule ipv6 filter INPUT 0 -m pkttype --pkt-type multicast -j ACCEPT Also we should disable the Another option might be not using Any thoughts on whether these changes could be applied? /cc @mhdawson @sam-github @refack |
@santigimeno the firewall-cmd commands or the commands to disable firewalld altogether should be added to the ansible templates so that they run when fedora machines are configured. Look for "- name: Firewall"https://github.com/nodejs/build/blob/master/ansible/roles/jenkins-worker/tasks/main.yml as an example of what was done on rhel72-s390x. It might even be possible that simply expanding the condition so that those run on fedora as well solves the problem but you'd have to review them to see. |
The messages must be actually sent to the multicast address.
a233801
to
38f7c7b
Compare
Added CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1529/ |
The CI looks pretty good now. There's only the issue with |
@bnoordhuis after this patch, that tried to fix the |
The messages must be actually sent to the multicast address. PR-URL: #2185 Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Landed in b571851. Thanks! |
The messages must be actually sent to the multicast address.