Enhancement request: access virtual filesystem in the test module

[weiiwang01]

Currently, the testing module of the operator framework includes a mock filesystem for the pebble-controlled container. However, the mock filesystem is initially empty, which differs from real-world scenarios where OCI images already contain numerous files and directories (e.g., /etc, /srv) that the charm relies on during execution.

Presently, we rely on the Harness.model.unit.get_container method to obtain the test pebble client before starting the test case and prime the mock filesystem with the necessary charm files. However, this approach presents a problem. The test pebble client obtained using this method is guarded by set_can_connect, requiring us to make the container connectable before adding files and directories to the mock filesystem. This could interfere with the actual test cases, since the connectable state is persisted until the harness is destroyed.

https://github.com/canonical/flask-k8s-operator/blob/979f3aaf0b8f120a874d1ad9f039983b7ffc37eb/tests/unit/conftest.py#L74-L77

    harness = Harness(FlaskCharm)
    flask_container: Container = harness.model.unit.get_container(FLASK_CONTAINER_NAME)
    harness.set_can_connect(FLASK_CONTAINER_NAME, True)
    flask_container.make_dir("/srv/flask", make_parents=True)

It would be great if the harness provided an API to access an unrestricted simulated container, not protected by set_can_connect. This container would be used exclusively by the test code for priming, inspecting the mock filesystem, and other similar operations. It will not be used by the charm code.

[benhoyt]

This is a good idea, I think we just need to come up with an appropriate API for it. What about a new Harness.connected_container(container: Union[str, model.Container]) method that returns a context manager intended for use with the with statement as follows:

harness = Harness(MyCharm)
with harness.connected_container(FLASK_CONTAINER_NAME) as container:
    container.make_dir("/srv/flask", make_parents=True)
    container.push(...)
    container.push_path(...)

The yielded container would be a regular Container object with can_connect set to True for the duration of the with statement, and then can_connect restored to its old value at the end of the with block.

I've added the 23.10 label to this one. It's not on our roadmap but if we can get agreement on the API it's small enough to sneak in.

Thoughts, @PietroPasotti? (Though of course you're going to say "this problem is already solved in ops-scenario". :-)

[weiiwang01]

Yes, this looks good. Thanks!

[benhoyt]

@weiiwang01 If you'd like to open a PR for this, go right ahead. Otherwise I'll try to get to it in the next few weeks.

[weiiwang01]

No problem, I will create a PR shortly.

[PietroPasotti]

Well of course if you used scenario this wouldn't be an issue, but I think the solution proposed by Ben above is good.
If we want that context manager to be even more useful, allow passing the value of can-connect as an argument, so we can use the context to temporarily set the value of can-connect to False as well.

with h.connected_container(foo, False):
    with assert_raises(SomeError):
        h.charm.do_something_you_can_only_do_if_container_can_connect()

with h.connected_container(foo, True):
    h.charm.do_something_you_can_only_do_if_container_can_connect()

WDYT @weiiwang01 @benhoyt

[weiiwang01]

Yes, that could totally work. However, in my opinion, this helper function seems to be designed for test setup, asserting, and similar tasks. When it comes to running charm code, I think the current Harness.set_can_connect is more suited.

[jameinel]

I'm more of a mind of exposing a Harness.get_filesystem(container), just from the statement that Harness is meant to be a way to poke at the private attributes of the model that will be exposed to the charm, that can do things that you shouldn't be able to do as a charm. (eg, you can create relations, which a charm cannot do, a charm actually gets a readonly error if you try to set Unit.name, etc)

[benhoyt]

Thanks @jameinel.

Just for the record, John also suggested, if we do end up going the with harness.connected_container(name) as container route, ensure the container returned in the as clause isn't the same as the runtime object, so if the charm tries to get the container within the with block it won't think it's connected -- i.e., distinguish between test setup object and runtime object.

[weiiwang01]

How about implementing a method called Harness.get_container_filesystem(container)? This method would return a ContainerFsFacade object, and only contains filesystem-related methods such as push, pull, list_files, push_path, etc and its access would always be permitted and unaffected by set_can_connect. The ContainerFsFacade would provide a familiar interface for setting up the container filesystem in tests while not being able to affect charm code.

Implementing this feature should be relatively straightforward with a proxy class.

[jameinel]

I'm in favor of something to that effect, as a nice way to set the "ecosystem" around the charm. John =:->

…

On Wed, May 17, 2023 at 11:44 PM Weii Wang ***@***.***> wrote: How about implementing a method called Harness.get_container_filesystem(container)? This method would return a ContainerFsFacade object, and only contains filesystem-related methods such as push, pull, list_files, push_path, etc and its access would always be permitted and unaffected by set_can_connect. The ContainerFsFacade would provide a familiar interface for setting up the container filesystem in tests while not being able to affect charm code. Implementing this feature should be relatively straightforward with a proxy class. — Reply to this email directly, view it on GitHub <#931 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABRQ7KUM7VSEG5KTECVZ4LXGWLI7ANCNFSM6AAAAAAYCCYUXI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[benhoyt]

Yeah, that sounds reasonable to me, too -- given that the interface will be the same as the existing FS methods, we're not (excatly) introducing swaths of brand new public API. @weiiwang01 If you're still game, go ahead and submit a PR for something like that, and then we'll refine it from there.

[weiiwang01]

Pull request created #933

[benhoyt]

Per discussion and per @weiiwang01's OP036 spec, we're going to change our testing approach here to use a temp directory on the real filesystem.

[benhoyt]

This is solved with the addition of Harness.get_filesystem_root in #960.