Bug 2004929

I believe something like the following should achieve what I am proposing.

https://github.com/SalDaniele/sos/commit/7cfbaf3c1acaf4c5d1d32720108f916e0a25b69c
https://github.com/SalDaniele/sos/commit/50cf0322076031a3132a7be3abc6f9566613858b

In general, ACK (and thanks for such code snippets). Some minor comments:

1)
https://github.com/SalDaniele/sos/commit/7cfbaf3c1acaf4c5d1d32720108f916e0a25b69c#diff-5f39fe3a37e240e6dc0966f25ab1d0a6c18041ff11438e04a99a83a25d34a59eR256  :

br_protos == [''] or br_protos == [] ? I don't know the output being parsed there to decide, but the current [''] sounds like an oversight?


2)
https://github.com/SalDaniele/sos/commit/7cfbaf3c1acaf4c5d1d32720108f916e0a25b69c#diff-5f39fe3a37e240e6dc0966f25ab1d0a6c18041ff11438e04a99a83a25d34a59eR262  :

Isn't there redundant trailing space in "OpenFlow versions " ? Could you show us example output being parsed?


3)
https://github.com/SalDaniele/sos/commit/7cfbaf3c1acaf4c5d1d32720108f916e0a25b69c#diff-5f39fe3a37e240e6dc0966f25ab1d0a6c18041ff11438e04a99a83a25d34a59eR265

Isn't this for-cycle intended to be executed *inside* "for line in ofp_version_range:" cycle? As that cycle overwrites ver_range, no? (Or does the ofp_version_range outut contain *exactly* once "OpenFlow versions"? If so, we can add "break" to that cycle)


4)
Maybe some comment explaining what is being appended to br_protos will be meaningful - e.g. with an example output being parsed and what strings we need to fetch from it?


5) The "ovsdb-client -f list dump" being *removed* is intentional?


6) /etc/openvswitch/conf.db : can't it have some sensitive data (passwords, keys,..)? Can't the file be "too huge" to be collected? (I dont expect so as anything in few MBs is fully OK; default limit is 25MB we collect)


7) There is nothing common with https://bugzilla.redhat.com/show_bug.cgi?id=1975414 , right?

(In reply to Pavel Moravec from comment #3)
> In general, ACK (and thanks for such code snippets). Some minor comments:
> 
> 1)
> https://github.com/SalDaniele/sos/commit/
> 7cfbaf3c1acaf4c5d1d32720108f916e0a25b69c#diff-
> 5f39fe3a37e240e6dc0966f25ab1d0a6c18041ff11438e04a99a83a25d34a59eR256  :
> 
> br_protos == [''] or br_protos == [] ? I don't know the output being parsed
> there to decide, but the current [''] sounds like an oversight?
> 
> 

'protocols           : []' is an example of line in 252 [1], which sets br_protos_ln to an empty string in the following lines. empty_string.split(", ") returns [''].

I agree it looks weird at a glace, I attached an example running this code on my KIND set up with various ovs-vsctl -t 5 list bridge to demonstrate the code will fall through to the default when "protocols : []"

[1] https://github.com/SalDaniele/sos/commit/7cfbaf3c1acaf4c5d1d32720108f916e0a25b69c#diff-5f39fe3a37e240e6dc0966f25ab1d0a6c18041ff11438e04a99a83a25d34a59eR252

> 2)
> https://github.com/SalDaniele/sos/commit/
> 7cfbaf3c1acaf4c5d1d32720108f916e0a25b69c#diff-
> 5f39fe3a37e240e6dc0966f25ab1d0a6c18041ff11438e04a99a83a25d34a59eR262  :
> 
> Isn't there redundant trailing space in "OpenFlow versions " ? Could you
> show us example output being parsed?
> 

Certainly!

Example output:
sh-5.0# ovs-ofctl -t 5 --version
ovs-ofctl (Open vSwitch) 2.15.0
OpenFlow versions 0x1:0x6

Example parsing:
for line in ofp_version_range:
    if "OpenFlow versions" in line:
        print("line:", line)
        versions = line.split("OpenFlow versions ")[1].split(":")
        print("versions:", versions)
        ver_range = range(int(versions[0], 16), int(versions[1], 16)+1)
        print("ver_range:", ver_range)

ouput:
line: OpenFlow versions 0x1:0x6
versions: ['0x1', '0x6']
ver_range: range(1, 7)

> 
> 3)
> https://github.com/SalDaniele/sos/commit/
> 7cfbaf3c1acaf4c5d1d32720108f916e0a25b69c#diff-
> 5f39fe3a37e240e6dc0966f25ab1d0a6c18041ff11438e04a99a83a25d34a59eR265
> 
> Isn't this for-cycle intended to be executed *inside* "for line in
> ofp_version_range:" cycle? As that cycle overwrites ver_range, no? (Or does
> the ofp_version_range outut contain *exactly* once "OpenFlow versions"? If
> so, we can add "break" to that cycle)
> 

"for line in ofp_version_range:" accesses the line "OpenFlow versions 0x1:0x6"
if it exists, and parses out the hex range (which correspond to the default OFP
hex values[1]).

It then appends each protocol in that range. So in this example above:

br_protos = ['OpenFlow10', 'OpenFlow11', 'OpenFlow12', 'OpenFlow13', 'OpenFlow14', 'OpenFlow15']
as they should all be supported by default [2]

Does that answer the question? Perhaps I missed something.

[1] https://opennetworking.org/wp-content/uploads/2014/10/openflow-switch-v1.5.1.pdf
[2] https://docs.openvswitch.org/en/latest/faq/openflow/

> 
> 4)
> Maybe some comment explaining what is being appended to br_protos will be
> meaningful - e.g. with an example output being parsed and what strings we
> need to fetch from it?
> 

Great, I can add that!

> 
> 5) The "ovsdb-client -f list dump" being *removed* is intentional?
> 

The thinking was that everything in the client list dump is included in conf.db. You
could recreate the list dump using conf.db.

However, the human readable list dump does not allow one to restore ovsdb-server.

Copying both therefore seemed redundant, and could nearly double the size of the report
in large clusters. ovsdb-client also requires interaction with the ovsdb-server, which
should be avoided if possible (as you point out, this is discussed in [7]).

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1975414

> 
> 6) /etc/openvswitch/conf.db : can't it have some sensitive data (passwords,
> keys,..)? Can't the file be "too huge" to be collected? (I dont expect so as
> anything in few MBs is fully OK; default limit is 25MB we collect)
> 
Yes, I know the issue of size was discussed in [1]. I am not sure at scale
how large this could potentially be. I was partially following the
precedent in [2] where this file would be collected in the event "check_6wind"
is true. 

WRT sensitive data, I don't believe this is the case, though I will look further
into this to confirm. It should just contain the configurations for Openv Switch
daemon.

If there are logistical issues that prevent collecting conf.db, using "ovsdb-client
backup" instead of "-f list dump" would allow for restoring ovsdb offline.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1975414
[2] https://github.com/sosreport/sos/blob/main/sos/report/plugins/openvswitch.py#L46

> 
> 7) There is nothing common with
> https://bugzilla.redhat.com/show_bug.cgi?id=1975414 , right?


The same logic here is what lead me to suggesting cp conf.db rather than using something
like ovsdb-client backup (Adrian M is my mentor at RH / on this offline visibility work).


One question I did have is redundant copying of files in the event self.add_copy_spec
is told to add the same file twice. Is this possible?

(In reply to Salvatore Daniele from comment #5)
> (In reply to Pavel Moravec from comment #3)
> > In general, ACK (and thanks for such code snippets). Some minor comments:
> > 
> > 1)
> > https://github.com/SalDaniele/sos/commit/
> > 7cfbaf3c1acaf4c5d1d32720108f916e0a25b69c#diff-
> > 5f39fe3a37e240e6dc0966f25ab1d0a6c18041ff11438e04a99a83a25d34a59eR256  :
> > 
> > br_protos == [''] or br_protos == [] ? I don't know the output being parsed
> > there to decide, but the current [''] sounds like an oversight?
> > 
> > 
> 
> 'protocols           : []' is an example of line in 252 [1], which sets
> br_protos_ln to an empty string in the following lines.
> empty_string.split(", ") returns [''].

Ah right, it is evident from the context (that split method with arguments returns a list of strings, not an empty list).


> 
> I agree it looks weird at a glace, I attached an example running this code
> on my KIND set up with various ovs-vsctl -t 5 list bridge to demonstrate the
> code will fall through to the default when "protocols : []"
> 
> [1]
> https://github.com/SalDaniele/sos/commit/
> 7cfbaf3c1acaf4c5d1d32720108f916e0a25b69c#diff-
> 5f39fe3a37e240e6dc0966f25ab1d0a6c18041ff11438e04a99a83a25d34a59eR252
> 
> > 2)
> > https://github.com/SalDaniele/sos/commit/
> > 7cfbaf3c1acaf4c5d1d32720108f916e0a25b69c#diff-
> > 5f39fe3a37e240e6dc0966f25ab1d0a6c18041ff11438e04a99a83a25d34a59eR262  :
> > 
> > Isn't there redundant trailing space in "OpenFlow versions " ? Could you
> > show us example output being parsed?
> > 
> 
> Certainly!
> 
> Example output:
> sh-5.0# ovs-ofctl -t 5 --version
> ovs-ofctl (Open vSwitch) 2.15.0
> OpenFlow versions 0x1:0x6
> 
> Example parsing:
> for line in ofp_version_range:
>     if "OpenFlow versions" in line:
>         print("line:", line)
>         versions = line.split("OpenFlow versions ")[1].split(":")
>         print("versions:", versions)
>         ver_range = range(int(versions[0], 16), int(versions[1], 16)+1)
>         print("ver_range:", ver_range)
> 
> ouput:
> line: OpenFlow versions 0x1:0x6
> versions: ['0x1', '0x6']
> ver_range: range(1, 7)

I see, thanks for the example.


> 
> > 
> > 3)
> > https://github.com/SalDaniele/sos/commit/
> > 7cfbaf3c1acaf4c5d1d32720108f916e0a25b69c#diff-
> > 5f39fe3a37e240e6dc0966f25ab1d0a6c18041ff11438e04a99a83a25d34a59eR265
> > 
> > Isn't this for-cycle intended to be executed *inside* "for line in
> > ofp_version_range:" cycle? As that cycle overwrites ver_range, no? (Or does
> > the ofp_version_range outut contain *exactly* once "OpenFlow versions"? If
> > so, we can add "break" to that cycle)
> > 
> 
> "for line in ofp_version_range:" accesses the line "OpenFlow versions
> 0x1:0x6"
> if it exists, and parses out the hex range (which correspond to the default
> OFP
> hex values[1]).
> 
> It then appends each protocol in that range. So in this example above:
> 
> br_protos = ['OpenFlow10', 'OpenFlow11', 'OpenFlow12', 'OpenFlow13',
> 'OpenFlow14', 'OpenFlow15']
> as they should all be supported by default [2]
> 
> Does that answer the question? Perhaps I missed something.

Yes, now I got it. I didnt know the output being traversed.


> 
> [1]
> https://opennetworking.org/wp-content/uploads/2014/10/openflow-switch-v1.5.1.
> pdf
> [2] https://docs.openvswitch.org/en/latest/faq/openflow/
> 
> > 
> > 4)
> > Maybe some comment explaining what is being appended to br_protos will be
> > meaningful - e.g. with an example output being parsed and what strings we
> > need to fetch from it?
> > 
> 
> Great, I can add that!

That would be great, such an example let me explain all my questions, I think :)


> One question I did have is redundant copying of files in the event
> self.add_copy_spec
> is told to add the same file twice. Is this possible?


If you include a file in two self.add_copy_spec within the *same* plugin, then it does not matter - since we keep the copy_paths as a set:

https://github.com/sosreport/sos/blob/05be120ef94fecac2aacb44f44bb0d2f29998cf6/sos/report/plugins/__init__.py#L574

If you include a file in two self.add_copy_spec in *different* plugins, then we dont recommend it. Although *usually* nothing happens here (hence just a recommendation) - the file is just redundantly overwritten. But as plugins work independently, they dont expect situations like:
- 1st plugin collects a logfile (still within a sizelimit, hence whole file)
- some new log records appear in the file, such that its size exceeds sizelimit
- 2nd plugin attempts to collect the logfile, due to sizelimit it collects just tail of it. That is stored as a symlink from path/to/logfile to sos_strings dir. Creating the symlink fails if the path/to/file is a regular file created by the 1st plugin.



The commits sounds very good. Let me know if you can prepare an upstream PR from them, or if I shall do it by myself.

Great, thank you for your feedback! I can send it upstream, no worries.

(In reply to Pavel Moravec from comment #6)
> (In reply to Salvatore Daniele from comment #5)
> > (In reply to Pavel Moravec from comment #3)

> The commits sounds very good. Let me know if you can prepare an upstream PR
> from them, or if I shall do it by myself.

I changed the version on the bz to 8.4, as upon further testing we've determined this would need to be applied to sos 4.0, since this will be what OpenStack customers will be running.

Would this be possible? If so, I can move forward with the PR.

Adding OtherQA, thanks for the codechanges and QA offer. Trying to get qa_ack as the last (harder :) ) step.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (sos bug fix and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2022:1998