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

core, ipv6: enable optimistic_dad instead of completely disabling Duplicate Address Detection #261

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

flouthoc
Copy link
Collaborator

We are disabling duplicate address detection for ipv6 completely which
might not be needed so instead of setting accept_dad to 0 we can
still do optimistic_dad to 1 for required interface.

Some cons of using optimistic_dad

  • Might be still slower than disabling DAD.
  • Not good support for older kernels since it was added in later
    released of 3.x

Closes: #260

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 16, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@flouthoc flouthoc marked this pull request as draft March 16, 2022 08:35
@flouthoc flouthoc force-pushed the switch_optimistic_dad branch from 3781661 to a6a5c14 Compare March 16, 2022 08:35
// Do not accept Router Advertisements if ipv6 is enabled
let br_accept_dad = format!("/proc/sys/net/ipv6/conf/{}/accept_dad", ifname);
let br_optimistic_dad = format!("/proc/sys/net/ipv6/conf/{}/optimistic_dad", ifname);
let br_accept_ra = format!("net/ipv6/conf/{}/accept_ra", ifname);
Copy link
Collaborator Author

@flouthoc flouthoc Mar 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question not sure about this but do we need to enable Router Advertisements ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. I think this was copied from cni.

@flouthoc
Copy link
Collaborator Author

Caution: This is a draft PR and needs to be experimented on before this can be merged. Follow discussion here: #260

@flouthoc flouthoc requested a review from Luap99 March 16, 2022 08:44
…uplicate Address Detection

We are disabling duplicate address detection for `ipv6` complelety which
might not be needed so instead of setting `accept_dad` to `0` we can
still do `optimistic_dad` to `1` for required interface.

Some cons of using `optimistic_dad`
* Might be still slower than disabling DAD.
* Not good support for older kernels since it was added in later
  released of `3.x`

Signed-off-by: Aditya R <[email protected]>
@flouthoc flouthoc force-pushed the switch_optimistic_dad branch from a6a5c14 to 613d297 Compare March 16, 2022 08:46
@Luap99
Copy link
Member

Luap99 commented Mar 16, 2022

I don't think we need to support such old kernels.

The test failures are exactly the reason why we disabled dad. I guess optimistic dad does not work as hoped.

@flouthoc
Copy link
Collaborator Author

@Luap99 Yeah I think it will be still slow and I am still missing the reason why do we need to enable DAD inside container is it to avoid conflicts for the new device addresses ? But nvm I'll check a few things and if this does not works out I'll close the issue.

@Luap99
Copy link
Member

Luap99 commented Mar 17, 2022

@flouthoc Can you try setting use_optimistic as well.

@sbrivio-rh
Copy link

sbrivio-rh commented Mar 17, 2022

Hi,

@Luap99 Yeah I think it will be still slow and I am still missing the reason why do we need to enable DAD inside container is it to avoid conflicts for the new device addresses ? But nvm I'll check a few things and if this does not works out I'll close the issue.

There are two reasons I suggested optimistic DAD should be used instead of disabling DAD completely:

  • if the container's MAC address changes, forced neighbour advertisements are allowed if DAD is enabled, and that will clear/update neighbour caches. See also libnetwork/osl: If Optimistic DAD (RFC 4429) is available, use that instead of moby/moby#42746
  • if you have a networking model where NDP crosses the container boundary (I'm not sure netavark supports something like that at the moment, or if it ever will), disabling DAD might lead to a catastrophic collision, albeit in extremely rare cases

I see the netcat test is still failing. I don't think that's because of the delay from optimistic DAD (that's really small, and comparable with address assignment timing), but I can try that out in a simplified scenario with a simple network namespace if it helps.

@flouthoc
Copy link
Collaborator Author

Hi @sbrivio-rh, Thank you so much for the explanation. Sure could you please try to reprod this in a smaller env. That would be great help. Meanwhile I'm also trying to look at this.

Thank you for the help :D

@openshift-merge-robot
Copy link
Collaborator

@flouthoc: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use optimistic_dad instead of disabling DAD
4 participants