modified down rules to pre-down rules to ensure that default route is… by kannankvs · Pull Request #3853 · sonic-net/sonic-buildimage

kannankvs · 2019-12-06T12:24:31Z

- What I did
When management VRF is deleted, zebra was crashing as specified in the gitissue3798. As discussed in the issue, it has got two parts. One part is to fix the crash in FRR, which will happen in the future. Other part is to ensure that default route is deleted just before interface is brought down by using "pre-down" commands instead of "down" commands, which will avoid the zebra crash. Irrespective of this change, FRR will be fixed to avoid the crash in zebra code.

- How I did it
Changed "down" commands to "pre-down" commands.

- How to verify it
Executed management VRF test cases specified in the sonic-mgmt PR1210 and verified that they are passing.

- Description for the changelog
Changed the commands given under "down" to "pre-down"

… deleted just before interface is made down

…pre-down instead of down.

kannankvs · 2019-12-06T14:07:05Z

retest please

…pre-down instead of down.

kannankvs · 2019-12-08T08:20:36Z

@lguohan , @jleveque : Is it possible to review this and mark it for merging to 201911?

tylerlinp · 2019-12-09T01:22:01Z

files/image_config/interfaces/interfaces.j2

-    up ip link set dev lo-m master mgmt
-	down ip link delete dev lo-m
+    down ip link delete dev lo-m
+{% endif %}


duplicated condition here if, should remove one.

@tylerlinp : Which is duplicated condition? We modified only the order.

The condition from line 8 to 20, so line 12 and 19 is duplicated. And it is better to add a block mgmt.

tylerlinp · 2019-12-09T03:43:51Z

files/image_config/interfaces/interfaces.j2

+    pre-down ip {{ '-4' if prefix | ipv4 else '-6' }} route delete {{ prefix | network }}/{{ prefix | prefixlen }} dev eth0 table {{ vrf_table }}
+    pre-down ip {{ '-4' if prefix | ipv4 else '-6' }} rule delete from {{ prefix | ip }}/{{ '32' if prefix | ipv4 else '128' }} table {{ vrf_table }}
 {% if (MGMT_VRF_CONFIG) and (MGMT_VRF_CONFIG['vrf_global']['mgmtVrfEnabled'] == "true") %}
+    pre-down ip link set dev eth0 nomaster


Why add this? It is different from original style. And it may be wrong, in kernel handling set nomaster is including event down, so here do a down in pre-down. I think cgdelete is enough.

@tylerlinp : How come nomaster is doing an event down? Yes, we also expect the cgdelete to handle to take care of nomaster. But, similar to pre-down for route delete, we thought that "explicitly setting to nomaster" is better/safe for applications like zebra/dhcp_relay to handle this "nomaster" event separately. Anyway, this "nomaster" is not making any difference in the behavior in applications dhcp_relay and hence we think that it is safe to remove this.
@prsunny , @jleveque : Any comments?

To be exactly, linux kernel do change master just like event down, will flush neighbors and routes. If set nomaster before delete routes in kernel(here they all in pre-down, it make troubles), kernel will clear routes in handling down, and commands to delete doutes will do nothing, and there is no deldoute netlink, and our fix will fail again.

case NETDEV_DOWN: fib_disable_ip(dev, event, false); break; case NETDEV_CHANGE: flags = dev_get_flags(dev); if (flags & (IFF_RUNNING | IFF_LOWER_UP)) fib_sync_up(dev, RTNH_F_LINKDOWN); else fib_sync_down_dev(dev, event, false); /* fall through */ case NETDEV_CHANGEMTU: rt_cache_flush(net); break; case NETDEV_CHANGEUPPER: info = ptr; /* flush all routes if dev is linked to or unlinked from * an L3 master device (e.g., VRF) */ if (info->upper_dev && netif_is_l3_master(info->upper_dev)) fib_disable_ip(dev, NETDEV_DOWN, true); break;

At least here cannot use pre-down. But now that there is no up ip link set dev eth0 master mgmt how comes down ip link set dev eth0 nomaster? I think this is not safe but dangerous.

tylerlinp · 2019-12-09T03:47:08Z

files/image_config/interfaces/interfaces.j2

    vrf mgmt
    up cgcreate -g l3mdev:mgmt
    up cgset -r l3mdev.master-device=mgmt mgmt
+    pre-down ip link set dev eth0 nomaster


same as line 57.

…wn the interface. cgdelete is enough

kannankvs · 2019-12-09T10:11:52Z

@tylerlinp : We removed the nomaster as discussed. Assuming that cgdelete takes care of it. Request you to review and approve the changes.

…wn the interface. cgdelete is enough

prsunny · 2019-12-17T13:35:40Z

@tylerlinp , can you check the changes?

tylerlinp · 2019-12-18T01:21:24Z

@prsunny I had checked a few days ago and a suggestion was not replied.

The condition from line 8 to 20, so line 12 and 19 is duplicated. And it is better to add a block mgmt.

kannankvs · 2019-12-19T06:18:19Z

@prsunny I had checked a few days ago and a suggestion was not replied.

The condition from line 8 to 20, so line 12 and 19 is duplicated. And it is better to add a block mgmt.

@tylerlinp : Addressed the comment. Please check and approve.
@prsunny : Can you please mark it for cherrypicking to 201911 release?

kannankvs · 2019-12-19T15:41:10Z

retest vsimage please

qiluo-msft · 2020-01-17T05:23:47Z

Retest mellanox please

wendani · 2020-02-05T21:01:03Z

Request for 201911 branch

#3853) * modified down rules to pre-down rules to ensure that default route is deleted just before interface is made down

sonic-net#3853) * modified down rules to pre-down rules to ensure that default route is deleted just before interface is made down

…atically (#22466) #### Why I did it src/sonic-utilities ``` * e6fb8f96 - (HEAD -> 202411, origin/202411) Stop the hwclock service and timer files before updating CMOS time (#3853) (22 hours ago) [Saikrishna Arcot] ``` #### How I did it #### How to verify it #### Description for the changelog

…AD automatically (sonic-net#1066) #### Why I did it src/sonic-utilities ``` * ba955a91 - (HEAD -> 202412, origin/202412) Merge pull request sonic-net#172 from mssonicbld/sonicbld/202412-merge (21 hours ago) [mssonicbld] * 4d5c7cb9 - Merge branch '202411' of https://github.com/sonic-net/sonic-utilities into 202412 (23 hours ago) [Sonic Automation] * e6fb8f96 - Stop the hwclock service and timer files before updating CMOS time (sonic-net#3853) (2 days ago) [Saikrishna Arcot] ``` #### How I did it #### How to verify it #### Description for the changelog

modified down rules to pre-down rules to ensure that default route is…

8d005b7

… deleted just before interface is made down

kannankvs mentioned this pull request Dec 6, 2019

Zebra crash is observed when management VRF is disabled. Crash seen with simple linux scripts as well #3798

Open

modified sample test output related to changing interfaces.j2 to use …

c59ad3a

…pre-down instead of down.

modified sample test output related to changing interfaces.j2 to use …

59346e8

…pre-down instead of down.

tylerlinp suggested changes Dec 9, 2019

View reviewed changes

modified interfaces.j2 to remove nomaster that results in bringing do…

b96ac60

…wn the interface. cgdelete is enough

modified interfaces.j2 to remove nomaster that results in bringing do…

6bfe7af

…wn the interface. cgdelete is enough

removed duplicated if condition and added block for mgmt_vrf

cd898fb

tylerlinp approved these changes Dec 19, 2019

View reviewed changes

prsunny approved these changes Jan 17, 2020

View reviewed changes

prsunny merged commit d150721 into sonic-net:master Jan 17, 2020

prsunny added the Request for 201911 Branch label Feb 8, 2020

abdosi pushed a commit that referenced this pull request Feb 14, 2020

modified down rules to pre-down rules to ensure that default route is… (

74ac9b0

#3853) * modified down rules to pre-down rules to ensure that default route is deleted just before interface is made down

abdosi added the Included in 201911 Branch label Feb 14, 2020

Conversation

kannankvs commented Dec 6, 2019

Uh oh!

kannankvs commented Dec 6, 2019

Uh oh!

kannankvs commented Dec 8, 2019

Uh oh!

tylerlinp Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

kannankvs Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

tylerlinp Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

tylerlinp Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

kannankvs Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

tylerlinp Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

tylerlinp Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

kannankvs commented Dec 9, 2019

Uh oh!

prsunny commented Dec 17, 2019

Uh oh!

tylerlinp commented Dec 18, 2019

Uh oh!

kannankvs commented Dec 19, 2019

Uh oh!

kannankvs commented Dec 19, 2019

Uh oh!

qiluo-msft commented Jan 17, 2020

Uh oh!

wendani commented Feb 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants