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.

Sign up for GitHub

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

Jump to bottom

Async docker daemon communication [SMAGENT-979] #1190

Closed

mattpag wants to merge 10 commits into dev from async-docker-daemon-comm

Contributor

mattpag commented Aug 9, 2018

Instead of using libcurl in a blocking manner, queue the requests to the local docker daemon in a multi handle and poll it from inspector::next().
Add and notify the new container only when all the metadata have been successfully retrieved.

Verified that events are no longer dropped even if multiple containers are started simultaneously.


          Asynchronous docker daemon communication

ac0dc7b

Instead of using libcurl in a blocking manner, queue the requests
to the local docker daemon in a multi handle and poll
it from inspector::next().
Add and notify the new container only when all the metadata
have been successfully retrieved.

mattpag requested a review from mstemm

August 9, 2018 14:34

mstemm reviewed

View reviewed changes

Contributor

mstemm left a comment

Overall this looks very good, and I like the approach of polling async results instead of spawning a thread.

userspace/libsinsp/container.h Outdated

		static void set_query_image_info(bool query_image_info);

		struct docker_request_info

Contributor

mstemm Aug 9, 2018

Instead of directly providing the url as a part of the request struct, maybe you could specify the path (images vs. containers), api version, and container id separately, and assemble the url in start_docker_request(). I think this would avoid the need for id_to_query_for_ip, as a second query to look up a container resulting from a NetworkMode -> "container:" would just end up looking like any other request.

Contributor Author

mattpag Aug 10, 2018

this sounds definitely better, will do

userspace/libsinsp/container.h Outdated

+              		bool query_images_endpoint;
+              		string id_to_query_for_ip;
+              		sinsp_container_info container_info;
+              		sinsp_threadinfo *tinfo;

Contributor

mstemm Aug 9, 2018

I think there's a possible race where the thread could go away by the time you've completed the request to the docker daemon and gotten a response back. If so, it's probably safer to provide just the pid and do the lookup of sinsp_threadinfo in parse_docker.

Contributor Author

mattpag Aug 10, 2018

You're right. This was the reason why I preferred to set tinfo->m_container_id inside resolve instead of waiting to have all the metadata. But yes, a race it's still possible since we're using just a raw pointer. Will fix.

userspace/libsinsp/container.cpp Outdated

		@@ -489,72 +635,90 @@ bool sinsp_container_engine_docker::resolve(sinsp_container_manager* manager, si
		return false;

		tinfo->m_container_id = container_info.m_id;

Contributor

mstemm Aug 9, 2018

As we were discussing on slack, it's probably better to set the threadinfo's container id only when the full container info is available. This is important for falco for rules like evt.type=execve and proc.vpid=1 and container.id=host and <other container info checks>. If the container id were set too early, the rule would start trying to look up container information before it really was available.

userspace/libsinsp/container.cpp Outdated

+              					}
+              					else
+              					{
+              						ASSERT(false);

Contributor

mstemm Aug 9, 2018

There's now a possibility of a partial failure where a container request starts and completes, then an image request starts and fails (maybe due to some temporary failure). Will the pending request be properly cleaned up in this case?

Contributor Author

mattpag Aug 10, 2018

No it's not, will fix

userspace/libsinsp/container.cpp Outdated

               		return false;
               	tinfo->m_container_id = container_info.m_id;
-              	if (!manager->container_exists(container_info.m_id))
+              	if (!manager->container_exists(container_info.m_id) &&
+              	    m_pending_requests.find(container_info.m_id) == m_pending_requests.end())

Contributor

mstemm Aug 9, 2018

I think it'd be a good idea to have some cap on the number of outstanding requests. It can be pretty high (even hundreds). I just don't want to have a degenerate case where a docker daemon failure results in an endless set of pending requests.

Contributor Author

mattpag Aug 10, 2018

I agree, will do

userspace/libsinsp/container.cpp Outdated

+              				else
+              				{
+              					Json::Reader reader;
+              					bool done = true;

Contributor

mstemm Aug 9, 2018

I think query_images_endpoint, id_to_query_for_ip, and done kind of act as a state machine to figure out which queries you've performed so far and which other queries you need to perform. Seems like the states are "query initial container", "query image", "query networked container", and "done". Based on the results you get back you might jump directly from "query initial container" to "done", or might visit one of the other steps along the way.

You might want to replace these variables with a state variable with values and make the required state transitions.

Contributor Author

mattpag Aug 10, 2018

My first implementation was using a state machine, then I switched to use query_images_endpoint before implementing id_to_query_for_ip. I agree with you, now that we can have 3+ steps a state machine it's surely more elegant. Will do.

userspace/libsinsp/container.cpp

		@@ -273,21 +354,17 @@ bool sinsp_container_engine_docker::parse_docker(sinsp_container_manager* manage
		string net_mode = hconfig_obj["NetworkMode"].asString();
		if(strncmp(net_mode.c_str(), "container:", strlen("container:")) == 0)

Contributor

mstemm Aug 9, 2018

In the worst case, you might actually have a chain of dependent lookups, right? container a has network config that refers to container b, which refers to container c. This might be another way in which a state machine would help, because you could unwind the chain and return complete results as the dependent images have container information.

Contributor Author

mattpag Aug 10, 2018

It's probably possible and doesn't require much work, I will add support for it

Contributor

mstemm Aug 10, 2018

We chatted offline and the dependent lookup is used only to get the correct ip address to associate with the original container. That eliminates some of the cases I was thinking of, where the dependent container lookup would result in a PPME_CONTAINER_JSON event, etc. This makes sense now.

Contributor Author

mattpag commented Aug 10, 2018

@mstemm PTAL
I ended up removing all the references to the URL inside docker_request_info since it can be easily reconstructed in start_docker_request just based on the current state.

mstemm reviewed

View reviewed changes

userspace/libsinsp/filterchecks.cpp Outdated

+              		container_id = tinfo->m_container_id;
+              	}
+              	else
+              	{

Contributor

mstemm Aug 10, 2018

can you add a comment here that points out that you need to parse the json from the event in the case where there is no threadinfo?

mstemm reviewed

View reviewed changes

userspace/libsinsp/sinsp.cpp Outdated

-              	// new cgroup may have created a container event.
+              	if(!is_capture() && !m_meta_evt_pending)
+              	{
+              		m_container_manager.refresh();

Contributor

mstemm Aug 10, 2018

Since this could happen for every system call, I think it would be good to measure the performance. When there are outstanding requests, you'll try calling curl_multi_perform up to m_n_polling_attempts times. If each of those involves a select()/poll()/etc, that could amplify the number of system calls significantly.

If that is happening, you could use a run_on_interval to only check for responses every tens of ms or so.

mattpag mentioned this pull request

Add support for container metaevent to detect container spawning falcosecurity/falco#405

Closed

mattpag added 9 commits

August 14, 2018 22:49


          Fix OSX builds

bc18e0d


          Refactor docker_request_info

d16842b

Use a state machine to handle the request stages, and
avoid to save in the structure unnecessary data.


          Set tinfo->m_container_id only when container metadata are available

317f6c7

Also, instead of keeping in the request info a potentially invalid
sinsp_threadinfo pointer, store and refresh a valid container process id
that can be used to retrieve a valid thread info.


          Limit the number of concurrent docker requests to 256

4bc533b


          Set m_tid on container metaevents

c4d5846

Setting a valid container thread id allow the metaevent
to be properly used in filterchecks.


          Set lookup_only as false when calling get_thread_info on internal events

259b5e0


          More refactoring docker request handling

3028f57

- Call curl_multi_perform as soon as we see a new container. That
  will possibly send the request right away.
- Inside refresh(), don't call curl_multi_perform multiple times
  but just make sure to check and consume all the finished handles.
- Instead of switching to just an alive container process, just make
  sure to have a valid init container pid.


          Add run_on_interval template class

3ab4e81

It can be used to run code on a specific interval.


          Refresh the container manager every 1ms

6a7605b

The docker container metadata fetching may be an
expensive operation, so run it every ms, not
every syscall.

mattpag force-pushed the async-docker-daemon-comm branch from 9593214 to 6a7605b Compare

August 14, 2018 20:50

anoop-sysd requested review from speedyguy17 and gnosek

November 16, 2018 17:12

anoop-sysd changed the title ~~Async docker daemon communication~~ Async docker daemon communication [SMAGENT-979]

Contributor Author

mattpag commented Dec 18, 2018

Closing this, we separately fixed a bug in the current code that resolved the issues we're seeing. We may move to an asynchronous approach in the future, where this PR can be used as a starting point if needed.

mattpag closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet