search

canonical / operator

Pure Python framework for writing Juju charms
Apache License 2.0
246 stars 119 forks source link

Symlink for `hooks/upgrade-charm` is not created when missing #104

Closed evhan closed 4 years ago

evhan commented 4 years ago

Hi there, this is a bit of an edge case but I've just been troubleshooting a problem caused by this sequence of events:

  1. Create a charm that includes a hooks/upgrade-charm script
  2. Deploy the charm (juju deploy foo ...)
  3. Remove hooks/upgrade-charm from the charm's sources
  4. Deploy the new version of the charm (juju upgrade-charm --path foo ...)

As a result, the hooks/upgrade-charm symlink is missing and the upgrade_charm event is no longer passed through to the user's charm.py. This is reported in the logs as juju.worker.uniter.operation skipped "upgrade-charm" hook (missing). This may happen for other event types as well, but I haven't checked yet and I know start, install, and upgrade-charm are kind of special so I'd guess this problem is particular to upgrade-charm.

I think it would be best if all missing symlinks were created any time the code in /var/lib/juju/agents/*/charm changes (I guess whenever juju.downloader does its thing?) so that the resulting behaviour always matches what the user has defined in the deployed version of the charm, regardless of what past versions might have looked like. Thoughts?

jameinel commented 4 years ago

I think we certainly could create upgrade-charm. I believe the main caveat as to why we don't, is that for anything that was a legacy charm, it needs to have upgrade-charm already defined, because the way the new codebase gets distributed is via upgrade-charm. (eg, v0 of the charm is using just shell scripts, v1 of the charm is now using the new framework. 'install' won't get called again, 'upgrade-charm' will be). So maybe the better thing is to update README to indicate the hooks that charms should define (install, start, upgrade-charm). That said, Juju 2.8 will actually have a new hook called "process-hook" which will be called for any hook which is not otherwise defined (currently being implemented). Which means that you don't have to define individual hooks anymore.

Probably a good compromise is to document the hooks like 'upgrade-charm' for existing compatibility, and then we can improve the documentation once Juju itself has been updated.

johnsca commented 4 years ago

For framework-first charms, the upgrade-charm hook symlink will be created along with all other hook symlinks during the first hook that runs which has been manually linked (essentially, install). During upgrade, new or changed hook symlinks will be created or updated as needing during upgrade-charm as long as the existing symlink for upgrade-charm still works.

In your case, you explicitly deleted the upgrade-charm hook link and then ran an upgrade, so it seems reasonable that it didn't work. I'm not entirely against all hooks trying to ensure the validity of the links, but it also seems somewhat unnecessary as there are only specific points at which they can reasonably be expected to change (first run, and upgrade).

As John mentioned, the transition from a non-framework charm to a framework charm is by definition going to require a bit of special handling, so it will be necessary to manually ensure the upgrade-charm link is in place just like it's necessary to manually ensure the first run hook link (install) is in place for new charms.

evhan commented 4 years ago

Right, this is certainly an odd case and I appreciate that there's added weirdness around the transition. I suppose the main thing I would like to communicate with this issue is the desire that the behaviour of a charm depend solely on the charm's code as deployed (whether first time or via upgrade), i.e. it should be insensitive to anything that might have happened previously due to files being present or missing.

As far as the best way to achieve that, I can't say, I just wanted to share a snag that we had hit and say that our assumption (well-founded or not) was that the upgrade-charm hook would be restored by the agent harness. Thanks both for having a look!

jameinel commented 4 years ago

I think we'll close this for now. With a) upgrade-charm would be created as long as it isn't 'upgrade-charm' that is triggering the next event b) Ultimately we are going to switch to a catch-all hook once Juju implements it for 2.8, at which point this also becomes moot because we won't need to create a symlink farm anymore.

evhan commented 4 years ago

Yep, that makes sense, I think point b totally sorts it. Thanks John.