Add openstacksdk option to openstack_controller #3109

coignetp · 2019-02-13T16:56:04Z

What does this PR do?

Add the OpenstackSdkApi option to the api factory of openstack_controller. It uses openstacksdk (v 0.24.0)

Motivation

Review checklist

PR has a meaningful title or PR has the no-changelog label attached
Feature or bugfix has tests
Git history is clean
If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo
If PR adds a configuration option, it has been added to the configuration file.

Additional Notes

Openstacksdk is in version 0.24.0 and is not complete yet.

hypervisor_uptime is not available with openstacksdk yet, so the metrics hypervisor_load.1, hypervisor_load.5 and hypervisor_load.15 can't be created (this is like setting collect_hypervisor_load to False in the configuration file).
server_diagnostics is not available with openstacksdk yet, but it should be available in the next release (commit), so a few metrics are not created (this is like setting collect_server_diagnostic_metrics to False).

codecov · 2019-02-13T17:30:49Z

Codecov Report

Merging #3109 into master will decrease coverage by 4.58%.
The diff coverage is 90.73%.

@@            Coverage Diff             @@
##           master    #3109      +/-   ##
==========================================
- Coverage   84.51%   79.92%   -4.59%     
==========================================
  Files         675       68     -607     
  Lines       36297     5894   -30403     
  Branches     4307      642    -3665     
==========================================
- Hits        30675     4711   -25964     
+ Misses       4362     1020    -3342     
+ Partials     1260      163    -1097

gzussa

This is great 💯 🥇 . Definitely on the right track 👏 . I made a few comments to push this the extra mile.

gzussa · 2019-02-14T11:09:17Z

openstack_controller/datadog_checks/openstack_controller/data/conf.yaml.example

+  #    - <KEY_2>:<VALUE_2>
+
+  ## @param openstack_sdk_config_file_path - optional - string


Let's move these up in the config next to user. Also let's explain how user can authenticate using one or the other.

gzussa · 2019-02-14T11:10:57Z

openstack_controller/datadog_checks/openstack_controller/data/conf.yaml.example

+  ## Absolute path of the configuration file for the connection to openstack with openstacksdk.
+  #
+  # openstack_sdk_config_file_path: "/path/to/file.yaml"


let's remove sdk

gzussa · 2019-02-14T11:17:14Z

openstack_controller/datadog_checks/openstack_controller/data/conf.yaml.example

+  ## Name of the cloud  configuration to use
+  #
+  # openstack_sdk_cloud_name: "simple-cloud"


Let's remove sdk. In general, the config should not expose how thing are being done under the hood. This is not a strong rule 😄

gzussa · 2019-02-14T11:18:14Z

openstack_controller/requirements-dev.txt

@@ -1 +1,2 @@
 datadog-checks-dev
+openstacksdk==0.24.0


You can remove this since it's is not in addition to regular deps

gzussa · 2019-02-14T12:40:06Z

openstack_controller/datadog_checks/openstack_controller/openstack_controller.py

@@ -700,7 +708,8 @@ def check(self, instance):
            self.collect_hypervisors_metrics(custom_tags=custom_tags,
                                             use_shortname=use_shortname,
                                             collect_hypervisor_metrics=collect_hypervisor_metrics,
-                                             collect_hypervisor_load=collect_hypervisor_load)
+                                             collect_hypervisor_load=collect_hypervisor_load,
+                                             openstack_sdk_cloud_name=openstack_sdk_cloud_name)


Would it be possible to have this within the api implementation and have the api function that is not supported raise an exception that we would then catch in the check?

gzussa · 2019-02-14T12:44:08Z

openstack_controller/datadog_checks/openstack_controller/api.py

@@ -72,6 +83,158 @@ def get_networks(self):
        raise NotImplementedError()


+class OpenstackSdkApi(AbstractApi):


Could we rename this OpenstackSDKApi. Not a big deal tho, completely opinionated and definitely coming from my OCD issues 😄

Says the one who wrote Api

gzussa · 2019-02-14T12:46:40Z

openstack_controller/datadog_checks/openstack_controller/api.py

+
+        api = None
+
+        if openstack_sdk_cloud_name is None:


Let's add comments here to explain what is going on for people you don't have the full context.

gzussa · 2019-02-14T12:47:12Z

openstack_controller/datadog_checks/openstack_controller/api.py

@@ -1,6 +1,8 @@
 # (C) Datadog, Inc. 2018
 # All rights reserved
 # Licensed under a 3-clause BSD style license (see LICENSE)
+from openstack import connection


Let's move from .... import ... after import ...

gzussa · 2019-02-14T12:52:56Z

openstack_controller/tests/test_openstack_sdk_api.py

+    api = OpenstackSdkApi(None)
+    api.connection = MockOpenstackConnection()
+
+    assert api.get_os_hypervisors_detail() == EXAMPLE_HYPERVISORS_VALUE


For all these methods, is the output the same as what the simpleApi Would return.
It would be great to use the same assertion values in both cases just to make sure things match.

Some of these methods return more information than SimpleApi, and some less information (not used by the check). There are some commentary for the methods that have less information.

I guess the tests then should only assert for values actually used by the check, then we can keep the same assertions for both APIs

gzussa · 2019-02-14T12:53:32Z

openstack_controller/tests/test_openstack_sdk_api.py

+                                                            MissingNovaEndpoint, MissingNeutronEndpoint)
+
+
+EXAMPLE_PROJECTS_VALUE = [


For all these Api output, do you have a link to the doc that can help check out the structure?

The doc does not specify the output, I called each methods from the test env to have their explicit output.

zippolyte

Awesome 👍, small nits aside :)

openstack_controller/datadog_checks/openstack_controller/api.py

zippolyte · 2019-02-15T12:26:53Z

openstack_controller/datadog_checks/openstack_controller/data/conf.yaml.example

+  ## @param openstack_config_file_path - optional - string
+  ## Absolute path of the configuration file for the connection to openstack with openstacksdk.
+  #
+  # openstack_config_file_path: "/path/to/file.yaml"


Suggested change

# openstack_config_file_path: "/path/to/file.yaml"

# openstack_config_file_path: <PATH_TO_YAML_FILE>

let's follow the configuration file norm https://github.com/DataDog/integrations-core/blob/master/docs/dev/new_check_howto.md#configuration-file

zippolyte · 2019-02-15T12:27:10Z

openstack_controller/datadog_checks/openstack_controller/data/conf.yaml.example

+  ## openstack_config_file_path, or at the default location:
+  ## ~/.config/openstack or /etc/openstack
+  #
+  # openstack_cloud_name: "simple-cloud"


Suggested change

# openstack_cloud_name: "simple-cloud"

# openstack_cloud_name: <CLOUD_NAME>

gzussa

Looks good to me. Great Job!

approved

arbll

🐍

l0k0ms

Fixed some issue in the configuration file, otherwise LGTM 👌

coignetp requested review from a team as code owners February 13, 2019 16:56

coignetp added the integration/openstack_controller label Feb 14, 2019

gzussa suggested changes Feb 14, 2019

View reviewed changes

gzussa added the changelog/Added label Feb 14, 2019

zippolyte requested changes Feb 14, 2019

View reviewed changes

coignetp added 20 commits February 15, 2019 10:32

add OpenstackSdkApi endpoints

189f562

add get endpoints tests

d659bd0

fix typo

0262f18

add projects methods

df4364b

add hypervisors methods

3bac3f6

add aggregates method

6b50a74

add flavors

b7084a3

fix get endpoints

0b87f85

add get networks for openstacksdk

47cf766

add servers methods to OpenstackSdkApi

1f09e0a

fix flavors query param for OpenstackSdkApi

02a9d16

fix flavors returned format

a0fc8c9

disable openstacksdk server diagnostics

bf62074

fix openstacksdk dependency

dd397ba

refactor openstack sdk api tests

fc910f7

refactor get flavors for openstack sdk

3781bf0

address some reviews

cb1d454

refactor api common responses for tests

4421629

refactor some tests

c522975

address reviews

1aba6f4

coignetp requested review from a team as code owners February 15, 2019 09:46

coignetp force-pushed the paul/openstack-controller-sdk branch from 7f19bff to 1aba6f4 Compare February 15, 2019 09:54

zippolyte previously requested changes Feb 15, 2019

View reviewed changes

address configuration file norm review

42046c5

gzussa previously approved these changes Feb 18, 2019

View reviewed changes

arbll previously approved these changes Feb 18, 2019

View reviewed changes

fixing configuration file

c683c98

l0k0ms dismissed stale reviews from arbll and gzussa via c683c98 February 18, 2019 13:13

placeholder update

921d884

l0k0ms approved these changes Feb 18, 2019

View reviewed changes

arbll approved these changes Feb 18, 2019

View reviewed changes

zippolyte approved these changes Feb 18, 2019

View reviewed changes

gzussa merged commit 97b0144 into master Feb 18, 2019

ofek deleted the paul/openstack-controller-sdk branch February 18, 2019 17:35

zippolyte mentioned this pull request Feb 28, 2019

Pin datadog_checks_base>=6.1.0 which includes openstacksdk dependency #3225

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add openstacksdk option to openstack_controller #3109

Add openstacksdk option to openstack_controller #3109

coignetp commented Feb 13, 2019

codecov bot commented Feb 13, 2019 •

edited

Loading

gzussa left a comment

gzussa Feb 14, 2019

gzussa Feb 14, 2019

gzussa Feb 14, 2019

gzussa Feb 14, 2019

gzussa Feb 14, 2019

gzussa Feb 14, 2019

zippolyte Feb 14, 2019

gzussa Feb 14, 2019

gzussa Feb 14, 2019

gzussa Feb 14, 2019

coignetp Feb 14, 2019

zippolyte Feb 14, 2019

gzussa Feb 14, 2019

coignetp Feb 14, 2019

zippolyte left a comment

zippolyte Feb 15, 2019

zippolyte Feb 15, 2019

zippolyte Feb 15, 2019

gzussa left a comment

arbll left a comment

l0k0ms left a comment

		# - <KEY_2>:<VALUE_2>

		## @param openstack_sdk_config_file_path - optional - string

		@@ -72,6 +83,158 @@ def get_networks(self):
		raise NotImplementedError()


		class OpenstackSdkApi(AbstractApi):

		MissingNovaEndpoint, MissingNeutronEndpoint)


		EXAMPLE_PROJECTS_VALUE = [

	# openstack_config_file_path: "/path/to/file.yaml"
	# openstack_config_file_path: <PATH_TO_YAML_FILE>

	# openstack_cloud_name: "simple-cloud"
	# openstack_cloud_name: <CLOUD_NAME>

Add openstacksdk option to openstack_controller #3109

Add openstacksdk option to openstack_controller #3109

Conversation

coignetp commented Feb 13, 2019

What does this PR do?

Motivation

Review checklist

Additional Notes

codecov bot commented Feb 13, 2019 • edited Loading

Codecov Report

gzussa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zippolyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzussa left a comment

Choose a reason for hiding this comment

arbll left a comment

Choose a reason for hiding this comment

l0k0ms left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 13, 2019 •

edited

Loading