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

Calling Snap.ensure(....) with the same revision shouldn't refresh the snap if the revision is already installed #120

Open
addyess opened this issue May 6, 2024 · 4 comments

Comments

@addyess
Copy link
Contributor

addyess commented May 6, 2024

I didn't expect the ensure(...) method to actually cause a snap-refresh if the revision requested already matched the installed revision. If one calls ensure(...) with a channel, and the revision is unchanged, no refresh is triggered.

So, in my charm where i'm trying to set a specific revision on the snap, I had to add this if clause prevent a constant snap refresh to a revision where it already was.

    cache = snap_lib.SnapCache()
    which = cache[args.name]
    if which.revision != args.revision:
        log.info("Ensuring %s snap revision=%s", args.name, args.revision)
        which.ensure(**args.dict(exclude_none=True))
@benhoyt
Copy link
Collaborator

benhoyt commented May 9, 2024

@addyess Can you include a repro case? Is this just an optimisation, to avoid starting the snap refresh command if the Python code already determines it's already at the requested revision? Because presumably snap refresh --revision= itself won't do anything if the snap is already at that revision?

@addyess
Copy link
Contributor Author

addyess commented May 9, 2024

@benhoyt I don't think it'd be hard to go back to something and reproduce in my charm code. I found a event that was firing regularly into a path which called:

    cache = snap_lib.SnapCache()
    which = cache[MY_SNAP]
    log.info("Ensuring %s snap revision=%s", MY_SNAP, MY_REV)
    which.ensure(state=SnapState.Present, name=MY_SNAP, revision=MY_REV)

Everytime the refresh occurred, snapd would restart the services provided by that even though it was already on that revision.

i think i didn't anticipate that ensure would actually trigger a restart of all the snap services even when the revision had not changed.

@benhoyt
Copy link
Collaborator

benhoyt commented May 10, 2024

So I just tested this with the cups snap on my machine (just for lack of something better to try), and there are two things I found:

  1. When you just run snap refresh cups with additional arguments, it says snap "cups" has no available updates and doesn't do anything further (that's what I expected).
  2. When you run snap refresh cups --revision=1047, that is, with a --revision specified, even if it's the current installed revision, it will do a full restart of the services. That is not what I expected snap refresh to do -- I would have thought that if the revision is the currently-installed revision, it would do nothing, but for whatever reason it doesn't have those smarts in snap itself.

Here your code is specifying the revision, so --revision is passed, and you get the second behaviour. :-(

We could ask the snap team why it does that, but probably our best bet is to update this library to check "if all of the details are the same, do nothing". Would you be willing to put up a PR and test this? It'll be something like so (completely untested):

if (not channel or channel != self._channel or
    not revision or revision != self._revision or
    # similar constructs for cohort and devmode ...
        ):
    self._refresh(...)
else:
    logger.info("Snap already up to date")

@benhoyt
Copy link
Collaborator

benhoyt commented May 24, 2024

Fixed by #121. Thanks @addyess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants