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

server: More concise error messages. #1236

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

igsilya
Copy link
Contributor

@igsilya igsilya commented Feb 28, 2024

On the CNI request failure, multus-cni prints out cmdArgs. In all cases, except for debug printing, this is done with %s and a special printing function. However, the handleCNIRequest is an exception for some reason. That leads to unintelligible error messages in case of CNI request failures (severely abridged):

 CmdAdd (shim): CNI request failed with status 400:
 '&{ContainerID:<id> Netns:/var/run/netns/<uuid> IfName:eth0
    Args:<args> Path: StdinData:[125 121 111 117 114 32 97 100 118
    101 114 116 105 115 101 109 101 110 116 32 99 111 117 108 100
    32 98 101 32 104 101 114 101 125 ... another 650 numbers ]}
 ContainerID:"<id>" Netns:"/var/run/netns/<uuid>" IfName:"eth0"
 Args:"<args>" Path:"" ERRORED: error configuring pod ...

printCmdArgs() should be used for this case as well to avoid huge hardly readable logs.

At the same time, the content of cniCmdArgs is always appended to the error twice as seen in the example above. The first time by the HandleCNIRequest and another time by the handleCNIRequest. Same for the HandleDelegateRequest path.

Just removing the prefixing from the lower level handlers while keeping higher level ones. The ERRORED part migrated to the higher level handler functions to preserve the overall look of the error.

On the CNI request failure, multus-cni prints out cmdArgs.  In all
cases, except for debug printing, this is done with %s and a special
printing function.  However, the handleCNIRequest is an exception for
some reason.  That leads to unintelligible error messages in case
of CNI request failures (severely abridged):

 CmdAdd (shim): CNI request failed with status 400:
 '&{ContainerID:<id> Netns:/var/run/netns/<uuid> IfName:eth0
    Args:<args> Path: StdinData:[125 121 111 117 114 32 97 100 118
    101 114 116 105 115 101 109 101 110 116 32 99 111 117 108 100
    32 98 101 32 104 101 114 101 125 ... another 650 numbers ]}
 ContainerID:"<id>" Netns:"/var/run/netns/<uuid>" IfName:"eth0"
 Args:"<args>" Path:"" ERRORED: error configuring pod ...

printCmdArgs() should be used for this case as well to avoid huge
hardly readable logs.

At the same time, the content of cniCmdArgs is always appended to
the error twice as seen in the example above.  The first time by the
HandleCNIRequest and another time by the handleCNIRequest.  Same for
the HandleDelegateRequest path.

Just removing the prefixing from the lower level handlers while
keeping higher level ones.  The 'ERRORED' part migrated to the higher
level handler functions to preserve the overall look of the error.

Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
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 clean up on these error messages

@coveralls
Copy link

Coverage Status

coverage: 62.778% (+0.03%) from 62.749%
when pulling ddc78f1 on igsilya:failure-logs
into 5f0b4cd on k8snetworkplumbingwg:master.

@dougbtv dougbtv merged commit ad81dbf into k8snetworkplumbingwg:master Mar 14, 2024
24 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.

3 participants