[Config] Update config logic to look for checks in 3rd-party directory #2273

tmichelet · 2016-02-18T17:52:57Z

Add tests on config's logic
Refactor logic
Update logic to look for checks in 3rd-party directory

Logic tested on staging

talwai · 2016-02-19T22:06:19Z

config.py

-        conf_exists = False
+    try:
+        thrird_party_path = get_3rd_party_path(osname)
+        places.append(lambda name: os.path.join(thrird_party_path, name, 'check.py'))


typo third

talwai · 2016-02-19T22:07:43Z

For better coverage, how about a test for the case where the check class inherits from a subclass of AgentCheck? E.g. class SnmpCheck(NetworkCheck)

talwai · 2016-02-19T22:12:55Z

config.py

+    if 'pythonpath' in check_config:
+        pythonpath = check_config['pythonpath']
+        if not isinstance(pythonpath, list):
+            pythonpath = [pythonpath]


Does it make sense to do some stricter checks here? For example [os.path.exists(path) for path in pythonpath]? You probably know better whether that case should fail the initialization, print a warning, or do nothing at all

I'd rather keep it safe leave the current logic untouched if you don't mind

talwai · 2016-02-19T22:36:40Z

tests/core/test_config.py

+        import config as Config
+        Config.get_checksd_path = self.patched_get_checksd_path
+        Config.get_confd_path = self.patched_get_confd_path
+        Config.get_3rd_party_path = self.patched_get_3rd_party_path


This pattern seems weird. Is it possible to use a MockConfig object with the dummy methods, and then decorate at the class-level:

@patch.object(config, 'Config', MockConfig) class TestConfigLoadCheckDirectory(unittest.TestCase):

talwai · 2016-02-19T22:37:44Z

Nice cleanup, long overdue! Left a few comments

tmichelet · 2016-02-22T18:13:41Z

@talwai thanks for the review, took your comments into account

talwai · 2016-02-22T18:17:00Z

looks great 👍

tmichelet · 2016-02-22T19:25:15Z

Added a test on the deprecated nagios logic, and fix the logic for this case

irabinovitch · 2016-02-25T16:08:56Z

@tmichelet is this intended for 5.7?

tmichelet · 2016-02-25T16:20:51Z

no @irabinovitch we'll release it as part of 5.8

olivielpeau · 2016-03-16T20:42:17Z

config.py

+    # Look for the per-check config, which *must* exist
+    if not check_config.get('instances'):
+        log.error("Config %s is missing 'instances'" % config_path)
+        return False, None, {}


Isn't this check already taken care of by the check_yaml function?

olivielpeau · 2016-03-16T22:02:49Z

There's one thing that's not handled anymore if I read through the code correctly: the .yaml.default files should also be taken into account (but should be ignored when a .yaml file is present for the same check).

For instance we use default confs for all the checks that are enabled by default (ntp, disk, network and agent_stats if I remember correctly).

Tests that should pass:

    def testConfigDefault(self, *args):
        copyfile('%s/valid_conf.yaml' % FIXTURE_PATH,
            '%s/test_check.yaml.default' % TEMP_ETC_CONF_DIR)
        copyfile('%s/valid_check_1.py' % FIXTURE_PATH,
            '%s/test_check.py' % TEMP_ETC_CHECKS_DIR)
        checks = load_check_directory({"additional_checksd": TEMP_ETC_CHECKS_DIR}, "foo")
        self.assertEquals(1, len(checks['initialized_checks']))

    def testConfigCustomOverDefault(self, *args):
        copyfile('%s/valid_conf.yaml' % FIXTURE_PATH,
            '%s/test_check.yaml' % TEMP_ETC_CONF_DIR)
        copyfile('%s/valid_conf_2.yaml' % FIXTURE_PATH,  # a 2nd valid conf file, slightly different so that we can test which one has been picked up (with 2 instances for instance)
            '%s/test_check.yaml.default' % TEMP_ETC_CONF_DIR)
        copyfile('%s/valid_check_1.py' % FIXTURE_PATH,
            '%s/test_check.py' % TEMP_ETC_CHECKS_DIR)
        checks = load_check_directory({"additional_checksd": TEMP_ETC_CHECKS_DIR}, "foo")
        self.assertEquals(1, len(checks['initialized_checks']))
        self.assertEquals(1, checks['initialized_checks'][0].instance_count())  # check that we picked the right conf

Apart from that it's a very welcome refactoring! 👍

olivielpeau · 2016-03-16T22:06:11Z

Oh and FYI flake8 is complaining about tests/core/fixtures/checks/invalid_check_2.py:1:1: F401 'nothing' imported but unused, a # noqa should fix it

review fix from #2273

tmichelet · 2016-03-24T19:51:03Z

@olivielpeau rebased on master, and added last two commits, could you have a second look please?

olivielpeau · 2016-03-28T15:36:32Z

tests/core/test_config.py

+
+
+TMP_DIR = tempfile.gettempdir()
+TEMP_3RD_PARTY_CHECKS_DIR = '%s/dd-agent-tests/3rd-party' % TMP_DIR


os.path.join(TMP_DIR, 'dd-agent-tests', '3rd-party') would be slightly better here

(same thing for the 3 lines below)

olivielpeau · 2016-03-28T15:38:18Z

One last nitpick, other than that feel free to squash and merge!

review fix from #2273

tmichelet · 2016-04-12T23:37:43Z

@olivielpeau updated and squashed, can be merged I think

- add tests on the config logic - refactor the logic - add some logic to add for checks in 3rd-party folder -- We are starting to install checks in the 3rd-party folder. Let the agent use these checks. Priority should be: - /etc/dd-agent/checks.d/ - /opt/datadog-agent/3rd-party/ - /opt/datadog-agent/agent/checks.d/

remh · 2016-04-21T15:38:42Z

Needs a rebase

olivielpeau · 2016-05-04T21:16:51Z

Closing in favor of #2467

talwai reviewed Feb 19, 2016
View reviewed changes

tmichelet force-pushed the tristan/sdk branch from 4d1b088 to 4539088 Compare February 19, 2016 22:21

talwai reviewed Feb 19, 2016
View reviewed changes

tmichelet force-pushed the tristan/sdk branch from 4539088 to db66e08 Compare February 22, 2016 15:27

olivielpeau added this to the 5.8.0 milestone Feb 29, 2016

olivielpeau reviewed Mar 16, 2016
View reviewed changes

olivielpeau self-assigned this Mar 16, 2016

tmichelet force-pushed the tristan/sdk branch from 59cc583 to d142f69 Compare March 24, 2016 19:50

tmichelet added a commit that referenced this pull request Mar 24, 2016

[Config] Review fixes (see #2273)

eacc1d2

tmichelet added a commit that referenced this pull request Mar 24, 2016

[Config] handle .yaml.default files

d142f69

review fix from #2273

olivielpeau reviewed Mar 28, 2016
View reviewed changes

tmichelet force-pushed the tristan/sdk branch from d142f69 to 9acaf0d Compare April 12, 2016 19:23

tmichelet added a commit that referenced this pull request Apr 12, 2016

[Config] handle .yaml.default files

9acaf0d

review fix from #2273

tmichelet force-pushed the tristan/sdk branch from 9acaf0d to 0fec85c Compare April 15, 2016 20:28

olivielpeau mentioned this pull request May 4, 2016

[Config] Update config logic to look for checks in 3rd-party directory (rebased) #2467

Merged

olivielpeau closed this May 4, 2016

olivielpeau deleted the tristan/sdk branch June 1, 2016 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Config] Update config logic to look for checks in 3rd-party directory #2273

[Config] Update config logic to look for checks in 3rd-party directory #2273

tmichelet commented Feb 18, 2016

talwai Feb 19, 2016

talwai commented Feb 19, 2016

talwai Feb 19, 2016

tmichelet Feb 22, 2016

talwai Feb 19, 2016

talwai commented Feb 19, 2016

tmichelet commented Feb 22, 2016

talwai commented Feb 22, 2016

tmichelet commented Feb 22, 2016

irabinovitch commented Feb 25, 2016

tmichelet commented Feb 25, 2016

olivielpeau Mar 16, 2016

olivielpeau commented Mar 16, 2016

olivielpeau commented Mar 16, 2016

tmichelet commented Mar 24, 2016

olivielpeau Mar 28, 2016

olivielpeau commented Mar 28, 2016

tmichelet commented Apr 12, 2016

remh commented Apr 21, 2016

olivielpeau commented May 4, 2016



		TMP_DIR = tempfile.gettempdir()
		TEMP_3RD_PARTY_CHECKS_DIR = '%s/dd-agent-tests/3rd-party' % TMP_DIR

[Config] Update config logic to look for checks in 3rd-party directory #2273

[Config] Update config logic to look for checks in 3rd-party directory #2273

Conversation

tmichelet commented Feb 18, 2016

talwai Feb 19, 2016

Choose a reason for hiding this comment

talwai commented Feb 19, 2016

talwai Feb 19, 2016

Choose a reason for hiding this comment

tmichelet Feb 22, 2016

Choose a reason for hiding this comment

talwai Feb 19, 2016

Choose a reason for hiding this comment

talwai commented Feb 19, 2016

tmichelet commented Feb 22, 2016

talwai commented Feb 22, 2016

tmichelet commented Feb 22, 2016

irabinovitch commented Feb 25, 2016

tmichelet commented Feb 25, 2016

olivielpeau Mar 16, 2016

Choose a reason for hiding this comment

olivielpeau commented Mar 16, 2016

olivielpeau commented Mar 16, 2016

tmichelet commented Mar 24, 2016

olivielpeau Mar 28, 2016

Choose a reason for hiding this comment

olivielpeau commented Mar 28, 2016

tmichelet commented Apr 12, 2016

remh commented Apr 21, 2016

olivielpeau commented May 4, 2016