many: remove configure-snapd task again and handle internally #4298

mvo5 · Nov 24, 2017

Using a separate configure-snapd task via the configstate manager
proves to be problematic (see https://forum.snapcraft.io/t/2774).

This PR changes things so that the "configure" hook for "core"
gets intercepted and instead of running an external hook we
run our internal corecfg code.

Some unit tests are still missing, but I'm keen to get results from spread.

We may also remove the taskrunner from the ConfigureManager now as
it serves no purpose anymore.

pedronis · Nov 24, 2017

pedronis reviewed Nov 24, 2017

View changes

we probably want a TODO here about having a general RegisterHijack mechanism or something later

I pused a version that uses the hijacker now.

codecov-io · Nov 24, 2017

Codecov Report

Merging #4298 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4298      +/-   ##
==========================================
+ Coverage   76.16%   76.21%   +0.04%     
==========================================
  Files         444      444              
  Lines       38687    38635      -52     
==========================================
- Hits        29466    29444      -22     
+ Misses       7202     7181      -21     
+ Partials     2019     2010       -9

Impacted Files	Coverage Δ
overlord/configstate/configmgr.go	`93.93% <100%> (+20.6%)`	⬆️
overlord/configstate/configstate.go	`87.5% <100%> (+0.46%)`	⬆️
overlord/hookstate/hookmgr.go	`67.87% <100%> (+1.84%)`	⬆️
overlord/snapstate/snapmgr.go	`87.18% <0%> (ø)`	⬆️
overlord/snapstate/handlers.go	`65.83% <0%> (+0.08%)`	⬆️
timeutil/schedule.go	`94.87% <0%> (+2.37%)`	⬆️
overlord/configstate/hooks.go	`71.66% <0%> (+9.99%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a94330b...f6e9593. Read the comment docs.

pedronis · Nov 24, 2017

pedronis requested changes Nov 24, 2017

View changes

two comments, the 2nd needs addressing, the first only partially, we can revisit the RegisterHijack interface after 2.30, anyway it's purely internal, not reflected in the state

I have mixed feeling about this interface:

the asymmetry vs the Register interface is a bit strange, I would at least switch the order of hookName, snapName
otoh a symmetrical interface would be much more complicated

:/

I switched the order now, but yeah, having a RegsterHijacked with the same signature as Register() is probably tricky.

we should find out if we have an "hijack" much earlier, so that this check:

    hookExists := info.Hooks[hooksup.Hook] != nil
    if !hookExists && !hooksup.Optional {
            return fmt.Errorf("snap %q has no %q hook", hooksup.Snap, hooksup.Hook)
    }

is not relevant in the case we have one. Otherwise we can hijack only optional hooks if their missing from the snap (which usually they will).

pedronis · Nov 24, 2017

pedronis reviewed Nov 24, 2017

View changes

we should panic I think if there's already an entry

pedronis · Nov 27, 2017

pedronis approved these changes Nov 27, 2017

View changes

thank you, small issue though

missing fmt.Sprintf ?

niemeyer · Nov 27, 2017

niemeyer requested changes Nov 27, 2017

View changes

LGTM besides the error handling which is probably just an oversight.

The key for this map might also be a struct:

type hijackKey struct { hook, snap string }

This way we don't need to sprintf every time:

return m.hijackMap[hijackKey{hookName, snapName}]

Given there is likely a change necessary anyway to address the error checking, I'll nitpick just slightly. :)

"hijacked" feels like something that already took place, but really the whole workflow is exactly the same until we get right here in this line above. Considering that, it feels like it would be just slightly more clear if we used the "hijack" term in the present, as a request of something we'd like to happen, and use "hijacked", in the past, only once and if we're talking about something that did happen before. So RegisterHijack, and perhaps mustHijack instead of isHijacked, etc.

Again, this is getting deeply into nitpick territory, so feel free to ignore if you don't feel like it's worth it.

I think when I suggested RegisterHijack it was indeed the name or present, seems I didn't notice the change to Hijacked when I re-reviewed. Anyway also +1 on switching from me.

Indeed, thank you. This makes it clearer/cleaner!

Shouldn't this have the opportunity to return an error, which is logged as usual into the handler?

Yes, great catch, thank you!

pedronis · Nov 28, 2017

pedronis reviewed Nov 28, 2017

View changes

couple small comments

should this be hijackMap now?

I was thinking at this point its hijacked already, but it seems hijackMap is cleaner and more consistent with hijack{Key,Func} so 👍 - I fix it :)

does the err bit merit a test?

YES, thank you.

pedronis · Nov 28, 2017

there's a real problem with locks in this PR

hookmanager: add new HookManager.RegisterHijacked() and use it

f166d54

configstate: use the right way to get the context transaction

ed35057

address review feedback (thanks to Samuele)

4fb6991

hookmgr: panic correctly

1f44188

mvo5 added this to the 2.30 milestone Nov 27, 2017

address review feedback (thanks to Gustavo)

555f84b

hookstate: add missing hijack related tests

5513321

pedronis referenced this pull request Nov 28, 2017
Merged
many: allow to configure core before it is installed #4310

mvo5 and others added some commits Nov 28, 2017

hookstate: move context locking into the hijack function

387534f

add a test for the core configure hijack in configstate itself

f6e9593

pedronis merged commit ed40634 into snapcore:master Nov 28, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

-        -				return fmt.Errorf("run hook %q: %v", hooksup.Hook, err)
+        +	// the core snap is special and is handled differently
+        +	var output []byte
+        +	if hooksup.Snap == "core" && hooksup.Hook == "configure" {

-        -				return fmt.Errorf("run hook %q: %v", hooksup.Hook, err)
+        +	// some hooks get hijacked, e.g. the core configuration
+        +	var output []byte
+        +	if f := m.hijacked(hooksup.Snap, hooksup.Hook); f != nil {

+        +func (m *HookManager) RegisterHijacked(hookName, snapName string, f HookFunc) {
+        +	key := fmt.Sprintf("%s:%s", hookName, snapName)
+        +	if _, ok := m.hijackedMap[key]; ok {
+        +		panic("hook %s already hijacked")

@@ -142,6 +147,18 @@ func (m *HookManager) Stop() {
          	m.runner.Stop()
          }
+        +func (m *HookManager) hijacked(hookName, snapName string) HookFunc {
+        +	return m.hijackedMap[fmt.Sprintf("%s:%s", hookName, snapName)]

-        -				return fmt.Errorf("run hook %q: %v", hooksup.Hook, err)
+        +	// some hooks get hijacked, e.g. the core configuration
+        +	var output []byte
+        +	if f := m.hijacked(hooksup.Hook, hooksup.Snap); f != nil {

snapcore/snapd

many: remove configure-snapd task again and handle internally #4298

mvo5 commented Nov 24, 2017 •

Edited 1 time

mvo5
Nov 24, 2017

pedronis reviewed Nov 24, 2017

View changes

pedronis requested changes Nov 24, 2017

View changes

pedronis reviewed Nov 24, 2017

View changes

pedronis approved these changes Nov 27, 2017

View changes

mvo5 added this to the 2.30 milestone Nov 27, 2017

niemeyer requested changes Nov 27, 2017

View changes

pedronis reviewed Nov 28, 2017

View changes

pedronis referenced this pull request Nov 28, 2017

many: allow to configure core before it is installed #4310

pedronis commented Nov 28, 2017

mvo5 and others added some commits Nov 28, 2017

pedronis merged commit `ed40634` into snapcore:master Nov 28, 2017
1 check passed

1 check passed

snapcore/snapd

Join GitHub today

many: remove configure-snapd task again and handle internally #4298

Conversation

mvo5 commented Nov 24, 2017 • edited Edited 1 time mvo5 Nov 24, 2017

pedronis reviewed Nov 24, 2017 View changes

Codecov Report

pedronis requested changes Nov 24, 2017 View changes

pedronis reviewed Nov 24, 2017 View changes

pedronis Nov 24, 2017 • edited Edited 1 time pedronis Nov 24, 2017

pedronis approved these changes Nov 27, 2017 View changes

mvo5 added this to the 2.30 milestone Nov 27, 2017

niemeyer requested changes Nov 27, 2017 View changes

pedronis reviewed Nov 28, 2017 View changes

pedronis referenced this pull request Nov 28, 2017

pedronis commented Nov 28, 2017

mvo5 and others added some commits Nov 28, 2017

Hide details View details pedronis merged commit ed40634 into snapcore:master Nov 28, 2017 1 check passed

1 check passed

mvo5 commented Nov 24, 2017 •

Edited 1 time

mvo5
Nov 24, 2017

pedronis reviewed Nov 24, 2017

View changes

pedronis requested changes Nov 24, 2017

View changes

pedronis reviewed Nov 24, 2017

View changes

pedronis Nov 24, 2017 •

Edited 1 time

pedronis
Nov 24, 2017

pedronis approved these changes Nov 27, 2017

View changes

niemeyer requested changes Nov 27, 2017

View changes

pedronis reviewed Nov 28, 2017

View changes

pedronis merged commit `ed40634` into snapcore:master Nov 28, 2017
1 check passed