search

canonical / operator

Pure Python framework for writing Juju charms
Apache License 2.0
247 stars 119 forks source link

Test Harness should support peer relations implicitly if defined #547

Closed balbirthomas closed 2 years ago

balbirthomas commented 3 years ago

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.

jameinel commented 3 years ago

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 https://github.com/canonical/operator/issues/547, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABRQ7JIBPXG5H2QUCCJI6LTRCVOPANCNFSM46CR4MOA .

balbirthomas commented 3 years ago

@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).

jameinel commented 3 years ago

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

unit/1

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 https://github.com/canonical/operator/issues/547#issuecomment-857546965, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABRQ7K4K7YKIT7YA4LOBFLTR4ZDJANCNFSM46CR4MOA .

mthaddon commented 3 years ago

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:

mthaddon@tenaya:~$ 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
mthaddon@tenaya:~$ 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)
jameinel commented 3 years ago

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 https://github.com/canonical/operator/issues/547#issuecomment-867513284, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABRQ7OJFSMYGVCQNDNO5QDTUL7ZHANCNFSM46CR4MOA .

mmanciop commented 3 years ago

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 relation_joined via the test harness. We might have to be careful about (1) the unit id we give to the unit under test (it always being 0 might hide bugs) and to sanity-check the unit id of newly-added units, if we intend the add_relation_unit method to also to be used for peer relations.

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).

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.

rwcarlsen commented 2 years ago

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 begin_with_initial_hooks.

rwcarlsen commented 2 years ago

I'm looking at the harness code, and it does auto-add peer relations inside begin_with_initial_hooks if peers are present in the yaml file. You get the relation-created events emited, but no relation-joined/changed events unless you add relation units just like I would expect. As far as I understand, juju doesn't fire joined/changed events either unless more than one (peer) unit is explicitly deployed - so the harness is currently operating like juju here (w.r.t. begin_with_initial_hooks at least). Feel free to interject here if I've misunderstood anything, but that basically leaves only documentation clarifications to resolve the remainder of this issue.

balbirthomas commented 2 years ago

Yes I think documentation clarification may be enough. I presume using begin() rather than begin_with_initial_hooks is not forbidden if necessary when unit testing peer relations. If so then also the documentation of ops.testing.Harness.add_relation() may need to extend to say it allows creation of peer relations too (not just "remote app"), which it actually does (so no code changes here either).