Fix and enhancements for OVN plugins #1767

danalsan · 2019-08-22T12:15:15Z

[ovn_host] Add support for containerized setups
[ovn_central] Add support to containerized setups
Allow reading commands output from containers
Minor enhancements to the OVN plugin

This patch is adding 'show' commands for both OVN NorthBound and SouthBound databases. It's also fixing a table name when listing the local Open_vSwitch table. Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>

This patch is setting stdin to PIPE when executing the sos commands. Otherwise, it'll use TTY for reading the input but, since 'podman exec' will run in dettached mode, there's no TTY and it'll hang forever (not even the timeout mechanism will make the process exit). Signed-off-by: Jakub Libosvar <jlibosva@redhat.com> Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>

pmoravec · 2019-08-22T14:13:18Z

sos/plugins/ovn_central.py

            'ovn-sbctl lflow-list',
            'ovn-nbctl get-ssl',
            'ovn-nbctl get-connection',
            'ovn-sbctl get-ssl',
            'ovn-sbctl get-connection',
        ]

+


Please remove this extra empty line (Travis fails on it - you can run "pycodestyle" on modified code to spot it locally before committing it).

pmoravec · 2019-08-22T14:17:59Z

sos/plugins/ovn_central.py

+                cmd = ("%s cp %s:/usr/share/openvswitch/ovn-%s.ovsschema "
+                       "/tmp/" % (self._container_runtime,
+                                  self._container_name, db))
+                if self.get_command_output(cmd)['status'] != 0:


What is the purpose to copy the files? (to get the copy command output that is usefull for some troubleshooting?)

Is it safe to expose that file to publicly available /tmp directory - can't it be misused by some intruder?

(I think I got this; you collect the file in add_database_output and I guess it is really safe to expose them since non-containerised environment exposes them in similar way - am I right?)

What is the purpose to copy the files? (to get the copy command output that is usefull for some troubleshooting?)

Is it safe to expose that file to publicly available /tmp directory - can't it be misused by some intruder?

(I think I got this; you collect the file in add_database_output and I guess it is really safe to expose them since non-containerised environment exposes them in similar way - am I right?)

Yes, you're right. On non containerized environments, this is exposed in the host filesystem while in containerized environments we want to read the files to figure out what tables to pull data from.

Re. security, these files are just the DB schema and are publicly available anyways in the OVS/OVN repositories, they do not contain any data but just the schema definition.

TurboTurtle · 2019-08-22T15:02:26Z

sos/plugins/ovn_central.py

+            if self._container_runtime == 'podman':
+                dst = self.get_command_output(
+                    "podman mount %s" % self._container_name)['output']
+                cmd = "cp " + dst.rstrip()


podman cp exists and matches the docker syntax to copy files out of a container. In no way should we be mounting anything anywhere in a plugin.

This whole block to construct the command can be reduced to:

if containerized: cmd = "%s cp %s:" % (self._container_runtime, self._container_name)

podman cp exists and matches the docker syntax to copy files out of a container. In no way should we be mounting anything anywhere in a plugin.

I took a RHEL 8.0 base system where podman cp doesn't exist. Do you mean is it fair to assume that whatever version of podman is now packaged in RHEL8 would support that command? If so, I'll fallback to my previous version.

[root@controller-0 ovn_central]# podman cp
Command "cp" not found.
See podman --help.

It looks like podman cp is a 1.1 feature, whereas 8.0 has 1.0 podman. I'd imagine the next rebase for podman in RHEL will update it to at least 1.1 (Fedora is on 1.4 right now for reference).

In any event, I am still completely against doing mount operations within a plugin. Maybe there needs to be some logic to gate against podman-1.0, or we just accept that it won't work for that version of podman.

TurboTurtle · 2019-08-22T15:03:27Z

sos/plugins/ovn_central.py

+
+            for db in ['nb', 'sb']:
+                cmdline = ("%s/usr/share/openvswitch/ovn-%s.ovsschema "
+                           "/tmp/" % (cmd, db))


The above being said, I'm not a fan of sos copying files out of a container to a location in /tmp just to read them and then do nothing else.

Can we not just run the db query commands directly in the container and record that output?

sos/plugins/ovn_central.py

danalsan · 2019-08-22T21:07:36Z

On 22 Aug 2019, at 22:01, Jake Hunsaker ***@***.***> wrote: @TurboTurtle commented on this pull request. In sos/plugins/ovn_central.py: > @@ -65,12 +86,37 @@ def setup(self): ] schema_dir = '/usr/share/openvswitch' + if containerized: + if self._container_runtime == 'podman': + dst = self.get_command_output( + "podman mount %s" % self._container_name)['output'] + cmd = "cp " + dst.rstrip() It looks like podman cp is a 1.1 feature, whereas 8.0 has 1.0 podman. I'd imagine the next rebase for podman in RHEL will update it to at least 1.1 (Fedora is on 1.4 right now for reference). In any event, I am still completely against doing mount operations within a plugin. Maybe there needs to be some logic to gate against podman-1.0, or we just accept that it won't work for that version of podman.

As I said, the directory is accesible even without mounting but I need to find it some how; it is not a real mount and anyways it’s just mounted right after the file is fetched. What then about using glob if Python 3 is installed to search for the files in /var/lib and “docker cp” otherwise? We need the ovn plugin to work on RHEL8. Can you suggest something that would work for you and later on when podman > 1.0 is available, we will just use “podman cp”?

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

TurboTurtle · 2019-08-22T21:36:08Z

As I said, the directory is accesible even without mounting but I need to find it some how; it is not a real mount and anyways it’s just mounted right after the file is fetched. What then about using glob if Python 3 is installed to search for the files in /var/lib and “docker cp” otherwise? We need the ovn plugin to work on RHEL8. Can you suggest something that would work for you and later on when podman > 1.0 is available, we will just use “podman cp”?

As I said, I am still not onboard with performing file copies on the host. podman cp is a last resort in my mind, if we even agree to go that route at all.

What is represented by the table names in the ovn-(nb|sb).ovsschema files? Do they map to some other component within OVN that can be listed with some other ovn-ctl command? I would find it odd if an end user needed to go digging through schema files in order to run valid ovn-nbctl list $schema on their own.

If some kind of list command like that is present, we can simplify this substantially and making it container-compatible would be trivial.

danalsan · 2019-08-26T16:18:34Z

@TurboTurtle I changed the code to read the file inside the container to memory. If it's not a containerized setup, the old behavior is kept. This way we don't mount or copy any files into the host filesystem and it shouldn't be a problem as the schema file shouldn't grow much (it doesn't contain any data but just the tables, columns and relationships). Let me know if this works for you.
Thanks,
daniel

bmr-cymru · 2019-08-26T16:25:55Z

This pull request introduces 2 alerts when merging 8178714 into f9e0a23 - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Wrong number of arguments in a call

Warning - Automated code review for sosreport/sos will be disabled on October 1, 2019. You can avoid this by installing the LGTM.com GitHub App. Read about the benefits of migrating to GitHub Apps in the blog.

Comment posted by LGTM.com

danalsan · 2019-08-26T18:48:16Z

The travis failures are unrelated to this PR, the offending lines belong to the following commit:

^2453c238 (Jake Hunsaker 2019-06-07 13:08:12 -0400 926) strfile = file_name.replace(os.path.sep, ".") + ".tailed"
^2453c238 (Jake Hunsaker 2019-06-07 13:08:12 -0400 927) self.add_string_as_file(tail(_file, sizelimit), strfile)

I can send a separate PR to fix it.

TurboTurtle · 2019-08-26T19:46:36Z

The offending commit is actually 9c421f8

But we can get that cleaned up in master in a moment before we close 3.8.

danalsan · 2019-08-27T07:41:13Z

The offending commit is actually 9c421f8

But we can get that cleaned up in master in a moment before we close 3.8.

ack, thanks a lot.
Let me know please if there's anything you'd like me to change from this PR.

pmoravec · 2019-09-05T07:19:13Z

sos/utilities.py

@@ -147,7 +147,7 @@ def _child_prep_fn():
        else:
            expanded_args.append(arg)
    try:
-        p = Popen(expanded_args, shell=False, stdout=PIPE,
+        p = Popen(expanded_args, stdin=PIPE, shell=False, stdout=PIPE,


Why this change, please?

@bmr-cymru and @TurboTurtle , any comment here? Could it have some drawbacks?

Thanks a lot for taking a look.
Without that change, podman hangs for me [0] and with strace I could see that the process is receiving SIGTTIN all the time.

[0] https://bugzilla.redhat.com/show_bug.cgi?id=1732525

pmoravec · 2019-09-05T07:21:50Z

sos/plugins/ovn_central.py

+                    "Could not open DB schema file %s: %s" % (filename, ex))
+                return
+        return [
+            table for table in six.iterkeys(db['tables']) if table not in skip]


Theoretically, the JSON file might lack 'tables' (can we 100% guarantee the json file cant be corrupted or empty?). If this would happen, an uncaught exception would be raised.

Good point, plus it was handled before my patch. Will fix.
Thanks!

pmoravec · 2019-09-05T07:26:56Z

sos/plugins/ovn_central.py

+            schema_dir, 'ovn-sb.ovsschema'), ['Logical_Flow'])
+
+        self.add_database_output(nb_tables, cmds, 'ovn-nbctl')
+        self.add_database_output(sb_tables, cmds, 'ovn-sbctl')


Will be "cmds" variable really updated in the cmds.append('%s list %s' % (ovn_cmd, table)) call inside add_database_output? As:

>>> def plus(a,b,c): ... c = a+b ... >>> s1=3 >>> s2=5 >>> s=6 >>> plus(s1,s2,s) >>> s 6

(this could be over-worked by using "self.cmds" in setup and add_database_output and not passing "cmds" as its argument)

As it is a list, we can be sure that the cmds variable will be updated. (My patch is not changing the behavior though, it was like that before). For example:

def _bb(param):
param.append(3)

aa = [1, 2]
_bb(aa)
print(aa)

$ python3.7 test.py
[1, 2, 3]

$ python2.7 test.py
[1, 2, 3]

Ah right, forgot it is a list..

This patch is adding support in ovn_central plugin to containerized setups. Now it's detecting if the OVN central services are running in container and execute the relevant commands inside it. The support covers both podman and docker runtimes. Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>

Prior to this patch, ovn_host was disabled on containerized setups due to the fact that ovn-controller package is not installed in the host. This patch fixes it by checking if the ovn-controller process is running. Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>

bmr-cymru · 2019-09-11T15:22:48Z

Please make sure patches are in [component] description of change format. I've fixed the first one on this branch (and also split it into two separate patches - it was adding two items to one plugin and fixing a completely unrelated bug in another...).

I've dropped the stdin=PIPE patch as this is working around a problem in podman when it is run without a controlling TTY.

Prior to this patch, ovn_host was disabled on containerized setups due to the fact that ovn-controller package is not installed in the host. This patch fixes it by checking if the ovn-controller process is running. Resolves: sosreport#1767 Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> Signed-off-by: Bryn M. Reeves <bmr@redhat.com>

danalsan added 2 commits July 18, 2019 14:08

Minor enhancements to the OVN plugin

42153dc

This patch is adding 'show' commands for both OVN NorthBound and SouthBound databases. It's also fixing a table name when listing the local Open_vSwitch table. Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>

pmoravec reviewed Aug 22, 2019

View reviewed changes

TurboTurtle requested changes Aug 22, 2019

View reviewed changes

pmoravec reviewed Sep 5, 2019

View reviewed changes

danalsan added 2 commits September 5, 2019 10:40

bmr-cymru closed this in a895bf4 Sep 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and enhancements for OVN plugins #1767

Fix and enhancements for OVN plugins #1767

danalsan commented Aug 22, 2019

pmoravec Aug 22, 2019

pmoravec Aug 22, 2019 •

edited

Loading

danalsan Aug 22, 2019

TurboTurtle Aug 22, 2019

danalsan Aug 22, 2019 •

edited

Loading

TurboTurtle Aug 22, 2019

TurboTurtle Aug 22, 2019

danalsan commented Aug 22, 2019 via email

TurboTurtle commented Aug 22, 2019

danalsan commented Aug 26, 2019 •

edited

Loading

bmr-cymru commented Aug 26, 2019

danalsan commented Aug 26, 2019

TurboTurtle commented Aug 26, 2019

danalsan commented Aug 27, 2019

pmoravec Sep 5, 2019

danalsan Sep 5, 2019

pmoravec Sep 5, 2019

danalsan Sep 5, 2019

pmoravec Sep 5, 2019

danalsan Sep 5, 2019

pmoravec Sep 10, 2019

bmr-cymru commented Sep 11, 2019

Fix and enhancements for OVN plugins #1767

Fix and enhancements for OVN plugins #1767

Conversation

danalsan commented Aug 22, 2019

Choose a reason for hiding this comment

pmoravec Aug 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danalsan Aug 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danalsan commented Aug 22, 2019 via email

TurboTurtle commented Aug 22, 2019

danalsan commented Aug 26, 2019 • edited Loading

bmr-cymru commented Aug 26, 2019

danalsan commented Aug 26, 2019

TurboTurtle commented Aug 26, 2019

danalsan commented Aug 27, 2019

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

bmr-cymru commented Sep 11, 2019

pmoravec Aug 22, 2019 •

edited

Loading

danalsan Aug 22, 2019 •

edited

Loading

danalsan commented Aug 26, 2019 •

edited

Loading