Add tests for pkg/grpc/exec #218
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tpapagian
force-pushed
the
pr/apapag/cache_tests
branch
from
e837d4a to
8716128
Compare
kkourt
reviewed
Jul 12, 2022
There was a problem hiding this comment.
Thanks!
Your last commit mentions:
This commit tries to fix the issues with the previous tests.
Are those issues described somewhere? That is, where there bugs uncovered with the test? If so, I think it's worth describing them in the commit message. E.g., did the test fail? How?
tpapagian
force-pushed
the
pr/apapag/cache_tests
branch
from
8716128 to
e8bcadc
Compare
tpapagian
force-pushed
the
pr/apapag/cache_tests
branch
3 times, most recently
from
93848b2 to
e5ceb65
Compare
tpapagian
force-pushed
the
pr/apapag/cache_tests
branch
4 times, most recently
from
acf93f5 to
08c9b73
Compare
This will be used to create unit tests that run in a small period of time. Signed-off-by: Anastasios Papagiannis <[email protected]>
An exec and exit event can be delivered out of order to the agent. For this reason, we have a cache that keeps the un-populated events for a small period of time to wait for the exec event. There are also other caches that deal with this situations. This commit adds tests for pkg/grpc/exec. For now it only contains 2 different cases: 1. 2 events in order (TestGrpcExecInOrder) 2. 2 events out of order (TestGrpcExecOutOfOrder) Now the TestGrpcExecOutOfOrder fails while the TestGrpcExecInOrder passes. This can be uses as a skeleton to implement tests for other caches as well. ps. Trying to understand if the failure is due to the exec.go or the test itself. Signed-off-by: Anastasios Papagiannis <[email protected]>
tpapagian
force-pushed
the
pr/apapag/cache_tests
branch
from
08c9b73 to
faf4cf2
Compare
Resolve obvious cases for Needed() where we have a process pointer, but are missing binary and other information. This happens through common paths for example when exit events are received before their exec event. To fix just extend the Needed() check to search for Binary name. I introduced this while preparing initial release and doing some refactoring. Fixes: 931b70f "Tetragon: Cilium developers happily contribute Tetragon" Signed-off-by: John Fastabend <[email protected]>
tpapagian
force-pushed
the
pr/apapag/cache_tests
branch
5 times, most recently
from
ad13ba6 to
b4bcc35
Compare
If event.internal was nil here we would then dereference a nil pointer and promptly panic. So, lets delete this if/else condition. I believe at some point we didn't always set the internal field, but now if we track back into pkg/grpc code we see that if the internal field can not be looked up we create an empty one and set the time/pid so we at least have some reference to the process. Signed-off-by: John Fastabend <[email protected]>
There will be no k8s info if the k8s is disabled and so lets not try to query it. This can cause events to remain in cache for the full timeout when k8s is disabled, but underlying system does have docker IDs. At the same time remove the blanket check to skip handleEvent logic when k8s is disabled because we will still want to handle the case where the process info is missing due to out of order event. For example when the tracing event is processed before the execve event. Signed-off-by: John Fastabend <[email protected]>
The eventcache event handler (not execve handlers) does not actually wait for the internal process pointer to be populated. Because we are just looking at a pointer and waiting for the execve event to get handled we need some extra checks. Signed-off-by: John Fastabend <[email protected]>
Its possible we have a reference to a process that is missing its K8s podInfo. If this is the case simply wait for it to get flushed through the cache. This happens when both the pkg/grpc/* event is in the cache and the associated execve event. In this case we just have to wait for the pod watcher to catch up with us. Signed-off-by: John Fastabend <[email protected]>
tpapagian
force-pushed
the
pr/apapag/cache_tests
branch
from
15deb00 to
28e16eb
Compare
Eventcache tries in each iteration to find if the process info is in the cache. In order to do so, we use pid and the time that the process started. We used timestamppb which is actually normalized with ktime.DecodeKtime() and is not what we need. This resulted that we always wait for all iterations to to end before we produce the event. This commit used the raw ktime (uint64) and if we need a timestamppb.Timestamp we simply generate that on-demand. Signed-off-by: Anastasios Papagiannis <[email protected]>
Collecting endpoint does not do anything here so lets drop it for now. Presumably, we lost the code that used this in one of our refactors so we can add it back if we have a need for it later. We also need to update the skeleton e2e test since we are now receiving more events than initially expected which would cause the test to cross the event limit. To fix this, bump the limit up to a conservative 100 events. Signed-off-by: John Fastabend <[email protected]> Signed-off-by: William Findlay <[email protected]>
tpapagian
force-pushed
the
pr/apapag/cache_tests
branch
from
47a8a05 to
d769fda
Compare
jrfastab
approved these changes
Aug 2, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
An exec and exit event can be delivered out of order to the agent. For this reason, we have a cache that keeps the un populated events for a small period of time to wait for the exec event.
There are also other caches that deal with this situations.
This commit adds tests for pkg/grpc/exec. For now it only contains 2 different cases:
Signed-off-by: Anastasios Papagiannis [email protected]