search

containernetworking / plugins

Some reference and example networking plugins, maintained by the CNI team.
Apache License 2.0
2.23k stars 788 forks source link

macvlan cmdDel: replace the loadConf function with json.unmarshal #954

Closed cyclinder closed 1 year ago

cyclinder commented 1 year ago

When the master interface on the node has been deleted, and loadConf tries to get the MTU, This causes cmdDel to return a linkNotFound error to the runtime. The cmdDel only needs to unmarshal the net config. No need to get the MTU. So we just replaced the loadConf function with json.unmarshal in cmdDel.

Fixes https://github.com/containernetworking/plugins/issues/953

mlguerrero12 commented 1 year ago

looks good now, please improve the commit title and message. See https://cbea.ms/git-commit/ as reference.

cyclinder commented 1 year ago

thanks @mlguerrero12, This is my first time contributing to CNI, Thank you for helping me.

mlguerrero12 commented 1 year ago

In the description, you mention that the problem is when the pod's nic is deleted, but in reality the bug you are seeing is because the master interface is deleted and loadConf tries to get the MTU. The delete command only needs to unmarshall the net config. No need to get the MTU. Please update the description and wrap the body of the commit around 72 characters or so (as mentioned in bullet 6 of the link I shared above). Currently, you have a very long line as body which is difficult to read.

cyclinder commented 1 year ago

thanks a lot @mlguerrero12. How about the latest description?

mlguerrero12 commented 1 year ago

LGTM

Just a small nit: please correct the grammar mistakes in the body of the commit.

@squeed, could you please have a look at this?

cyclinder commented 1 year ago

Updated, Thanks @mlguerrero12 @squeed

cyclinder commented 1 year ago

Hey @mlguerrero12 @squeed, Is time to merge this?