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

Set Relation.app via JUJU_REMOTE_APP or "relation-list --app" if no units #475

Merged
merged 5 commits into from Feb 17, 2021

Conversation

benhoyt
Copy link
Collaborator

@benhoyt benhoyt commented Feb 10, 2021

Charm errors were happening because we were setting Relation.app from the app of the first unit, but if an event was fired before any units had started up (this happens on relation-created, for example), the app field was None and this caused a KeyError later when charms did things like event.relation.data[event.app] (event.app was None).

The code still starts with the current method (first app of the units), but in the case when that doesn't work because there are no units, we fall back to:

  1. If the relation in question is this event's relation (JUJU_RELATION_ID), then use JUJU_REMOTE_APP as the remote app name.
  2. If it's not this event's relation, use relation-list --app to fetch the remote app name (--app was introduced in Juju 2.8.1).

Fixes #175 and manifests itself as issues like https://bugs.launchpad.net/juju/+bug/1914415

Copy link
Member

@jameinel jameinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this in person, but it isn't complete as is.
The problem is that it assumes JUJU_REMOTE_APP is valid for all relations, while it is only valid for the current JUJU_RELATION_ID.
However, if we tracked those two pieces, we probably have a reasonable fix for #175 that would be backwards compatible with juju 2.7 and 2.8. And while it doesn't solve the overall modeling problem it does, at least, fix a common error case.

@benhoyt benhoyt changed the title Set Relation.app from JUJU_REMOTE_APP directly to avoid KeyError Set Relation.app via JUJU_REMOTE_APP to avoid app=None in many cases Feb 12, 2021
Copy link
Member

@jameinel jameinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is certainly going in the right direction. A few small changes.
We also are missing the direct tests of the _ModelBackend (when the env var is set, when it is set but matches a different relation_id, etc)

ops/model.py Outdated Show resolved Hide resolved
ops/model.py Outdated Show resolved Hide resolved
ops/model.py Outdated Show resolved Hide resolved
ops/model.py Outdated Show resolved Hide resolved
ops/testing.py Outdated Show resolved Hide resolved
test/test_testing.py Outdated Show resolved Hide resolved
@benhoyt benhoyt changed the title Set Relation.app via JUJU_REMOTE_APP to avoid app=None in many cases Set Relation.app via JUJU_REMOTE_APP or "relation-list --app" if no units Feb 15, 2021
@benhoyt
Copy link
Collaborator Author

benhoyt commented Feb 15, 2021

@jameinel Ian pointed out that relation-list --app might exist already, and sure enough, it does! I've updated the code/tests to use that.

ops/model.py Outdated Show resolved Hide resolved
ops/model.py Outdated
return self._run('relation-list', '-r', str(relation_id), '--app',
return_output=True, use_json=True)
except ModelError as e:
if 'relation not found' in str(e):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the frequency of this pattern:

        except ModelError as e:
            if 'relation not found' in str(e):
                raise RelationNotFoundError() from e
            raise

(Which happens 3 times quickly together.)

I do wonder about trying to find commonality there, but it is a relatively short snippet. (it just happens to be easy to typo 'relation no found' and then never triggers but you don't notice.

I'm also not 100% sure what should happen if you ask for a remote application name but the relation doesn't exist. Is 'return None' the right answer here? The other _ModelBackend commands actually raise an exception, though at a higher level they do suppress the error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a _is_relation_not_found helper to unify and avoid typos.

Regarding returning None here -- it seemed right for this function (and is documented and used as such). Given that it's an internal function, I think it's fine, and we can always iterate later if we want to.

test/test_model.py Show resolved Hide resolved
test/test_model.py Show resolved Hide resolved
Copy link
Member

@jameinel jameinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates

Copy link
Member

@hpidcock hpidcock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thank-you

@benhoyt benhoyt merged commit f521841 into canonical:master Feb 17, 2021
@benhoyt benhoyt deleted the fix-relation-keyerror branch February 17, 2021 01:17
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

Successfully merging this pull request may close these issues.

Remote app is not added to RelationMapping for relations in some cases
3 participants