Remove some circular dependencies on container engine (#1547)

* make container_engine_base
* remove direct container_manager and sinsp dependencies inside container_engine

userspace/libsinsp/CMakeLists.txt

@@ -41,7 +41,7 @@ set(SINSP_SOURCES
 	chisel.cpp
 	chisel_api.cpp
 	container.cpp
-	container_engine/container_engine.cpp
+	container_engine/container_engine_base.cpp
 	container_engine/docker_common.cpp
 	container_info.cpp
 	ctext.cpp

userspace/libsinsp/container.cpp

@@ -502,8 +502,10 @@ void sinsp_container_manager::subscribe_on_remove_container(remove_container_cb
 
 void sinsp_container_manager::create_engines()
 {
+#ifdef CYGWING_AGENT
+	m_container_engines.emplace_back(new container_engine::docker(m_inspector /*wmi source*/));
+#else
 	m_container_engines.emplace_back(new container_engine::docker());
-#ifndef CYGWING_AGENT
 #if defined(HAS_CAPTURE)
 	m_container_engines.emplace_back(new container_engine::cri());
 #endif

userspace/libsinsp/container.h

@@ -33,10 +33,11 @@ limitations under the License.
 #include <curl/multi.h>
 #endif
 
-#include "container_engine/container_engine.h"
+#include "container_engine/container_cache_interface.h"
+#include "container_engine/container_engine_base.h"
 #include "mutex.h"
 
-class sinsp_container_manager
+class sinsp_container_manager : public libsinsp::container_engine::container_cache_interface
 {
 public:
 	using map_ptr_t = libsinsp::ConstMutexGuard<std::unordered_map<std::string, sinsp_container_info::ptr_t>>;
@@ -57,7 +58,7 @@ class sinsp_container_manager
 	 * @param container_info shared_ptr owning the container_info to add/update
 	 * @param thread a thread in the container, only passed to callbacks
 	 */
-	void add_container(const sinsp_container_info::ptr_t& container_info, sinsp_threadinfo *thread);
+	void add_container(const sinsp_container_info::ptr_t& container_info, sinsp_threadinfo *thread) override;
 
 	/**
 	 * @brief Update a container by replacing its entry with a new one
@@ -77,7 +78,7 @@ class sinsp_container_manager
 	 * the container, get a new shared_ptr<sinsp_container_info> and pass it
 	 * to replace_container()
 	 */
-	sinsp_container_info::ptr_t get_container(const std::string &id) const;
+	sinsp_container_info::ptr_t get_container(const std::string &id) const override;
 
 	/**
 	 * @brief Generate container JSON event from a new container
@@ -86,7 +87,7 @@ class sinsp_container_manager
 	 * Note: this is unrelated to on_new_container callbacks even though
 	 * both happen during container creation
 	 */
-	void notify_new_container(const sinsp_container_info& container_info);
+	void notify_new_container(const sinsp_container_info& container_info) override;
 
 	/**
 	 * @brief Detect container engine for a thread
@@ -111,7 +112,7 @@ class sinsp_container_manager
 	// across execs e.g. "sh -c /bin/true" execing /bin/true.
 	void identify_category(sinsp_threadinfo *tinfo);
 
-	bool container_exists(const std::string& container_id) const {
+	bool container_exists(const std::string& container_id) const override{
 		auto containers = m_containers.lock();
 		return containers->find(container_id) != containers->end() ||
 			m_lookups.find(container_id) != m_lookups.end();
@@ -145,7 +146,7 @@ class sinsp_container_manager
 	 * state of the lookup via this method and call should_lookup() before
 	 * starting a new lookup.
 	 */
-	void set_lookup_status(const std::string& container_id, sinsp_container_type ctype, sinsp_container_lookup_state state)
+	void set_lookup_status(const std::string& container_id, sinsp_container_type ctype, sinsp_container_lookup_state state) override
 	{
 		m_lookups[container_id][ctype] = state;
 	}
@@ -160,7 +161,7 @@ class sinsp_container_manager
 	 * This method effectively checks if m_lookups[container_id][ctype]
 	 * exists, without creating unnecessary map entries along the way.
 	 */
-	bool should_lookup(const std::string& container_id, sinsp_container_type ctype)
+	bool should_lookup(const std::string& container_id, sinsp_container_type ctype) override
 	{
 		auto container_lookups = m_lookups.find(container_id);
 		if(container_lookups == m_lookups.end())
@@ -175,7 +176,7 @@ class sinsp_container_manager
 	bool container_to_sinsp_event(const std::string& json, sinsp_evt* evt, std::shared_ptr<sinsp_threadinfo> tinfo);
 	std::string get_docker_env(const Json::Value &env_vars, const std::string &mti);
 
-	std::list<std::unique_ptr<libsinsp::container_engine::resolver>> m_container_engines;
+	std::list<std::unique_ptr<libsinsp::container_engine::container_engine_base>> m_container_engines;
 
 	sinsp* m_inspector;
 	libsinsp::Mutex<std::unordered_map<std::string, std::shared_ptr<const sinsp_container_info>>> m_containers;

userspace/libsinsp/container_engine/bpm.cpp

@@ -21,7 +21,7 @@ limitations under the License.
 
 using namespace libsinsp::container_engine;
 
-bool bpm::resolve(sinsp_container_manager* manager, sinsp_threadinfo* tinfo, bool query_os_for_missing_info)
+bool bpm::resolve(container_cache_interface *cache, sinsp_threadinfo *tinfo, bool query_os_for_missing_info)
 {
 	sinsp_container_info container_info;
 	bool matches = false;
@@ -59,12 +59,12 @@ bool bpm::resolve(sinsp_container_manager* manager, sinsp_threadinfo* tinfo, boo
 	}
 
 	tinfo->m_container_id = container_info.m_id;
-	if (manager->should_lookup(container_info.m_id, CT_BPM))
+	if(cache->should_lookup(container_info.m_id, CT_BPM))
 	{
 		container_info.m_name = container_info.m_id;
 		auto container = std::make_shared<sinsp_container_info>(container_info);
-		manager->add_container(container, tinfo);
-		manager->notify_new_container(container_info);
+		cache->add_container(container, tinfo);
+		cache->notify_new_container(container_info);
 	}
 	return true;
 }

userspace/libsinsp/container_engine/bpm.h

@@ -19,18 +19,17 @@ limitations under the License.
 
 #pragma once
 
-class sinsp_container_manager;
 class sinsp_container_info;
 class sinsp_threadinfo;
 
-#include "container_engine/container_engine.h"
+#include "container_engine/container_engine_base.h"
 
 namespace libsinsp {
 namespace container_engine {
-class bpm : public resolver
+class bpm : public container_engine_base
 {
 public:
-	bool resolve(sinsp_container_manager* manager, sinsp_threadinfo* tinfo, bool query_os_for_missing_info) override;
+	bool resolve(container_cache_interface *cache, sinsp_threadinfo *tinfo, bool query_os_for_missing_info) override;
 };
 }
 }

userspace/libsinsp/container_engine/container_cache_interface.h

@@ -0,0 +1,60 @@
+/*
+Copyright (C) 2013-2018 Draios Inc dba Sysdig.
+
+This file is part of sysdig.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+
+*/
+
+#pragma once
+
+#include "container_info.h"
+
+namespace libsinsp
+{
+namespace container_engine
+{
+
+/**
+ * Interface for a container cache for container engines.
+ */
+class container_cache_interface
+{
+public:
+	virtual ~container_cache_interface() = default;
+
+	virtual void notify_new_container(const sinsp_container_info& container_info) = 0;
+
+	virtual bool should_lookup(const std::string& container_id, sinsp_container_type ctype) = 0;
+
+	virtual void set_lookup_status(const std::string& container_id, sinsp_container_type ctype, sinsp_container_lookup_state state) = 0;
+
+	/**
+	 * Get a container from the cache.
+	 */
+	virtual sinsp_container_info::ptr_t get_container(const std::string& id) const = 0;
+
+	/**
+	 * Add a new container to the cache.
+	 */
+	virtual void add_container(const sinsp_container_info::ptr_t& container_info, sinsp_threadinfo *thread) = 0;
+
+	/**
+	 * Return whether the container exists in the cache.
+	 */
+	virtual bool container_exists(const std::string& container_id) const = 0;
+};
+
+}
+}

userspace/libsinsp/container_engine/container_engine.cpp → userspace/libsinsp/container_engine/container_engine_base.cpp

@@ -17,10 +17,10 @@ limitations under the License.
 
 */
 
-#include "container_engine/container_engine.h"
+#include "container_engine/container_engine_base.h"
 
 using namespace libsinsp::container_engine;
 
-void resolver::cleanup()
+void container_engine_base::cleanup()
 {
 }

userspace/libsinsp/container_engine/container_engine_base.h

@@ -0,0 +1,49 @@
+/*
+Copyright (C) 2013-2019 Draios Inc dba Sysdig.
+
+This file is part of sysdig.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+
+*/
+
+#pragma once
+
+#include "container_engine/container_cache_interface.h"
+
+class sinsp_threadinfo;
+
+namespace libsinsp {
+namespace container_engine {
+
+/**
+ * Base class for container engine. This provided the interfaces to
+ * associate a container with a thread.
+ */
+class container_engine_base {
+public:
+	virtual ~container_engine_base() = default;
+
+	/**
+	 * Find a container associated with the given tinfo and add it to the
+	 * cache.
+	 */
+	virtual bool resolve(container_cache_interface* cache,
+			     sinsp_threadinfo* tinfo,
+			     bool query_os_for_missing_info) = 0;
+
+	virtual void cleanup();
+};
+}
+}
+

userspace/libsinsp/container_engine/cri.cpp

@@ -248,7 +248,7 @@ void cri::set_cri_delay(uint64_t delay_ms)
 	s_cri_lookup_delay_ms = delay_ms;
 }
 
-bool cri::resolve(sinsp_container_manager* manager, sinsp_threadinfo* tinfo, bool query_os_for_missing_info)
+bool cri::resolve(container_cache_interface *cache, sinsp_threadinfo *tinfo, bool query_os_for_missing_info)
 {
 	std::string container_id;
 
@@ -270,7 +270,7 @@ bool cri::resolve(sinsp_container_manager* manager, sinsp_threadinfo* tinfo, boo
 		return false;
 	}
 
-	if(!manager->should_lookup(container_id, m_cri->get_cri_runtime_type()))
+	if(!cache->should_lookup(container_id, m_cri->get_cri_runtime_type()))
 	{
 		return true;
 	}
@@ -300,19 +300,19 @@ bool cri::resolve(sinsp_container_manager* manager, sinsp_threadinfo* tinfo, boo
 
 		if(!m_async_source)
 		{
-			auto async_source = new cri_async_source(manager, m_cri.get(), s_cri_timeout);
+			auto async_source = new cri_async_source(cache, m_cri.get(), s_cri_timeout);
 			m_async_source = std::unique_ptr<cri_async_source>(async_source);
 		}
 
-		manager->set_lookup_status(container_id, m_cri->get_cri_runtime_type(), sinsp_container_lookup_state::STARTED);
-		auto cb = [manager](const libsinsp::cgroup_limits::cgroup_limits_key& key, const sinsp_container_info &res)
+		cache->set_lookup_status(container_id, m_cri->get_cri_runtime_type(), sinsp_container_lookup_state::STARTED);
+		auto cb = [cache](const libsinsp::cgroup_limits::cgroup_limits_key& key, const sinsp_container_info& res)
 		{
 			g_logger.format(sinsp_logger::SEV_DEBUG,
 					"cri_async (%s): Source callback result=%d",
 					key.m_container_id.c_str(),
 					res.m_lookup_state);
 
-			manager->notify_new_container(res);
+			cache->notify_new_container(res);
 		};
 
 		sinsp_container_info result;
@@ -344,7 +344,7 @@ bool cri::resolve(sinsp_container_manager* manager, sinsp_threadinfo* tinfo, boo
 	}
 	else
 	{
-		manager->notify_new_container(*container);
+		cache->notify_new_container(*container);
 	}
 	return true;
 }

userspace/libsinsp/container_engine/cri.h

@@ -22,11 +22,10 @@ limitations under the License.
 #include <string>
 #include <stdint.h>
 
-class sinsp_container_manager;
 class sinsp_threadinfo;
 
 #include "cgroup_limits.h"
-#include "container_engine/container_engine.h"
+#include "container_engine/container_engine_base.h"
 #include "container_info.h"
 #include <cri.h>
 
@@ -53,9 +52,9 @@ class cri_async_source : public sysdig::async_key_value_source<
         sinsp_container_info>
 {
 public:
-	explicit cri_async_source(sinsp_container_manager* manager, ::libsinsp::cri::cri_interface* cri, uint64_t ttl_ms) :
+	explicit cri_async_source(container_cache_interface *cache, ::libsinsp::cri::cri_interface *cri, uint64_t ttl_ms) :
 		async_key_value_source(NO_WAIT_LOOKUP, ttl_ms),
-		m_container_manager(manager),
+		m_cache(cache),
 		m_cri(cri)
 	{
 	}
@@ -72,16 +71,16 @@ class cri_async_source : public sysdig::async_key_value_source<
 	bool parse_containerd(const runtime::v1alpha2::ContainerStatusResponse& status, sinsp_container_info& container);
 	void run_impl() override;
 
-	sinsp_container_manager* m_container_manager;
+	container_cache_interface *m_cache;
 	::libsinsp::cri::cri_interface *m_cri;
 };
 
-class cri : public resolver
+class cri : public container_engine_base
 {
 public:
 	cri();
 
-	bool resolve(sinsp_container_manager* manager, sinsp_threadinfo* tinfo, bool query_os_for_missing_info) override;
+	bool resolve(container_cache_interface *cache, sinsp_threadinfo *tinfo, bool query_os_for_missing_info) override;
 	void cleanup() override;
 	static void set_cri_socket_path(const std::string& path);
 	static void set_cri_timeout(int64_t timeout_ms);