canonical / operator

Pure Python framework for writing Juju charms
Apache License 2.0
240 stars 113 forks source link

[RFE] Feature request to retrieve charm revision #1255

Open MiaAltieri opened 3 weeks ago

MiaAltieri commented 3 weeks ago

Hello Ops Team 👋 A few of us on Data Platform team would be very grateful for a feature to retrieve the revision of a charm, maybe something like self.cham.revision which would retrieve what is already in the file sudo cat /var/lib/juju/agents/unit-<charm-name>-<unit-id>/charm/.juju-charm in charm containers. i.e. a string that represents:

  1. revision number
  2. and whether the charm was deployed from charmhub / locally (In .juju-charm (it includes ch: or local: depending on where it was deployed)

As I mentioned a few of us would really appreciate this feature so we have a variety of use cases. But I will only outline three cases (if you would like to hear more, let me know and I can add them):

Use Case 1: UX improved, specially for juju status during an upgrade which reports which revision we are upgrading to

Use Case 2: Blocking incompatible upgrades

Use Case 3: Blocking incompatible revisions in Multi-application deployments (i.e. a sharded MongoDB cluster which is made up of three separate juju applications of the same charm "Charmed MongoDB")

tonyandrewmeyer commented 3 weeks ago

This seems reasonable to me, although it feels like some of it would better belong in Juju (maybe as small Starlark code?). It seems unlikely that would happen any time in the next year+, though.

We would need to verify with the Juju team that the .juju-charm file being in that location and in the current format is something they guarantee or whether it's considered an implementation detail and they'd want to expose the data in another way (e.g. a hook tool).

(Note that the sudo shouldn't be needed, I think, because the file - at least in my testing - has a+r permissions and the charm must have access to that directory already).

It seems like this would probably belong as an ops.Unit property, and probably as a parsed object rather than just charm_url.

@dataclasses.dataclass(frozen=True)
class CharmURL:  # Needs a better name
    source: str  # or maybe an enum of "local", "ch", etc?
    architecture: str|None  # or maybe an enum?
    base: str|None  # I don't think base is the right name, but I couldn't quickly find a schema for the charm URL in juju/juju.
    name: str  # I _think_ this is the upstream name, versus the possible alias the charm is installed under?
    revision: int
    version: str|None  # charm-version, ie. from the `version` file

I will only outline three cases (if you would like to hear more, let me know and I can add them)

The three you've provided are compelling, but it's always great to get the full picture (for example - it helps us socialise and document the feature once (if!) implemented), so if you could add them, I think that would be valuable. For what it's worth, I would probably bundle 2 & 3 together as "blocking incompatible upgrades" and note that there are multiple cases, so if it's more examples of that, then I don't think it's needed.

benhoyt commented 3 weeks ago

Agreed with @tonyandrewmeyer that this seems like a reasonable request, though I definitely don't think Ops should rely on the presence or format of the .juju-charm file -- that seems like an implementation detail. I mean, you could use that in your charm as a workaround till we have a feature like this, but we wouldn't bake it into Ops.

Regarding the dataclass proposal, I agree that's a good idea if we do this. There is charm.URL in the charm package used by Juju, though I'm not sure that's the current thing, as it still has "series" instead of "base".

In any case, we plan to talk to the Juju team about this next week to see what's feasible and where they think we (or the charm) should get the information from.

MiaAltieri commented 3 weeks ago

Hi @tonyandrewmeyer and @benhoyt

Thank you very much for your response here :pray: :)

The solution you proposed of a class named CharmURL would fit our use cases perfectly.

As for the other use cases let me collect those from my other teammates and get them added here.

From my own knowledge of the MongoDB charm, Use Case 3 can be expanded outside of upgrades. i.e. if we have a sharded cluster of three separate applications of MongoDB:

shard 0 - charm revision 88 - workload 6.0.6
shard 1 - charm revision 100 - workload 6.0.6
config-svr - charm revision 100 - workload 6.0.6

but their relation changed hooks are incompatible. So when we do juju integrate shard-0 config-svr the relation-joined event fails since config-svr rev 100 supports TLS but the shard-0 rev 88 application does not.

This use case is true for other features, i.e. COS, password rotation strategies, backups, etc. If you'd like more details on these please let me know and I am happy to provide them.

Thanks again! Mia

taurus-forever commented 3 weeks ago

Hi Team,

We will have to read .juju-charm in all our charms (Mia provided reasons). We will have to re-share the code through data-platform-libs (to minimize duplicates). At some point we will have another 'Juju Secrets' story to move it to Ops (where it naively/natively should be). This feature request has been created to shortcut implementation in the first place. :-D

Do we really need one year to add a Python method to read and return the single file content on a static path? BTW, the file is there for 9 years already. When/if Juju remove it => deprecate the method in Ops.

P.S. thank you for the great stability in the latest Ops releases and a lot of new features there! They inspired us to request more! ;-)

carlcsaposs-canonical commented 3 weeks ago

Another use case is to detect rollbacks during an in-place upgrade

If the user rolls back to the same exact revision as before the upgrade, we handle that differently than if the user were to upgrade to a different revision while an upgrade is in-progress. For example, we skip pre-upgrade checks during rollback because the user might be rolling back because of an issue with the new version & probably wants to restore service as quickly as possible on the upgraded units

To achieve this currently, we have to create a unique charm_version file within each charm—but, as Mia mentioned, the UX for the user here is confusing (we have to show charm_version instead of revision, because we don't know revision, and then the user has to go to our github releases, look up the commit hash, and then pick the correct revision for their architecture & ubuntu base—all while they might be panicking because service is down and they're trying to restore it as quickly as possible)

phvalguima commented 3 weeks ago

Fyi, this file exists for more than 10 years. So, it is safe to consider it will continue there.

@tonyandrewmeyer @benhoyt we need this feature and it makes a lot of sense it to be part of CharmBase, e.g. a revision property. Honestly, only the way to retrieve this information may change here; and reading this file would be a very simple thing. The reminder of the logic, e.g. the dataclass above, will remain the same.

benhoyt commented 2 weeks ago

Thanks for all the input, folks. We have discussed this as Charm Tech, and we talked to the Juju team today. A few thoughts on how to move forward with this:

All that said, we can clearly see that charms would find this information useful now. There's nothing stopping a charm from reading this file in the charm, though of course when Juju removes it, you'll have to update the charm with a workaround for new Juju versions.

For now, what we suggest is that you create a very small charm-revision charm library, and use that in your charms that need it. Then if the .juju-charm file is ever changed or removed, you can update the charm lib in one place. We'd be happy to review the Python design and code for this charm library.

I know it's not quite what you wanted to hear, but I hope that helps provide clarity and direction. I'll leave this issue open for a week or two for further discussion.

benhoyt commented 2 weeks ago

We discussed a little further with @wallyworld today, and a couple of further suggestions from that:

Also, relations numbers aren't simple and not even necessarily increasing (what with tracks and channels and bases) -- @wallyworld can add more on that.

wallyworld commented 2 weeks ago

Ideally we would want to look at modelling epochs for charms. In the meantime, revision numbers are not very useful as a means of recording the concept of a logical charm version. Different charm blobs will be expected to exist for the matrix of base, arch within a given track, and each blob will have its own revision. What's missing is a way of recording a logical grouping of charms across these axes to represent a semantic release. There's been discussions on what that might mean, but nothing concrete has been planned as yet.

carlcsaposs-canonical commented 2 weeks ago

fwiw, our use cases (at least 1 and 2 and detecting rollbacks) were for comparing exact revision number, not for greater than/less than comparisons

MiaAltieri commented 2 weeks ago

Hi @wallyworld + @benhoyt - thank you for your response. On the NoSQL subteam we have a small bit of code that does exactly what you describe:

For now, what we suggest is that you create a very small charm-revision charm library, and use that in your charms that need it.

As for a long term solution. Is the idea right now to wait until:

Longer term, the Juju team wants to solve the revision / blocking upgrades issue with something similar to Snap Epochs. But obviously that's a bigger design and implementation task for the Juju team. And they're in the midst of the big Mongo to Dqlite conversion now (hence Tony mentioning "one year").

benhoyt commented 2 weeks ago

As for a long term solution. Is the idea right now to wait until [epochs is ready]?

Yep, aside from doing a charm lib as a workaround -- that's right. Though it can be "active waiting", that is, giving the Juju team feedback about what's important for their next planning cycle. I don't know, but it may be that designing and implementing epochs in Juju is not so big a feature, seeing it's already designed for snapd (but I don't really know).

BTW @MiaAltieri Did you intend to close and reopen?