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

Suppress error message in cmdDel, in thick plugin #1084

Merged
merged 1 commit into from
May 2, 2023

Conversation

s1061123
Copy link
Member

@s1061123 s1061123 commented May 1, 2023

This fix suppress error message in thick plugin's shim, for DEL command, to align with CNI spec.

Fix #1080

@coveralls
Copy link

coveralls commented May 1, 2023

Coverage Status

Changes Unknown when pulling 5d64ec3 on s1061123:fix/del-noerr into ** on k8snetworkplumbingwg:master**.

@s1061123 s1061123 changed the title Suppress error message in cmdDel, in thick plugin [WIP]Suppress error message in cmdDel, in thick plugin May 2, 2023
@s1061123 s1061123 changed the title [WIP]Suppress error message in cmdDel, in thick plugin Suppress error message in cmdDel, in thick plugin May 2, 2023
This fix suppress error message in thick plugin's shim, for
DEL command, to align with CNI spec.

Fix k8snetworkplumbingwg#1080
Copy link
Member

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the change to preserve the logging (when available)

@dougbtv dougbtv merged commit 0c37bb0 into k8snetworkplumbingwg:master May 2, 2023
24 checks passed
oshoval added a commit to oshoval/cluster-network-addons-operator that referenced this pull request May 28, 2023
In order to have a fix for
k8snetworkplumbingwg/multus-cni#1084
which hit us on lifecycle tests / cluster-sync.

See
kubevirt#1543

Signed-off-by: Or Shoval <[email protected]>
@nayihz
Copy link

nayihz commented Jan 12, 2024

It will aslo ignore common errors raised by CNI. I don't think this is what we expected.
For example, if a custom cni cmdDel return an error, multus-shim will aslo only log this error but ignore it. So containerd believe it's a successful deletion while we can see TearDown network for sandbox xxx successfully even if it failed to docmdDel actually.

Here are some containerd logs:

Jan 12 11:07:26 node containerd[584]: 2024-01-12T11:07:26+08:00 [error] CmdCheck (shim): CNI request failed with status 400: '&{ContainerID:b8633013d903140cb60bc179d7869326595628df9ff261b5733ad4bff019307f Netns:/var/run/netns/cni-600651ab-c6f4-a3f2-232d-0f5396dd31d2 IfName:eth0 Args:IgnoreUnknown=1;K8S_POD_NAMESPACE=default;K8S_POD_NAME=nginx-6749dd5b99-779g8;K8S_POD_INFRA_CONTAINER_ID=b8633013d903140cb60bc179d7869326595628df9ff261b5733ad4bff019307f;K8S_POD_UID=5bae17b3-ddbb-4ec6-8adb-30fca2bcfefa Path: StdinData:[123 34 99 104 114 111 111 116 68 105 114 34 58 34 47 104 111 115 116 114 111 111 116 34 44 34 99 108 117 115 116 101 114 78 101 116 119 111 114 107 34 58 34 47 104 111 115 116 47 101 116 99 47 99 110 105 47 110 101 116 46 100 47 48 53 45 99 105 108 105 117 109 46 99 111 110 102 108 105 115 116 34 44 34 99 110 105 67 111 110 102 105 103 68 105 114 34 58 34 47 104 111 115 116 47 101 116 99 47 99 110 105 47 110 101 116 46 100 34 44 34 99 110 105 86 101 114 115 105 111 110 34 58 34 48 46 51 46 49 34 44 34 108 111 103 76 101 118 101 108 34 58 34 118 101 114 98 111 115 101 34 44 34 108 111 103 84 111 83 116 100 101 114 114 34 58 116 114 117 101 44 34 109 117 108 116 117 115 65 117 116 111 99 111 110 102 105 103 68 105 114 34 58 34 47 104 111 115 116 47 101 116 99 47 99 110 105 47 110 101 116 46 100 34 44 34 109 117 108 116 117 115 67 111 110 102 105 103 70 105 108 101 34 58 34 97 117 116 111 34 44 34 110 97 109 101 34 58 34 109 117 108 116 117 115 45 99 110 105 45 110 101 116 119 111 114 107 34 44 34 115 111 99 107 101 116 68 105 114 34 58 34 47 104 111 115 116 47 114 117 110 47 109 117 108 116 117 115 47 34 44 34 116 121 112 101 34 58 34 109 117 108 116 117 115 45 115 104 105 109 34 125]} ContainerID:"b8633013d903140cb60bc179d7869326595628df9ff261b5733ad4bff019307f" Netns:"/var/run/netns/cni-600651ab-c6f4-a3f2-232d-0f5396dd31d2" IfName:"eth0" Args:"IgnoreUnknown=1;K8S_POD_NAMESPACE=default;K8S_POD_NAME=nginx-6749dd5b99-779g8;K8S_POD_INFRA_CONTAINER_ID=b8633013d903140cb60bc179d7869326595628df9ff261b5733ad4bff019307f;K8S_POD_UID=5bae17b3-ddbb-4ec6-8adb-30fca2bcfefa" Path:"" ERRORED: DelegateDel: error invoking DelegateDel - "xxxxxx": error in getting result from DelNetwork: cmdDel request failed: Delete "http://192.168.100.10:18888/cmdDel": dial tcp 192.168.100.10:18888: connect: connection refused
Jan 12 11:07:26 node containerd[584]: '
Jan 12 11:07:26 node containerd[584]: time="2024-01-12T11:07:26.819150976+08:00" level=info msg="TearDown network for sandbox \"b8633013d903140cb60bc179d7869326595628df9ff261b5733ad4bff019307f\" successfully"

@nayihz
Copy link

nayihz commented Jan 21, 2024

Hi @s1061123 @dougbtv , as I mentioned earlier, user's custom CNI return error but ignored by multus. I want to know if this meet our expectations?

@nayihz
Copy link

nayihz commented Mar 11, 2024

Raise an issue for discusstion. #1239

@s1061123 s1061123 deleted the fix/del-noerr branch May 9, 2024 04:39
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.

[multus-thick] Suppress error in case of no connection to server
4 participants