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

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

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

cyclinder
Copy link
Contributor

@cyclinder cyclinder commented Oct 11, 2023

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 #953

@cyclinder cyclinder force-pushed the improve_cmd_del branch 2 times, most recently from a020c4e to 2cd6d82 Compare October 11, 2023 09:38
@mlguerrero12
Copy link
Member

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

@cyclinder cyclinder changed the title improve macvlan cmdDel: ignore link not found macvlan cmdDel: replace the loadConf function with json.unmarshal macvlan cmdDel: ignore link not found Oct 12, 2023
@cyclinder
Copy link
Contributor Author

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

@cyclinder cyclinder changed the title macvlan cmdDel: replace the loadConf function with json.unmarshal macvlan cmdDel: ignore link not found macvlan cmdDel: replace the loadConf function with json.unmarshal Oct 12, 2023
@mlguerrero12
Copy link
Member

mlguerrero12 commented Oct 13, 2023

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
Copy link
Contributor Author

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

@mlguerrero12
Copy link
Member

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?

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 netConf. No need to
get the MTU. So we just replaced the loadConf function with
json.unmarshal in cmdDel.

Signed-off-by: cyclinder <[email protected]>
@cyclinder
Copy link
Contributor Author

Updated, Thanks @mlguerrero12 @squeed

@cyclinder
Copy link
Contributor Author

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

@squeed squeed merged commit abee8cc into containernetworking:main Nov 16, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macvlan: error killing pod: Link not found
3 participants