SDN check: Ignore node's canonical name #9511

Miciah · 2018-08-09T17:44:26Z

When verifying that a node's preferred host name or address resolves to its internal IP address, ignore the canonical name returned by name resolution. In some valid configurations, the node's preferred name or address differs from the canonical name.

This change makes the check more consistent with the behavior of the debug.sh script on which the check was based.

This commit also fixes a problem with string encodings and comparisons that shows up on Python 3.

This commit fixes bug 1614261.

https://bugzilla.redhat.com/show_bug.cgi?id=1614261

roles/openshift_health_checker/openshift_checks/sdn.py (SDNCheck.read_command_output): Add an optional parameter for UTF-8 encoding (default True).
(SDNCheck.resolve_address): Use read_command_output's new parameter to disable UTF-8 encoding. Ignore canonical name in result.
roles/openshift_health_checker/test/sdn_tests.py (test_resolve_address): New test.

Miciah · 2018-08-09T17:56:17Z

@pecameron, could you review this one?

vrutkovs · 2018-08-10T10:21:06Z

roles/openshift_health_checker/test/sdn_tests.py

@@ -169,6 +170,31 @@ def execute_module(module_name, args, *_):

    SDNCheck(execute_module, task_vars).run()

+def test_resolve_address():


/roles/openshift_health_checker/test/sdn_tests.py:173:1: E302 expected 2 blank lines, found 1

Fixed. Thanks!

vrutkovs · 2018-08-10T10:21:28Z

roles/openshift_health_checker/test/sdn_tests.py

+'''
+            }
+
+        return { 'rc': 2 }


./roles/openshift_health_checker/test/sdn_tests.py:192:17: E201 whitespace after '{' ./roles/openshift_health_checker/test/sdn_tests.py:192:25: E202 whitespace before '}'

Fixed. Thanks!

vrutkovs · 2018-08-10T10:21:54Z

roles/openshift_health_checker/test/sdn_tests.py

+    check = SDNCheck(execute_module, None)
+    assert check.resolve_address('foo') == '1.2.3.4'
+    with pytest.raises(OpenShiftCheckException):
+        check.resolve_address('baz')



./roles/openshift_health_checker/test/sdn_tests.py:199:1: E302 expected 2 blank lines, found 1

Fixed. Thanks!

vrutkovs · 2018-08-10T15:16:42Z

/approve

ramr

/lgtm

ramr · 2018-08-13T03:00:33Z

roles/openshift_health_checker/test/sdn_tests.py

+        if command_args[0] != '/bin/getent':
+            raise ValueError('not expecting command: %s' % args.raw_params)
+
+        # The expected command_args is ['/bin/getent', 'ahosts4', 'foo'].


it is in a comment .. but typo ahosts4 aka should be ['/bin/getent', 'ahostsv4', 'foo']

Thanks! Fixed.

ironcladlou · 2018-08-13T14:27:12Z

/retest

Miciah · 2018-08-13T16:18:15Z

Pushed a fix for the typo in the comment.

ironcladlou · 2018-08-13T16:41:01Z

/lgtm

pravisankar

LGTM

ironcladlou · 2018-08-14T15:51:08Z

/retest

ironcladlou · 2018-08-15T13:48:48Z

/retest

ironcladlou · 2018-08-15T18:27:18Z

/retest

ironcladlou · 2018-08-15T18:27:51Z

Looks like a legit failure? https://travis-ci.org/openshift/openshift-ansible/jobs/415525261

Miciah · 2018-08-15T20:39:25Z

Pushed a fix for an encoding issue that shows up in Python 3.

When verifying that a node's preferred host name or address resolves to its internal IP address, ignore the canonical name returned by name resolution. In some valid configurations, the node's preferred name or address differs from the canonical name. This change makes the check more consistent with the behavior of the debug.sh script on which the check was based: https://github.com/openshift/openshift-sdn/blob/d55c72b668018492059a862bee06f11745de1f97/hack/debug.sh#L126 This commit also fixes a problem with string encodings and comparisons that shows up on Python 3. This commit fixes bug 1614261. https://bugzilla.redhat.com/show_bug.cgi?id=1614261 * roles/openshift_health_checker/openshift_checks/sdn.py (SDNCheck.read_command_output): Add an optional parameter for UTF-8 encoding (default True). (SDNCheck.resolve_address): Use read_command_output's new parameter to disable UTF-8 encoding. Ignore canonical name in result. * roles/openshift_health_checker/test/sdn_tests.py (test_resolve_address): New test.

Miciah · 2018-08-15T23:17:59Z

/retest

pravisankar · 2018-08-15T23:40:19Z

roles/openshift_health_checker/openshift_checks/sdn.py

@@ -425,15 +427,15 @@ def resolve_address(self, addr):
        """Look up the given IPv4 address using getent."""
        command = ' '.join(['/bin/getent', 'ahostsv4', addr])
        try:
-            out = self.read_command_output(command)
+            out = self.read_command_output(command, False)


We are passing utf8=False here because we have a test case for resolve_address() that runs on incompatible python version?
This seems fragile instead why not detect if the given string result['stdout'] is encodable and act accordingly?
Something like:

try: output = result['stdout'].encode('utf-8') except UnicodeEncodeError: output = result['stdout'] return output

read_command_output encodes the output as UTF-8 to avoid problems saving log files containing non-ASCII characters. In Python 2, this encoding presents no problems; UTF-8-encoded strings look like regular strings and behave as expected in comparisons:

>>> 'foo'.encode('utf-8') 'foo' >>> 'foo'.encode('utf-8') == 'foo' True

In Python 3, encoding the string into UTF-8 produces a "byte string", which looks and behaves differently:

>>> 'foo'.encode('utf-8') b'foo' >>> 'foo'.encode('utf-8') == 'foo' False

When I tested with Python 2, everything was fine. However, CI tests with both Python 2 and Python 3, and in the latter case, the test reveals that resolve_address is comparing incompatible string types.

An alternative to adding the utf8 parameter would be to use byte strings in resolve_address. Python 2 has forwards compatibility so that it accepts the byte-string syntax and treats a byte string as a regular string:

>>> b'foo' == 'foo' True

So we could use record[1] == b'STREAM', and it would make no difference on Python 2 (where the code already works properly) and would fix the problem on Python 3 (where encoding the output as UTF-8 turns it into a byte string). However, I decided to add the utf8 parameter instead because it lets us avoid the weird byte-string syntax and the unnecessary encoding.

I don't know the unnecessary encoding is a particularly serious issue in this performance context, but I do like the idea of a named parameter rather than the weird byte-string syntax. Not sure either way is "right" but I don't think it's worth debating over style... let's go ahead and fix the bug.

ironcladlou · 2018-08-16T15:24:16Z

/lgtm

openshift-ci-robot · 2018-08-16T15:25:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, Miciah, ramr, vrutkovs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~roles/openshift_health_checker/OWNERS~~ [vrutkovs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot requested review from sdodson and sosiouxme August 9, 2018 17:44

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 9, 2018

vrutkovs reviewed Aug 10, 2018

View reviewed changes

Miciah force-pushed the sdn-check-ignore-node's-canonical-name branch from d41c0d2 to 2d36442 Compare August 10, 2018 15:07

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 10, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2018

ramr approved these changes Aug 13, 2018

View reviewed changes

openshift-ci-robot assigned ramr Aug 13, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2018

Miciah force-pushed the sdn-check-ignore-node's-canonical-name branch from 2d36442 to eccffcb Compare August 13, 2018 16:18

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2018

openshift-ci-robot assigned ironcladlou Aug 13, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2018

pravisankar approved these changes Aug 13, 2018

View reviewed changes

Miciah force-pushed the sdn-check-ignore-node's-canonical-name branch from eccffcb to d6bd406 Compare August 15, 2018 20:38

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2018

Miciah force-pushed the sdn-check-ignore-node's-canonical-name branch from d6bd406 to 0ed00c7 Compare August 15, 2018 20:43

pravisankar reviewed Aug 15, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2018

openshift-merge-robot merged commit 03d98d8 into openshift:master Aug 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDN check: Ignore node's canonical name #9511

SDN check: Ignore node's canonical name #9511

Miciah commented Aug 9, 2018 •

edited

Miciah commented Aug 9, 2018

vrutkovs Aug 10, 2018

Miciah Aug 10, 2018

vrutkovs Aug 10, 2018

Miciah Aug 10, 2018

vrutkovs Aug 10, 2018

Miciah Aug 10, 2018

vrutkovs commented Aug 10, 2018

ramr left a comment

ramr Aug 13, 2018

Miciah Aug 13, 2018

ironcladlou commented Aug 13, 2018

Miciah commented Aug 13, 2018

ironcladlou commented Aug 13, 2018

pravisankar left a comment

ironcladlou commented Aug 14, 2018

ironcladlou commented Aug 15, 2018

ironcladlou commented Aug 15, 2018

ironcladlou commented Aug 15, 2018

Miciah commented Aug 15, 2018

Miciah commented Aug 15, 2018

pravisankar Aug 15, 2018

Miciah Aug 16, 2018 •

edited

ironcladlou Aug 16, 2018

ironcladlou commented Aug 16, 2018

openshift-ci-robot commented Aug 16, 2018

		@@ -169,6 +170,31 @@ def execute_module(module_name, args, *_):

		SDNCheck(execute_module, task_vars).run()

		def test_resolve_address():

SDN check: Ignore node's canonical name #9511

SDN check: Ignore node's canonical name #9511

Conversation

Miciah commented Aug 9, 2018 • edited

Miciah commented Aug 9, 2018

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

vrutkovs commented Aug 10, 2018

ramr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironcladlou commented Aug 13, 2018

Miciah commented Aug 13, 2018

ironcladlou commented Aug 13, 2018

pravisankar left a comment

Choose a reason for hiding this comment

ironcladlou commented Aug 14, 2018

ironcladlou commented Aug 15, 2018

ironcladlou commented Aug 15, 2018

ironcladlou commented Aug 15, 2018

Miciah commented Aug 15, 2018

Miciah commented Aug 15, 2018

Choose a reason for hiding this comment

Miciah Aug 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironcladlou commented Aug 16, 2018

openshift-ci-robot commented Aug 16, 2018

Miciah commented Aug 9, 2018 •

edited

Miciah Aug 16, 2018 •

edited