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

Fix issue with setting IP address in run time. #241

Merged
merged 1 commit into from
Jun 12, 2017

Conversation

oleksandrivantsiv
Copy link
Collaborator

Issue: sonic-net/SONiC#22
Issue was observed only when setting IP address via ifconfig
command. Caused by ifconfig command behavior: first is sets
IP address with netmask /8 and then changes netmask to specified
in command. Problem is not observed when set IP address with
"ip addr" command.

@stcheng
Copy link
Contributor

stcheng commented Jun 9, 2017

thanks for submitting this fix. i have once checked the ifconfig behavior and found this wired routine but didn't find a food solution of it. have you checked what's the time gap between the nlmsgs? because orchagent has retry logic while trying to apply tasks repeatedly, how many times of the overlaps logs have you seen during your test?

continue;
}

bool overlaps = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add some comments here to about the ifconfig issue? it helps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure

@oleksandrivantsiv
Copy link
Collaborator Author

oleksandrivantsiv commented Jun 9, 2017

@stcheng time frame between netlink messages is quite small. During tests I saw from one to four messages about overlap.

Issue: sonic-net/SONiC#22
Issue was observed only when setting IP address via ifconfig
command. Caused by ifconfig command behavior: first is sets
IP address with netmask /8 and then changes netmask to specified
in command. Problem is not observed when set IP address with
"ip addr" command.
@stcheng
Copy link
Contributor

stcheng commented Jun 9, 2017

Thanks. Could you also try to resolve the cla-required tag?

@msftclas
Copy link

@oleksandrivantsiv, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

@oleksandrivantsiv oleksandrivantsiv merged commit 4816b4b into sonic-net:master Jun 12, 2017
stcheng pushed a commit that referenced this pull request Jun 12, 2017
Issue: sonic-net/SONiC#22
Issue was observed only when setting IP address via ifconfig
command. Caused by ifconfig command behavior: first it sets
IP address with netmask /8 and then changes netmask to specified
in command. Problem is not observed when set IP address with
"ip addr" command.
Introduced overlaps variable to check if the newly set IP prefix
has any overhaps with the current IP prefixes and wait until
there is no overlaps at all.
/* NOTE: Overlap checking is required to handle ifconfig weird behavior.
* When set IP address using ifconfig command it applies it in two stages.
* On stage one it sets IP address with netmask /8. On stage two it
* changes netmask to specified in command. As DB is async event to
Copy link
Contributor

Choose a reason for hiding this comment

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

specified in command [](start = 34, length = 20)

Did you test ifconfig with a smaller netmask, such as /7 or /8?

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
* [pfcwd]: Add support to poll queue attr in syncd

* update as comments

* remove extra empty line
lukasstockner pushed a commit to genesiscloud/sonic-swss that referenced this pull request Apr 2, 2023
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.

4 participants