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

Service should be set to "uninitialized" after an update occurs #1641

Closed
reset opened this issue Jan 17, 2017 · 11 comments · Fixed by #1833
Closed

Service should be set to "uninitialized" after an update occurs #1641

reset opened this issue Jan 17, 2017 · 11 comments · Fixed by #1833
Assignees
Labels
Focus:Supervisor Related to the Habitat Supervisor (core/hab-sup) component Type:Additional Discussion Type: Bug Issues that describe broken functionality
Milestone

Comments

@reset
Copy link
Collaborator

reset commented Jan 17, 2017

An initialization hook only occurs when a package is started for the first time. After a service upgrade the initialization hook won't be run again and this is probably the behaviour that our users will expect. It's also the behaviour we describe in our documentation.

The simple fix for this is to set the service as uninitialized and not just as needing a restart upon update

@reset reset added Bug labels Jan 17, 2017
@adamhjk
Copy link
Contributor

adamhjk commented Jan 19, 2017

I would expect it not to run again, because it's already been initialized. If we need behavior that runs after upgrade, we should add another hook.

@reset
Copy link
Collaborator Author

reset commented Jan 19, 2017

@adamhjk this would mean the init hook is truly a run-once even if it's modified by a future version. If an app requires additional initialization after an upgrade we'd need to instead move it to an on-upgrade hook. If that's also behaviour that should happen at initialization time, I think people might be duplicating code in two hooks

@adamhjk
Copy link
Contributor

adamhjk commented Jan 19, 2017

I think there is code that needs to happen the first time a service is run, and never again (it's truly run-once). We probably don't do the right thing here now, because the init hook gets run the first time the service is started, which isn't quite the same.

Then there is code that needs to be run every time before a service is started/restarted, which is different.

A practical example - some systems need bootstrapping once, but never again. An example would be the generation of self signed certificates for authentication in PostgreSQL. While you could guard against it in a run always init hook, it feels like a different behavior to me.

@bodymindarts
Copy link
Contributor

bodymindarts commented Feb 21, 2017

I agree that init should be a 1 time only script (regardless of updates). That is what is documented and thus expected.
However a commit by @mwrock recently snuck in a change of behaviour regarding this.
https://github.com/habitat-sh/habitat/blame/master/components/sup/src/manager/service/mod.rs#L463-L464

As far as I can tell that is...

@mwrock
Copy link
Contributor

mwrock commented Feb 21, 2017

Currently many init hooks in core-plans and also our reference tutorial app either symlink or copy bits from pkg.path to the service directory. After an update, the pkg.path changes as well as, of course, its files. If we do not run the init hook on update then the supervisor will continue to operate on the files of the former package.

So removing init after update would break many plans.

Of course there are other init operations, like creating the system database tables in mysql that make sense to run only once ever. The hooks that I have seen which include such one time logic usually include some guard code to ensure this only happens once.

One might argue that after an update, you effectively have a new service that is in need of initialization. Regardless, as we consider when the init hook should be run we should be cognizant of the fact that any change to current behavior may break existing plans.

@mwrock
Copy link
Contributor

mwrock commented Feb 21, 2017

I should also note that without the above change, the init hook was not run once ever, it was just run once per hab start. While I agree there is a place for a hook that does run once and only once, that has never been the behavior with init.

@bodymindarts
Copy link
Contributor

bodymindarts commented Feb 21, 2017

I understand.

Currently many init hooks in core-plans and also our reference tutorial app either symlink or copy bits from pkg.path to the service directory. After an update, the pkg.path changes as well as, of course, its files. If we do not run the init hook on update then the supervisor will continue to operate on the files of the former package.

I would have expected those tasks to be handled in run or reconfigure.
Actually I would expect more consistency in handling of config updates and pkg updates in general.

Perhaps similar to reconfigure there needs to be a updated hook or something like that. Also the way the code is now the init hook will be called again but (afaict) the service will not be stopped and re-run after (re-)initialization.

@bodymindarts
Copy link
Contributor

I like the thought model around update == new service. Either way there needs to be conceptual clarity.

@bodymindarts
Copy link
Contributor

bodymindarts commented Feb 21, 2017

I see a few options conceptually:

  1. Change of Pkg version and Config changes / upgrades are treated identically in terms of hooks and update strategy
  2. One is treated as a special case of the other.... generally the same process applies but one or the other has additional handling
  3. They are conceptually different operations and are handled completely independently from each other (and each is well documented in its own right)

And finally:
4. Updates are not treated as part of the lifecycle of a service but rather as a new service beginning a new lifecycle.

@adamhjk
Copy link
Contributor

adamhjk commented Feb 21, 2017

I think 4 is basically right. We need a flowchart. :)

@reset
Copy link
Collaborator Author

reset commented Feb 22, 2017

@bodymindarts @adamhjk I'm in agreement with 4 as well

bodymindarts pushed a commit that referenced this issue Feb 28, 2017
When a package is updated the service should reset and begin a new
lifecycle. This commit stops the service after an update and resets the
initialized flag allowing the initialization process to take place via
the regular `execute_hooks()` code path on the next `service.tick()` call.

Closes #1641
bodymindarts pushed a commit that referenced this issue Feb 28, 2017
When a package is updated the service should reset and begin a new
lifecycle. This commit stops the service after an update and resets the
initialized flag allowing the initialization process to take place via
the regular `execute_hooks()` code path on the next `service.tick()` call.

Closes #1641

Signed-off-by: Justin Carter <justin@starkandwayne.com>
bodymindarts pushed a commit that referenced this issue Feb 28, 2017
When a package is updated the service should reset and begin a new
lifecycle. This commit stops the service after an update and resets the
initialized flag allowing the initialization process to take place via
the regular `execute_hooks()` code path on the next `service.tick()` call.

Closes #1641

Signed-off-by: Justin Carter <justin@starkandwayne.com>
bodymindarts pushed a commit that referenced this issue Feb 28, 2017
When a package is updated the service should reset and begin a new
lifecycle. This commit stops the service after an update and resets the
initialized flag allowing the initialization process to take place via
the regular `execute_hooks()` code path on the next `service.tick()` call.

Closes #1641

Signed-off-by: Justin Carter <justin@starkandwayne.com>
bodymindarts pushed a commit that referenced this issue Feb 28, 2017
When a package is updated the service should reset and begin a new
lifecycle. This commit stops the service after an update and resets the
initialized flag allowing the initialization process to take place via
the regular `execute_hooks()` code path on the next `service.tick()` call.

Closes #1641

Signed-off-by: Justin Carter <justin@starkandwayne.com>
@eeyun eeyun added this to the Help Wanted milestone Feb 28, 2017
@christophermaier christophermaier added Focus:Supervisor Related to the Habitat Supervisor (core/hab-sup) component Type: Bug Issues that describe broken functionality and removed A-supervisor labels Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Focus:Supervisor Related to the Habitat Supervisor (core/hab-sup) component Type:Additional Discussion Type: Bug Issues that describe broken functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants