Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contribute etc3 discovery provider #126

Closed
scottslewis opened this issue Jul 7, 2024 · 26 comments
Closed

Contribute etc3 discovery provider #126

scottslewis opened this issue Jul 7, 2024 · 26 comments

Comments

@scottslewis
Copy link
Collaborator

The current etcd discovery provider uses the python etcd (v2) client library. Etcd servers now almost exclusively uses etcd v3 and so a new client library has to be used, and parts of the etcd_discovery provider reimplemented.

@scottslewis
Copy link
Collaborator Author

Did some looking:

etcd-io/etcd#13285

https://etcd.io/docs/v3.5/integrations/#python

Unfortunately it looks like many of the etcdv3 python clients are not very active currently (as summarize at end of issue 13285 above very recently). I will do some more looking but given the protoc-generation of python code, it may be easier for me to implement the ipopo etcdv3 discovery provider directly from the generated classes. I've done this before...e.g. for java etcdv3 provider: https://github.com/ECF/etcd3-discovery-provider.

Unfortunately, however, getting an etcdv3 discovery provider is not going to be quick and easy.

I may see if I can get the etcdv2 discovery provider working (with etcdv2 server of course) for the interim.

@scottslewis scottslewis changed the title Replace etcd client library used in etcd discovery provider with an appropriate and quality etcd v3 client library Contribute etc3 discovery provider Aug 14, 2024
@scottslewis
Copy link
Collaborator Author

I've decided to use https://github.com/kragniz/python-etcd3

I've got an initial implementation of the etcd3-based discovery provider and am testing it.

@tcalmant: I can't remember how the ipopo logging config works. Is there an easy way (without creating whole logging config) ...e.g. via system properties to turn on the logging for just one (or a few) bundles? (without using shell to do so?) Thanksinadvance.

@tcalmant
Copy link
Owner

Thanks
Regarding the logging, iPOPO uses the standard Python logging module.
There is also a LogService in the misc services, but it relies on logging.

@scottslewis
Copy link
Collaborator Author

Thanks @tcalmant.

Another question. At a few places in the RSA code are lines like this:

if TYPE_CHECKING:
import pelix.rsa.remoteserviceadmin as rsa_impl

At least on my system, TYPE_CHECKING is False, and so these imports don't occur. e.g.

https://github.com/tcalmant/ipopo/blob/v3/pelix/rsa/providers/distribution/__init__.py#L63

In this case and a couple of others, the python compiler can't compile this line:

https://github.com/tcalmant/ipopo/blob/v3/pelix/rsa/providers/distribution/__init__.py#L261

because TYPE_CHECKING is False and so the import does not occur and rsa_impl isn't defined.

I've just been removing if TYPE_CHECKING and that has solved my local problems...but should I commit that? Or do something else in my local environment? It seems that TYPE_CHECKING is set to False (in my typing library) statically (const).

Please lmk

@tcalmant
Copy link
Owner

Hi @scottslewis

The TYPE_CHECKING flag is set only when an IDE is parsing the file to check type hints. It is always false on runtime.
This is useful to avoid import loops or to avoid loading a heavy module when not necessary.

The rule of thumb is: if a type is used only in type hints, it can be loaded in the TYPE_CHECKING block. In all other cases, it must be loaded outside that block.

If you remember iPOPO in Python 2, I had some imports that were often marked as unused as I was using them in type hint comments: those would nowadays be loaded in the TYPE_CHECKING block.

@scottslewis
Copy link
Collaborator Author

Hi @tcalmant

ok thanks. I get errors in IDE when using with pydev as per the file linked to above. Do I have to configure the IDE or interpreter somehow to avoid these errors?

@scottslewis
Copy link
Collaborator Author

Hi @tcalmant

I was googling around and found this question:

https://stackoverflow.com/questions/72922764/cannot-cast-to-class-imported-on-condition-of-type-checking

It suggests that the if TYPE_CHECKING:
import rsa_impl.RemoteServiceAdminImpl

could be removed and the reference to the class name could be simply changed to a string...e.g.

    import_regs = cast('rsa_impl.RemoteServiceAdminImpl', self._rsa)._get_import_regs()

FWIW this seems to work in my environment, as the parser lets it through (no if TYPE_CHECKING) with no error from missing import.

Another possibility: I happen to be using python 3.11. Should I downshift (to what?) rather than make changes as above? I understand from googling that the 3.9 -> 3.10 changes something about the default wrt typing. Thanks.

@tcalmant
Copy link
Owner

Hi @scottslewis

I'm against removing the typing as it is right now.
I think the issue is coming from PyDev, as PyCharm and VSCode have no issue there.

Could you try setting up PyDev to use Mypy for type checking?
See: https://www.pydev.org/manual_adv_mypy.html

Also note that iPOPO v3 drops support for Python version lower than 3.10, I'm working on it with both Python 3.10 and 3.12

@tcalmant
Copy link
Owner

tcalmant commented Aug 21, 2024

Hi @scottslewis,

I have an issue with the etcd3 library complaining about the version of protobuf being too recent. It seems it requires protobuf 3.x (3.20.3) and is incompatible with 4+.

Pip installs protobuf 5.27.3 as it's not explicitly limited neither by etcd3 setup nor requirements.txt.

The issue is that osgiservicebridge 1.5.3+ (in requirements.txt) explicitly requires protobuf 4.22.5.
Could the osgiservicebridge work with an older version?

Also, which version of protobuf do you have in your development environment? (pip list | grep protobuf)

@tcalmant
Copy link
Owner

I've prepared PR #130 that should fix your typing issues

@scottslewis
Copy link
Collaborator Author

Hi @tcalmant. I've built and deployed to pypi version 1.5.6. I should not have used protobuf 4, and I need to make the jump to protobuf 5, but that will take a little work on osgiservicebridge. In any event, I've got osgiservicebridge 1.5.6 out and have pr #132

@tcalmant
Copy link
Owner

Hi @scottslewis
It seems there is a version of etcd3 compatible with Protobuf 4 here but without any available source repository.
I wonder if it will be necessary to fork the python-etcd3 repository to upgrade it and publish a version on PyPI.

@tcalmant
Copy link
Owner

Hi @scottslewis

I've added back CI to the iPOPO repository.
I've updated etcd3 tests to make sure the assertions errors are raised at the right place.
I don't understand why the etcd3 tests are sometimes failing, I think it's mostly a startup timing issue.

I'll try to mitigate #133 and let you check afterwards. The good news is that it seems the v3 branch is in good shape and might soon be ready for a release.

@scottslewis
Copy link
Collaborator Author

Hi @scottslewis It seems there is a version of etcd3 compatible with Protobuf 4 here but without any available source repository. I wonder if it will be necessary to fork the python-etcd3 repository to upgrade it and publish a version on PyPI.

Hi @tcalmant. There are a couple of issues at play here. First (and probably most important atm), grpc itself doesn't yet support protobuf 4 or 5. I'm not completely sure why...but I believe it probably has to do with backward compatibility. In any event, since both java etcd3 and python etcd3 along with java grpc distribution provider are stuck on protobuf 3 for now because of grpc, that has to be solved for grpc before it makes any sense to upgrade the kragniz impl.

Another alternative would be to continue looking for another python impl of etcd3.

Also, it could make sense at some point to remove the kragniz impl completely and just use the protoc-generated stubs and substitute in our own code on that. The etcd3 client doesn't use much of the protoc-generated API, and the kragniz library is a thin layer on the generated stubs. I'm remain shocked that there isn't a full, maintained, python impl of etcd3 that I can find...since etcd3 is a central part of kubernetes....especially as python gets more popular with AI.

@tcalmant
Copy link
Owner

Hi @scottslewis

I finally found the issue in the etcd3 test, it was a timing issue linked to the reuse of a message queue between the processes.
We now use 2 queues in the test to avoid reading the shutdown order we sent to the child before the latter propagate its ready state.

Quick question: should we keep the etcd2 discovery?
I've merged PR #137 as the python-etcd library is still working for now, but urllib3-related deprecation warnings are visible.
It could be interesting to keep it until it fails in order to have a rendez-vous point between iPOPO v1 and v3.

First (and probably most important atm), grpc itself doesn't yet support protobuf 4 or 5. I'm not completely sure why...but I believe it probably has to do with backward compatibility. In any event, since both java etcd3 and python etcd3 along with java grpc distribution provider are stuck on protobuf 3 for now because of grpc, that has to be solved for grpc before it makes any sense to upgrade the kragniz impl.

Regarding protobuf, we will have to upgrade at some point as Python 3.11 and 3.12 are showing deprecation warnings related to it in etcd3:

[...]/etcd3/etcdrpc/rpc_pb2.py:248: DeprecationWarning: Call to deprecated create function FieldDescriptor().
Note: Create unlinked descriptors is going to go away. Please use get/find descriptors from generated code or query the descriptor_pool.

Also, it could make sense at some point to remove the kragniz impl completely and just use the protoc-generated stubs and substitute in our own code on that. The etcd3 client doesn't use much of the protoc-generated API, and the kragniz library is a thin layer on the generated stubs.

I think that will become necessary when we'll have to support Python 3.14/3.15 (the deprecation warning doesn't tell the API end of support).
If we start working on that package, it should be in a separate repository, as it will require support and releases unrelated to the iPOPO life cycle.

I'm remain shocked that there isn't a full, maintained, python impl of etcd3 that I can find...since etcd3 is a central part of kubernetes....especially as python gets more popular with AI.

I suppose K8S is mostly used via command line or via NodeJS (as Microsoft provides an official etcd3 client for it).

@scottslewis
Copy link
Collaborator Author

Hi @scottslewis

I finally found the issue in the etcd3 test, it was a timing issue linked to the reuse of a message queue between the processes. We now use 2 queues in the test to avoid reading the shutdown order we sent to the child before the latter propagate its ready state.

ok, sounds good.

Quick question: should we keep the etcd2 discovery? I've merged PR #137 as the python-etcd library is still working for now, but urllib3-related deprecation warnings are visible. It could be interesting to keep it until it fails in order to have a rendez-vous point between iPOPO v1 and v3.

Yes, I think it should be kept. The etcd3 server still has v2 compatibility...I'm sure because some consumers still use it for their services and don't want to be forced to upgrade server-side.

> > Regarding protobuf, we will have to upgrade at some point as Python 3.11 and 3.12 are showing deprecation warnings related to it in etcd3: > > ``` > [...]/etcd3/etcdrpc/rpc_pb2.py:248: DeprecationWarning: Call to deprecated create function FieldDescriptor(). > Note: Create unlinked descriptors is going to go away. Please use get/find descriptors from generated code or query the descriptor_pool.

Yeah. I'll try to untangle the protobuf version, grpc version, other libraries versions (on java side particularly) questions, etc. It makes life difficult when a library several layers down doesn't stay backwards compatible.

> > I think that will become necessary when we'll have to support Python 3.14/3.15 (the deprecation warning doesn't tell the API end of support). If we start working on that package, it should be in a separate repository, as it will require support and releases unrelated to the iPOPO life cycle.

ok.

I'm remain shocked that there isn't a full, maintained, python impl of etcd3 that I can find...since etcd3 is a central part of kubernetes....especially as python gets more popular with AI.

I suppose K8S is mostly used via command line or via NodeJS (as Microsoft provides an official etcd3 client for it).

Yeah...etcd3 is pretty heavily used now...I expect at least partially because of kubernetes. It makes the paucity of python-based clients surprising to me...especially since the proto-generated stub code does most of the actual work.

@tcalmant
Copy link
Owner

In that case, I'll review the documentation and readme this weekend and prepare a 3.0.0 release on Monday.

@scottslewis
Copy link
Collaborator Author

In that case, I'll review the documentation and readme this weekend and prepare a 3.0.0 release on Monday.

Super, thanks! If there is anything else I can do to help with the release please lmk.

One thing I'm likely to do is to put a note about the release to osgi-technology-dev mailing list at Eclipse. If there are other forums where you would like me to make an announcement please lmk.

@tcalmant
Copy link
Owner

Hi @scottslewis

Regarding the docs, I found a link to a tutorial on the Eclipse Wiki, which will soon shutdown. Do you plan to move it somewhere else?

Also the link the description of Py4j-RemoteServicesProvider should be updated to 3.0.0 (when ready).

One thing I'm likely to do is to put a note about the release to osgi-technology-dev mailing list at Eclipse. If there are other forums where you would like me to make an announcement please lmk.

Thanks :)
I'll send a release note to the usual Python and iPOPO mailing lists.
I'm not aware of other forums that would be interested.

@scottslewis
Copy link
Collaborator Author

Hi @scottslewis

Regarding the docs, I found a link to a tutorial on the Eclipse Wiki, which will soon shutdown. Do you plan to move it somewhere else?

Yes. The EF has required all projects move to the github/gitlab wiki. It's another 'unfunded mandate' for the non-corp run projects like ECF. So I'll get around to doing the conversion, but it may not be right away because it's just me at this point.

Also the link the description of Py4j-RemoteServicesProvider should be updated to 3.0.0 (when ready).

Ok, thanks.

@tcalmant
Copy link
Owner

Hi @scottslewis
FYI, iPOPO 3.0.0 has been released.
I think we can close this issue, WDYT?

@scottslewis
Copy link
Collaborator Author

Thanks Thomas. Yeah I'll close this issue. I posted your notice to osgi-technology-dev mailing list with copy of your notice.

I've got another release of the etcd3 discovery provider planned already: Etcd3 has lots of configuration (e.g. creds, certs for security, performance tweaks for different use cases, server-side replication/distribution). In any event, I'll open up a new issue for enhancing etcd3 configuration.

First though I'm going to enhance the java etcd3 client here

ECF/etcd3-discovery-provider#1

@scottslewis
Copy link
Collaborator Author

Hi @tcalmant

I've done some research on protobuf, grpc versions, etc and have learned some things. This is for my memory as well as for you:

  1. I posted this question on grpc discussion forum:

https://groups.google.com/g/grpc-io/c/us5bWRHyQcA/m/3xvaVgDaCgAJ

Earlier today I got this response

https://groups.google.com/g/grpc-io/c/us5bWRHyQcA/m/jRvDhpXaCgAJ

This response pointed to this grpc issue:

grpc/grpc-java#11015

which describes the fact that protobuf 4+ broke compatibility with protobuf-3...and this broke all of grpc (as well as most of the proto3-based applications of protobuf...i.e. all those that are actually running).

If you jump to here however: grpc/grpc-java#11015 (comment)

You will see that with protobuf 4.28 restores proto3 compatibility (not actually released yet, but soon), meaning that protobuf 4 generated code will be supported in grpc-java 1.67...once it is released...i.e. 'We're targeting upgrading in 1.67' from ejona.

I have a followup question to above thread asking about python and other lang upgrade to protobuf 4.28

My expectation is that I will wait a couple of weeks to get protobuf 4 support back in grpc (both java and python) and then update grpc-based across all my components at once...e.g. etcd3 discovery (java), rsa grpc distribution (java), etcd3 discovery (python/ipopo), and I will introduce a grpc-python-based distribution provider for ipopo as well at same time. Then everything can be at latest versions of everything going forward: protobuf 4+, grpc 1.67+, java-python etcd3 discovery (osgi and ipopo), and java-python grpc-distribution-based services (which will provide two python<->java discovery/distribution mechanisms for osgi<->ipopo services....i.e. py4j and grpc. That's the plan, anyway.

@scottslewis
Copy link
Collaborator Author

Hi @scottslewis

I finally found the issue in the etcd3 test, it was a timing issue linked to the reuse of a message queue between the processes. We now use 2 queues in the test to avoid reading the shutdown order we sent to the child before the latter propagate its ready state.

Quick question: should we keep the etcd2 discovery? I've merged PR #137 as the python-etcd library is still working for now, but urllib3-related deprecation warnings are visible. It could be interesting to keep it until it fails in order to have a rendez-vous point between iPOPO v1 and v3.

> Regarding protobuf, we will have to upgrade at some point as Python 3.11 and 3.12 are showing deprecation warnings related to it in etcd3: > > ``` > [...]/etcd3/etcdrpc/rpc_pb2.py:248: DeprecationWarning: Call to deprecated create function FieldDescriptor(). > Note: Create unlinked descriptors is going to go away. Please use get/find descriptors from generated code or query the descriptor_pool. ```

@tcalmant where/how was this DeprecationWarning generated? Reason I want to know: I believe it might make sense to move away from using the kragniz python-etcd3 at all, as it doesn't seem to be getting any maintenance attention. Also, I believe that I might be able to fix this simply by regenerating the pb2 code with protoc/grpc with recent versions and if that fixes this warning I'll start from that with simply leaving out the python-etcd3 code and writing the code myself for using the grpc-generated stubs.

But, I would like to reproduce this warning myself, so I can locally test out my newly-generated stub code on my forked copy of the python-etcd3 repo.

Thanks.

Scott

Also, it could make sense at some point to remove the kragniz impl completely and just use the protoc-generated stubs and substitute in our own code on that. The etcd3 client doesn't use much of the protoc-generated API, and the kragniz library is a thin layer on the generated stubs.

I think that will become necessary when we'll have to support Python 3.14/3.15 (the deprecation warning doesn't tell the API end of support). If we start working on that package, it should be in a separate repository, as it will require support and releases unrelated to the iPOPO life cycle.

I'm remain shocked that there isn't a full, maintained, python impl of etcd3 that I can find...since etcd3 is a central part of kubernetes....especially as python gets more popular with AI.

I suppose K8S is mostly used via command line or via NodeJS (as Microsoft provides an official etcd3 client for it).

@tcalmant
Copy link
Owner

tcalmant commented Sep 4, 2024

Hi @scottslewis

The deprecation warnings are visible when running the tests with pytest on Python 3.11 or 3.12 (preferred), regrouped at the end of the run in the "Warnings summary".
For example, they were visible during that GitHub action run: https://github.com/tcalmant/ipopo/actions/runs/10508407301/job/29112236342#step:7:144

You can also use the -W once::DeprecationWarning option to display them when running a script (python -W once::DeprecationWarning -m pelix.shell)

Thomas

@scottslewis
Copy link
Collaborator Author

Hi @tcalmant

Thanks for the info. I've gotten the warnings (when running tests) and I've cloned python-etcd3 and regenerated the grpc stubs from the etcd3 protofiles:

https://github.com/scottslewis/python-etcd3

the regenerated pb2 files (with protobuf 5.27.2) are in etcd.etcdrpc package.

I'm able to run the test_etcd3_discovery tests with the old generated pb2 files (warnings occur) and the protobuf 5.27.2-generated pb2 files (pushed to master on my fork...no warnings). I've also updated the version on my fork to 0.13.0 from 0.12.0 which is the last released version. This takes care of the warnings.

I'll submit pr upstream, but I'm not sanguine about it being applied and released...so I'll continue looking into removing python-etcd3 code as it's mostly just a wrapper around protoc-generated stubs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants