external: remove support of python2 for external clusters

[parth-gr]

Commit1: Fix the pylint CI

Commit2: Remove support of python2 for external clusters

closes: #14212

Checklist:

• Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
• Reviewed the developer guide on Submitting a Pull Request
• Pending release notes updated with breaking and/or notable changes for the next minor release.
• Documentation has been updated, if necessary.
• Unit tests have been added, if necessary.
• Integration tests have been added, if necessary.

[parth-gr]

I am importing packages,

 from ipaddress import ip_address
  from ipaddress import IPv4Address

And pylint is treating them as variables,

deploy/examples/create-external-cluster-resources.py:592:27: E0606: Possibly using variable 'ip_address' before assignment (possibly-used-before-assignment)
deploy/examples/create-external-cluster-resources.py:593:26: E0606: Possibly using variable 'IPv4Address' before assignment (possibly-used-before-assignment)

used here

ip_type = type(ip_address(endpoint_str_ip))
            if ip_type == IPv4Address:

Looks like a pylint bug, @rkachach @NymanRobin ??

[NymanRobin]

The error originates from here. In this case, the "variables" are actually a function and a class, so the error message could indeed be improved. However, if called with Python 2, this will throw an error. Therefore, I believe the style error is still valid. The example below, tries to highlight the issue

if True == False:
    foo = "bar"
print(foo)

The easiest way to fix this is to give the foo a baseline value and the style error and the potential bug is removed

foo = ""
if True == False:
    foo = "bar"
print(foo)

However, in this particular example it is a bit harder as the ipaddress library was added in python 3.3

Therefore, my suggestion in the issue was to completely drop Python 2 support. This would resolve the style issue and make the code more readable by removing the if-statement that checks for the Python version. While defining the "variables" as None is an option, it wouldn't make the Python 2 code functional. It seems that the current code cannot fully operate with Python 2 anyway, as these two variables are not defined. It might be partially functional at best. 😄

The other option I would consider if for some reason we care about supporting python2 is rewriting this functions for example using the sockets library

Hope this is helpful!

[parth-gr]

We want to keep the python2 support

[NymanRobin]

Okay, interesting. I think Python 2 probably should be part of the tests somehow then also otherwise this is very easy to break.

I think the best plan of action would be to remove the dependencies on ipaddress and use socket instead. Something like socket.inet_pton(socket.AF_INET, ip_str) should be able to determine the IPv type by checking if it throws error. What do you think about this approach, @parth-gr? Do you think this would be a feasible fix?

I also found this as an option: https://github.com/phihag/ipaddress, but I think that would be quite hard to make explicit as there is no real dependency management, especially in the Python 2 case.

Others might of course have some other opinions on how this should be fixed, which are more than welcome. 😄

[rkachach]

@parth-gr IMHO dropping Python2 support is something you may seriously consider as it's has been EOL for more than 4 years: https://devguide.python.org/versions/. All modern distros come with Python3 so I don't see the need to stick to a very old version with is already EOL.

[BlaineEXE]

Could you remind me why we want to keep py2 support? Supporting only py3 would vastly simplify things.

IIRC, this had something to do with some Ceph versions/containers that still relied on py2. However, it's been a while since we talked about this, and things have a tendency to change over time. It's possible this isn't as necessary anymore, and this seems like the right time to revisit the discussion.

[parth-gr]

Yes it is mainly depend on the RHCS version, what it supports

[NymanRobin]

Yes I agree @parth-gr the imports are not variables, but that is maybe a bit more cosmetic than functional but could be discussed with pylint community how easy it is to detect the "type" of import.
However the actual error that pylint reports I think is correct, tried to open it up in the comment above

[BlaineEXE]

Could you update the commit so that

1. the subject is more specific than 'pylint issue', and
2. add text in the subject to clarify what the commit changes ?

[BlaineEXE]

@NymanRobin and @rkachach are raising very good points in this thread around py2, and I would like to find some resolution here.

IMO, this is a great time to revisit py2 requirements. If, after review, we are still confident that we need to support py2, we should also determine if risk is great enough for us to consider adding a follow-up issue to add py2 to the test matrix.

[NymanRobin]

Awesome that the python2 could be removed!

[BlaineEXE]

I think this looks good, but can the commits be squashed? It doesn't make sense to me to make a bunch of changes in c1 and then undo the need for them entirely in an immediate follow-up commit.

[parth-gr]

I think this looks good, but can the commits be squashed? It doesn't make sense to me to make a bunch of changes in c1 and then undo the need for them entirely in an immediate follow-up commit.

Done

[BlaineEXE]

Looks good, and I'm glad to have @NymanRobin 's help and thumbs-up here. Thanks :)