search

canonical / k8s-operator

Machine charm for K8s following the operator framework
Apache License 2.0
4 stars 3 forks source link

snap.py's `management()` does not install snap if `arg.revision == snap_latest_revision` #128

Open ca-scribner opened 5 months ago

ca-scribner commented 5 months ago

When revision is defined in the snap_installation.yaml, management() installs the snap and holds the revision here:

https://github.com/canonical/k8s-operator/blob/5290074ca8b8d041f86656af6d10c40ee2282f99/charms/worker/k8s/src/snap.py#L116C1-L120C29

        elif isinstance(args, SnapStoreArgument) and args.revision:
            if which.revision != args.revision:
                log.info("Ensuring %s snap revision=%s", args.name, args.revision)
                which.ensure(**args.dict(exclude_none=True))
                which.hold()

where which.revision is the revision of the snap as pulled from the SnapCache() (likely the revision of latest/stable). Because the snap is only installed above if which.revision != args.revision, requesting the revision of the current latest/stable snap results in no snap being installed and the .hold() not being applied

@addyess mentioned a bug in snapd that if you refresh to the snap you're currently on, you still restart the services, so this might be an attempt at defending against that problem? afaict though, the newest version of the snap library has some protections against this. So probably we can do:

elif isinstance(args, SnapStoreArgument) and args.revision:
#             if which.revision != args.revision:  <--- removed
            log.info("Ensuring %s snap revision=%s", args.name, args.revision)
            which.ensure(**args.dict(exclude_none=True))
            which.hold()
addyess commented 5 months ago

@ca-scribner yes -- the library has that protection -- but i noticed that which.hold() will also cause the snap library to run refresh with the --hold args and i don't know if this TOO will restart the snap services.

ca-scribner commented 5 months ago

yep good call, and I dont see an easy way of working around that with the lib.

You could probably avoid some unnecessary refreshes by checking if hold is on and if revision is correct. But if revision is correct and hold is off, this looks like it'll refresh the snap in order to set hold

ca-scribner commented 5 months ago

(Whoops, meant to post this on Friday. Sorry!)

Ah, here's a win.

afaict snap refresh --hold=forever snapname does not restart the service, it just changes the hold status. I'm confirming this by watching snap log snapname then doing snap refresh --hold=forever. I dont see any log activity, so I think we're good to do a refresh --hold=forever any time