Surpress KeyError when applications are missing #367
Conversation
The following surpresses a KeyError when a application or a remote application is missing. Instead we should return None and correctly handle that a application or a remote-application could be missing. This should be safe to do so and the fact that an update from the all watcher should fill in the gaps eventually. If this is not the case, we should in the future ensure that a remote-application can't raise index/key errors when data is missing. The missing data when raised/thrown criples the library from being useful, instead we should really handle all the cases these exist and correctly insert None or a fallback.
| @@ -35,7 +35,7 @@ deps = | |||
| # default tox env excludes integration and serial tests | |||
| commands = | |||
| # These need to be installed in a specific order | |||
| pip install urllib3==1.22 | |||
| pip install urllib3==1.25.7 | |||
There was a problem hiding this comment.
This is to surpress the 3.8 warnings:
DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working
class AuthContext(collections.Mapping):
|
I tested with this PR and it fixed issue #366 for me. May only reservation (and this probably down my lack of understanding of how juju is modelling the relations) is that I don't understand why there would ever legitimately be no application associated with a relation. |
|
So this is a remoteApplication that it can't find, it should be show in pylibjuju. From my own testing (limited), the pylibjuju calls the AllWatcher Facade and consumes the delta changes from juju. These delta changes should be filling in the internal state of pylibjuju and we should have a local representation of juju internal model. |
|
It makes sense not to throw a KeyError if an absent application is a normal part of the deployment settling. But it sounds like there is another bug because the remote application never gets filled in, even rerunning the test 10 mins after the deployment. |
|
Indeed, I couldn't recreate that locally, but we have reproducing steps to potentially locate the issue. |
After having a long internal discussion around this, it's evident that this will cause errors down the line at some point. Instead we've decided that it's better to throw a custom error, one that can be caught individually to force the user of the library to disconnect and reconnect from the model, to pick up any missing deltas. This can be done in a way to the examples/model.py where retries can be forced to happen when an exception is raised. This code to change this is very rudimental and just exposes a new juju error - JujuEntityNotFoundError. I've also updated the test to fully expose the exception at a unit level.
|
@gnuoy having discussed this further with @mitechie - I believe it's better to expose a custom error - |
|
I don't think this patch will fix the issue for me. I am seeing the issue on a deployed totally stable system, no matter how many times I connect->query->discconnect there is still no data associated with the 'remote-XXX' relation |
|
@gnuoy I'll try to find out why juju/juju isn't sending the delta or why libjuju isn't correctly consuming the delta then. |
|
I added some debug to libjuju and I think the issue is that the remote applications (the apps in a different model) are unknown to this model, which makes sense. So using the bundles I mentioned earlier if the keystone model is in focus then the libjuju representation of the relations is: Those four remotes relate to apps in the other model. |
SimonRichardson
force-pushed
the
key-error
branch
from
09bdefa
to
49a3177
Compare
The following changes consume application offers, so that they're correctly picked up.
SimonRichardson
force-pushed
the
key-error
branch
from
49a3177
to
8f84084
Compare
|
!!build!! |
|
@gnuoy with my latest changes, I can get the ➜ tox -e example examples/model.py
example develop-inst-noop: /home/simon/Documents/python-libjuju
example installed: apipkg==1.5,asynctest==0.13.0,attrs==19.3.0,backcall==0.1.0,bcrypt==3.1.7,bleach==3.1.0,certifi==2019.9.11,cffi==1.13.2,chardet==3.0.4,cryptography==2.8,decorator==4.4.1,docutils==0.15.2,entrypoints==0.3,execnet==1.7.1,idna==2.8,importlib-metadata==0.23,ipdb==0.12.2,ipython==7.9.0,ipython-genutils==0.2.0,jedi==0.15.1,jeepney==0.4.1,-e git+git@github.com:SimonRichardson/python-libjuju.git@8f840848b24e9394e5314f448a093b323339ca82#egg=juju,jujubundlelib==0.5.6,keyring==19.2.0,macaroonbakery==1.2.3,mock==3.0.5,more-itertools==7.2.0,packaging==19.2,paramiko==2.6.0,parso==0.5.1,pexpect==4.7.0,pickleshare==0.7.5,pkginfo==1.5.0.1,pluggy==0.13.0,prompt-toolkit==2.0.10,protobuf==3.10.0,ptyprocess==0.6.0,py==1.8.0,pyasn1==0.4.8,pycparser==2.19,Pygments==2.4.2,pymacaroons==0.13.0,PyNaCl==1.3.0,pyparsing==2.4.5,pyRFC3339==1.1,pytest==5.3.0,pytest-asyncio==0.10.0,pytest-forked==1.1.3,pytest-xdist==1.30.0,pytz==2019.3,PyYAML==5.1.2,readme-renderer==24.0,requests==2.22.0,requests-toolbelt==0.9.1,SecretStorage==3.1.1,six==1.13.0,theblues==0.5.2,toposort==1.5,tqdm==4.38.0,traitlets==4.3.3,twine==3.0.0,urllib3==1.25.7,wcwidth==0.1.7,webencodings==0.5.1,websockets==7.0,zipp==0.6.0
example run-test-pre: PYTHONHASHSEED='1135101242'
example run-test: commands[0] | python examples/model.py
remote-e6933eeb944c4ee880ace6fd8980ff5a
remote-e6933eeb944c4ee880ace6fd8980ff5a
remote-e6933eeb944c4ee880ace6fd8980ff5a
__________________________________________________________________________________________________________________ summary ___________________________________________________________________________________________________________________
example: commands succeeded
congratulations :) |
This should help identify issues in the future.
| """Exception indicating that an entity was not found in the state. It was | ||
| expected that the entity was found in state and this is a terminal | ||
| condition. | ||
| To fix this condition, you should disconnect and reconnect to ensure that |
|
The latest patch fixes the issue for me, thanks @SimonRichardson for all your work on this. |
|
Although I thought this had fixed my cut down test case (I cannot reproduce my earlier success), going back to the original case suggests its not quite fixed. I am now getting: |
|
@gnuoy any chance you could un-comment this line to see what we're missing https://github.com/juju/python-libjuju/pull/367/files#diff-934c79f928f581e5f1d8903f275de68fR904 |
Do you mean the "raise..." line ? I did uncomment that and it didn't raise anything. |
|
I'm going to merge this fix whilst we're still investigating #366 |
The following surpresses a KeyError when a application or a remote
application is missing. Instead we should return None and correctly
handle that a application or a remote-application could be missing.
This should be safe to do so and the fact that an update from the
all watcher should fill in the gaps eventually. If this is not the
case, we should in the future ensure that a remote-application can't
raise index/key errors when data is missing.
The missing data when raised/thrown criples the library from being
useful, instead we should really handle all the cases these exist and
correctly insert None or a fallback.