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

datapath: Pass proxy port in to-proxy traces #17595

Merged

Conversation

jrajahalme
Copy link
Member

Include proxy port in the to-proxy trace messages.

Signed-off-by: Jarno Rajahalme jarno@isovalent.com

@jrajahalme jrajahalme added kind/enhancement This would improve or streamline existing functionality. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. labels Oct 13, 2021
@jrajahalme jrajahalme requested review from a team and jrfastab October 13, 2021 13:42
@jrajahalme jrajahalme mentioned this pull request Oct 13, 2021
9 tasks
@joestringer
Copy link
Member

joestringer commented Oct 14, 2021

/test

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sIstioTest Istio Bookinfo Demo Tests bookinfo inter-service connectivity

Failure Output

FAIL: Found 1 k8s-app=cilium logs matching list of errors that must be investigated:

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

@jrajahalme
Copy link
Member Author

GKE test failed due to level=error in the logs. The log is this:

2021-10-14T16:55:33.487891492Z level=error msg="Cannot update CEP" containerID=00ecab26e8 controller="sync-to-k8s-ciliumendpoint (1236)" datapathPolicyRevision=1 desiredPolicyRevision=1 endpointID=1236 error="ciliumendpoints.cilium.io \"k8s2-client\" not found" identity=53610 ipv4=10.120.1.170 ipv6= k8sPodName=default/k8s2-client subsys=endpointsynchronizer

This happens during endpoint regeneration, after which the endpoint is deleted. Is it possible that this error log from the endpoint synchronizer should not be considered an error at all? K8s has deleted the pod, but Cilium agent has not yet processed the deletion as it is in the middle of regeneration?

@jrajahalme
Copy link
Member Author

The CEP in question (default/k8s2-client) is not part of this test, it must be a remnant from a prior test. It would be misleading to track this as a flake on the Istio test.

@joestringer
Copy link
Member

This happens during endpoint regeneration, after which the endpoint is deleted. Is it possible that this error log from the endpoint synchronizer should not be considered an error at all? K8s has deleted the pod, but Cilium agent has not yet processed the deletion as it is in the middle of regeneration?

🤔 I think this makes sense. This controller is running because the endpoint was previously provisioned. Presumably by the time this controller first runs, there is guaranteed to be a k8s pod available so there's no way for this error to be returned. When the pod is deleted, if regeneration is under way then we probably haven't yet cancelled the ctx that is passed into this controller, so we don't yet know that the resource is gone. That said, if we attempt to update it and get a 'not found' error (specifically this one) then the only case I can think where this would occur.. is if the pod is being deleted. So yeah we shouldn't need to care in that case since this is probably the one last spurious error that this controller would see before it gets shut down.

Either we can move the 'Context' cancellation earlier for the ctx passed into this controller, or we can just detect the 'not found' error type and ignore that specific one without logging. 👍

@jrajahalme jrajahalme force-pushed the bpf-pass-proxy-port-in-to-proxy-traces branch from cb40ddf to 9402500 Compare October 18, 2021 11:19
@jrajahalme
Copy link
Member Author

/test

@joestringer joestringer added this to the 1.11.0-rc1 milestone Oct 25, 2021
Include proxy port in the to-proxy trace messages.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme jrajahalme force-pushed the bpf-pass-proxy-port-in-to-proxy-traces branch from 9402500 to d1aeed3 Compare October 28, 2021 14:08
@jrajahalme
Copy link
Member Author

/test

@joestringer joestringer merged commit 5e00eaa into cilium:master Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants