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

docs: clarify where StoredState is stored, and the upgrade behaviour #1416

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions ops/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,4 @@ class SomeCharm(ops.CharmBase): ...
Podspec charms that haven't previously used local storage and that
are running on a new enough Juju default to controller-side storage,
and local storage otherwise.
.. jujuremoved:: 4.0
The ``use_juju_for_storage`` argument is not available from Juju 4.0
"""
38 changes: 29 additions & 9 deletions ops/framework.py
Original file line number Diff line number Diff line change
Expand Up @@ -1132,14 +1132,9 @@ def set_default(self, **kwargs: Any):
class StoredState:
"""A class used to store data the charm needs, persisted across invocations.

Example::

class MyClass(ops.Object):
_stored = ops.StoredState()

Instances of ``MyClass`` can transparently save state between invocations by
setting attributes on ``_stored``. Initial state should be set with
``set_default`` on the bound object, that is::
``set_default`` on the bound object; for example::

class MyClass(ops.Object):
_stored = ops.StoredState()
Expand All @@ -1152,6 +1147,16 @@ def __init__(self, parent, key):
def _on_seen(self, event):
self._stored.seen.add(event.uuid)

Data is stored alongside the charm (in the charm container for Kubernetes
sidecar charms, and on the machine for machine charms), except for podspec
charms and charms explicitly passing `True` for `use_juju_for_storage` when
running :meth:`ops.main` (in those cases, the data is stored in Juju).

For machine charms, charms are upgraded in-place on the machine, so the data
is preserved. For Kubernetes sidecar charms, when the charm is upgraded, the
pod is replaced, so any data is lost. When data should be preserved across
upgrades, Kubernetes sidecar charms should use a peer-relation for the data
instead of `StoredState`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Random ideas -- I know that use_juju_for_storage isn't actually going away with 4.0, but do we want to continue to steer people away from it?

We could mention podspec charms going away in future? e.g.

"except for podspec charms (deprecated in favour of sidecare charms), where the data is stored in Juju."

And maybe move the bit about use_juju_for_storage to after the good recommendation in the last paragraph for preserving data across upgrades? e.g. as a final sentence:

"To explicitly opt into Juju data storage, set use_juju_for_storage to True when running :meth:ops.main (but this is not recommended)."

Copy link
Contributor

Choose a reason for hiding this comment

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

podspec charms are deprecated, aren't they? Second guessing myself ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

podspec charms are deprecated, aren't they? Second guessing myself ..

Yes :). You have to find that info in the Juju docs, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've adjusted it to mention podspec and use_juju_for_storage being deprecated - I agree that's worth doing.

I don't think the use_juju_for_storage mention should be at the end, though - even with a "this is not recommended" I think it promotes it too much saying that it's a way to opt in. We don't want people doing this (and they'll get a warning if they do) - it's mentioned here so people that are doing it know that it is a consequence, but if they want the storage to be in Juju then they should use a peer relation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense!

"""

def __init__(self):
Expand Down Expand Up @@ -1245,7 +1250,12 @@ def _wrapped_repr(obj: '_StoredObject') -> str:


class StoredDict(typing.MutableMapping[Hashable, Any]):
"""A dict-like object that uses the StoredState as backend."""
"""A dict-like object that uses the StoredState as backend.

Charms are not expected to use this class directly. Adding a
:class:`StoredState` attribute to a charm class will automatically use this
class to store dictionary-like data.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does Ops know whether to create a StoredDict, StoredList, or StoredSet when you create/access a _stored attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you set the attribute it gets wrapped and similarly unwrapped when accessed.

It does an isinstance check so I guess the doc should say "dictionary data" rather than
dictionary-like" even though the StoredDict class is more generic.

"""

def __init__(self, stored_data: StoredStateData, under: Dict[Hashable, Any]):
self._stored_data = stored_data
Expand Down Expand Up @@ -1280,7 +1290,12 @@ def __eq__(self, other: Any):


class StoredList(typing.MutableSequence[Any]):
"""A list-like object that uses the StoredState as backend."""
"""A list-like object that uses the StoredState as backend.

Charms are not expected to use this class directly. Adding a
:class:`StoredState` attribute to a charm class will automatically use this
class to store list-like data.
"""

def __init__(self, stored_data: StoredStateData, under: List[Any]):
self._stored_data = stored_data
Expand Down Expand Up @@ -1354,7 +1369,12 @@ def __ge__(self, other: Any):


class StoredSet(typing.MutableSet[Any]):
"""A set-like object that uses the StoredState as backend."""
"""A set-like object that uses the StoredState as backend.

Charms are not expected to use this class directly. Adding a
:class:`StoredState` attribute to a charm class will automatically use this
class to store set-like data.
"""

def __init__(self, stored_data: StoredStateData, under: Set[Any]):
self._stored_data = stored_data
Expand Down
Loading