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
Test Harness should support peer relations implicitly if defined #547
Comments
So we could support it implicitly, but that also wouldn't let you control
when you would have your charm execute relation-created events. You'd also
need a way to get access to the relation-id if you ever want to test
relation-joined and relation-changed.
I could see a case for begin_with_initial_hooks to initialize peer
relations, but I think for a standard begin() setup it is clearer to
include the add_relation
John
=:->
…On Fri, Jun 4, 2021, 06:48 Balbir Thomas ***@***.***> wrote:
Introduction
When using Juju, an administrator never has to execute juju add-relation
for a peer relation. However when writing unittest code using the Operator
Framework test harness one has to invoke self.harness.add_relation() even
for a peer relation, even if it is defined in metadata.yaml. Failing to
do so generates a KeyError on ops.model.Model.get_relation() when used
with the peer relation name. This is a bit counter intuitive. If this is
indeed the desired behaviour then perhaps it should at least be documented
explicitly and perhaps the motivation for this approach also explained.
Please note that the docstring of ops.testing.Harness.add_relation()
reads "Declare that there is a new relation between *this app and
remote_app*" which is misleading as it does not indicate that this method
must also be invoked for peer relations.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#547>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABRQ7JIBPXG5H2QUCCJI6LTRCVOPANCNFSM46CR4MOA>
.
|
@jameinel the concerns you raise are indeed troublesome. May I explore them in the context of Juju's actual behaviour. At what point does Juju fire a In any case this feels like a "gotcha" that we need to bring out explicitly in our docs (both docstrings and sdk docs). |
relation-joined would only be triggered when another unit enters scope
(after its start hook completes). Or after your unit enters scope if there
is already another peer. eg:
unit/0
- peer-relation-created
- install
- leader-elected
- config-changed
- start
unit/1
- peer-relation-created
- install
- config-changed
- start
unit/0 peer-relation-joined (unit/1) && unit/1 peer-relation-joined (unit/0)
unit/0 peer-relation-changed (unit/1) && unit/1 peer-relation-changed
(unit/0)
…On Wed, Jun 9, 2021 at 5:39 AM Balbir Thomas ***@***.***> wrote:
@jameinel <https://github.com/jameinel> the concerns you raise are indeed
troublesome. May I explore them in the context of Juju's actual behaviour.
At what point does Juju fire a relation-joined event for a peer relation
? I do not see how it could be firing this event in response to a
add-relation by the system administrator, since that never happens. It
must be firing this either automatically when charm is "started" or when
first peer unit joins. In case of former begin_with_initial_hooks()
firing relation-joined for peer relations may make sense. In the case of
the latter (firing in response to first unit added) why not mimic the same
behaviour in harness ?
In any case this feels like a "gotcha" that we need to bring out
explicitly in our docs (both docstrings and sdk docs).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#547 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABRQ7K4K7YKIT7YA4LOBFLTR4ZDJANCNFSM46CR4MOA>
.
|
I'm not sure if I'm misunderstanding the previous comment, but it sounds like you're saying you only get a peer relation with multiple units. That doesn't seem to be what I'm seeing:
There's only one unit in this model and the peer relation is there already.
|
You absolutely have the peer relation with a single unit. But you don't get
'relation-joined' because that event is telling you about the peer. (unit/0
gets told about unit/1 and vice versa)
I *think* you get peer-relation-created regardless, but I'm not positive.
My point was more about what should be default triggered in the Harness.
If you have a peer relation then triggering peer-relation-created during
'begin_with_initial_hooks' makes sense.
We could pre define all peer relations, but then the Test code wouldn't
know the relation-id to manipulate it (to add other peers, to set peer
relation data, etc).
And if you have to add a function call to get the relation ID you might as
well define it at the same time. (I'm not stuck on that if we have strong
case for it, but I don't want too much magic in the general case.)
John
=:->
…On Thu, Jun 24, 2021, 06:08 mthaddon ***@***.***> wrote:
I'm not sure if I'm misunderstanding the previous comment, but it sounds
like you're saying you only get a peer relation with multiple units. That
doesn't seem to be what I'm seeing:
***@***.***:~$ juju deploy postgresql-k8s
Located charm "postgresql-k8s" in charm-hub, revision 3
Deploying "postgresql-k8s" from charm-hub charm "postgresql-k8s", revision 3 in channel stable
***@***.***:~$ juju status --relations
Model Controller Cloud/Region Version SLA Timestamp
pg-test microk8s-localhost microk8s/localhost 2.9.5 unsupported 12:05:52+02:00
App Version Status Scale Charm Store Channel Rev OS Address Message
postgresql-k8s waiting 0/1 postgresql-k8s charmhub stable 3 kubernetes installing agent
Unit Workload Agent Address Ports Message
postgresql-k8s/0 waiting allocating installing agent
Relation provider Requirer Interface Type Message
postgresql-k8s:peer postgresql-k8s:peer peer peer joining
There's only one unit in this model and the peer relation is there
already. juju debug-log also has the following entries:
application-postgresql-k8s: 12:06:27 INFO juju.worker.caasoperator.uniter.postgresql-k8s/0.relation joining relation "postgresql-k8s:peer"
application-postgresql-k8s: 12:06:27 INFO juju.worker.caasoperator.uniter.postgresql-k8s/0.relation joined relation "postgresql-k8s:peer"
application-postgresql-k8s: 12:06:28 INFO juju.worker.caasoperator.uniter.postgresql-k8s/0.operation ran "peer-relation-created" hook (via hook dispatching script: dispatch)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#547 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABRQ7OJFSMYGVCQNDNO5QDTUL7ZHANCNFSM46CR4MOA>
.
|
I am under the impression that the problem is that using the Harness does not define an implicit unit in the peer relation (the unit under test, so to say), which is actually the behavior I would expect in Juju. With this optic in mind, adding a new unit to the peer relation would indeed always trigger the
Wouldn't this work? https://ops.readthedocs.io/en/latest/#ops.model.Model.get_relation We could ofc add a helper method for peer relations in the test harness to ease up the discovery. |
I think @jameinel's points about controlling when the hooks run is the crux of this. Perhaps some docs improvements/clarifications should be made. And we can possibly consider auto-adding the peer relation within |
I'm looking at the harness code, and it does auto-add peer relations inside |
Yes I think documentation clarification may be enough. I presume using |
I've also made some discourse doc improvements (https://discourse.charmhub.io/t/how-to-test-your-charm/4461) further clarifying that manual peer relation adding is necessary. Fixes canonical#547
I've also made some discourse doc improvements (https://discourse.charmhub.io/t/how-to-test-your-charm/4461) further clarifying that manual peer relation adding is necessary. Fixes canonical#547
Introduction
When using Juju, an administrator never has to execute
juju add-relation
for a peer relation. However when writing unittest code using the Operator Framework test harness one has to invokeself.harness.add_relation()
even for a peer relation, even if it is defined inmetadata.yaml
. Failing to do so generates aKeyError
onops.model.Model.get_relation()
when used with the peer relation name. This is a bit counter intuitive. If this is indeed the desired behaviour then perhaps it should at least be documented explicitly and perhaps the motivation for this approach also explained. Please note that the docstring ofops.testing.Harness.add_relation()
reads "Declare that there is a new relation between this app and remote_app" which is misleading as it does not indicate that this method must also be invoked for peer relations.The text was updated successfully, but these errors were encountered: