feat(cli): add `--dry` option to `pebble run` #214

anpep · 2023-04-18T12:48:50Z

This PR adds dry start functionality to managers through the DryStart() method on the StateManager interface (KO026) and adds the --dry option to the pebble run command.

The --dry option is useful for validating the Pebble layers without having to actually start the Pebble daemon.
Note that, in any case, pebble will exit. If the plan was valid, pebble will exit with an error code of 0; otherwise, with a non-zero exit code.

The previous iteration of this was done in PR #175.

benhoyt

Looks good overall (though I don't think I was part of the discussions about the new way to approach this -- but this seems reasonable). A few minor comments.

internal/overlord/backend.go

internal/overlord/stateengine.go

anpep · 2023-04-24T11:39:28Z

Looks good overall (though I don't think I was part of the discussions about the new way to approach this -- but this seems reasonable). A few minor comments.

Thanks for your review Ben! Please refer to KO026 where I explain the new approach. I will be fixing this stuff starting tomorrow, and will let you know when it's ready for another review. Thanks again!

internal/overlord/stateengine.go

hpidcock · 2023-05-21T22:23:05Z

@anpep was this ready for re-review? It's marked as Priority, but doesn't appear to be ready for review?

anpep · 2023-05-22T08:34:43Z

@anpep was this ready for re-review? It's marked as Priority, but doesn't appear to be ready for review?

Sorry for the confusion, yes, this PR is ready for review.

cmd/pebble/cmd_run.go

internal/daemon/daemon.go

internal/overlord/servstate/manager.go

internals/overlord/overlord_test.go

internals/overlord/stateengine.go

benhoyt · 2023-06-09T02:18:50Z

internals/overlord/overlord.go

@@ -133,6 +141,11 @@ func New(pebbleDir string, restartHandler restart.Handler, serviceOutput io.Writ
 	// the shared task runner should be added last!
 	o.stateEng.AddManager(o.runner)

+	// Dry start all managers.
+	if err := o.stateEng.DryStart(); err != nil {


Should this only happen if dry is true?

Not really; the dry start mechanism, as discussed on KO026, always runs in order to run initialization tasks that do not incur in unwanted side-effects.

Indeed, but it's a bit awkward to see this here at the end of Init.

I think this shoud be run when starting, or on a specific DryStart method that only does the dry-starting. This way we also don't need to provide further arguments into the constructor here, and no bools either.

The issue of running this when starting (e.g. at the top of overlord.Loop()) is that Loop() might be called too late and other components (e.g. the daemon) might have already done some state-mutating operations.

The issue of requiring callers to call an overlord.DryStart() method manually is that it's easy to forget, and thus misuse the API. Also, DryStart() is not an operation you do on the overlord, but rather on the each of the managers. Holding the caller responsible for initializing the managers doesn't seem right to me: the overlord should be the one doing this very internal operation on the managers.

I think running this at the bottom of overlord.New() is the sweet spot for these reasons. Also, we don't need any kind of arguments (that argument from previous commits was clearly a bug and a misunderstanding of the problem on my side), because DryStart() is meant to be called always.

* Remove dryStartCallback from sampleManager. * Unexport ErrStateEngineStopped->errStateEngineStopped. * Simplify multiError string when there's only one error. * Fix mistakenly swapped logging of dry start/ensure errors.

internals/daemon/daemon.go

internals/overlord/backend.go

internals/overlord/servstate/manager.go

internals/overlord/stateengine.go

internals/overlord/backend.go

* Add StateEngine.dryStarted check on Ensure() in order to make DryStart() call actually required. * Call DryStart() on overlord and state engine tests * Remove noopStateBackend: not required since --dry run is cut after the DryStart() call, so no state is ever persisted. Furthermore, this allows for potential dry-run of state patches. * Remove Overlord.addManager(), since StateEngine.AddManager() already exists for this purpose, and Overlord.AddManager() as well. * Don't plan.ReadDir() on ServiceManager.DryStart(), acquire plan instead so that we actually initialize the manager instead.

paul-rodriguez · 2023-06-13T08:21:57Z

internals/overlord/managers_test.go

@@ -42,7 +42,7 @@ func (s *mgrsSuite) SetUpTest(c *C) {

 	s.dir = c.MkDir()

-	o, err := overlord.New(s.dir, nil, nil)
+	o, err := overlord.New(s.dir, nil, nil, false)


This is the reason why I'm not a fan of boolean args: the call site is not readable unless you know the function prototype by heart.

I'm even less of a fan of providing nil as a valid value to anything, but that's idiomatic Go.

This function signature is just asking for a refactor of its arguments to a struct:

Suggested change

o, err := overlord.New(s.dir, nil, nil, false)

o, err := overlord.New(&overlord.Opts{

PebbleDir: s.dir,

RestartHandler: nil,

ServiceOutput: nil,

Dry: false,

})

But that's unrelated to this specific PR.

This method has remained quite simple, with those couple of arguments, for several years now. But every few weeks we have a PR that tries to add more parameters to it, and if we had allowed these to go on we'd have a struct with 50 different options by now. Not everything that needs to be passed down a chain of objects needs to be in a constructor. In the case at hand, per the other comment, we should not do DryStart on the Init, but rather inside Start itself and only if it has not yet run externally. On the Daemon, we can call a DryStart on the overlord itself when we're doing a dry run.

niemeyer

Thanks Angel. Probably the last pass.

internals/overlord/stateengine.go

niemeyer · 2023-06-23T13:36:17Z

internals/cli/cmd_run.go

@@ -154,6 +157,9 @@ func runDaemon(rcmd *cmdRun, ch chan os.Signal, ready chan<- func()) error {
 	if err != nil {
 		return err
 	}
+	if rcmd.Dry {


Shouldn't this go all the way to the sanityCheck below, right before Start?

I've extensively discussed this with @flotter, and one of his (rightful) concerns where around system-wide side effects caused by e.g. listening on a port, since it's an "external" side effect (i.e. contributes to Pebble initialization by persisting some state externally, be it listening on a socket or calling .Checkpoint() on the state backend).

The idea is for the validation to be contained in the .DryStart() methods, and also sanityCheck() is a no-op for now anyway.

Yeah, there's definitely a limit that is worth considering, so @flotter is right on track. The question here is the cost-benefit: what are we leaving out by following a bit further, and what are the issues that can happen. For example, if we just open the port, it means we've validated that the port may be opened at all. Given that benefit, what's the cost? Will any messages be handled, or maybe not because that's done further down?

niemeyer · 2023-06-23T13:39:52Z

internals/overlord/overlord.go

@@ -133,6 +141,11 @@ func New(pebbleDir string, restartHandler restart.Handler, serviceOutput io.Writ
 	// the shared task runner should be added last!
 	o.stateEng.AddManager(o.runner)

+	// Dry start all managers.
+	if err := o.stateEng.DryStart(); err != nil {


Indeed, but it's a bit awkward to see this here at the end of Init.

I think this shoud be run when starting, or on a specific DryStart method that only does the dry-starting. This way we also don't need to provide further arguments into the constructor here, and no bools either.

niemeyer · 2023-06-23T13:43:29Z

internals/overlord/managers_test.go

@@ -42,7 +42,7 @@ func (s *mgrsSuite) SetUpTest(c *C) {

 	s.dir = c.MkDir()

-	o, err := overlord.New(s.dir, nil, nil)
+	o, err := overlord.New(s.dir, nil, nil, false)


This method has remained quite simple, with those couple of arguments, for several years now. But every few weeks we have a PR that tries to add more parameters to it, and if we had allowed these to go on we'd have a struct with 50 different options by now. Not everything that needs to be passed down a chain of objects needs to be in a constructor. In the case at hand, per the other comment, we should not do DryStart on the Init, but rather inside Start itself and only if it has not yet run externally. On the Daemon, we can call a DryStart on the overlord itself when we're doing a dry run.

internals/overlord/stateengine.go

internals/overlord/overlord.go

internals/overlord/stateengine.go

niemeyer · 2023-06-26T12:45:40Z

anpep force-pushed the overlord-prevalidation branch from 73f1670 to 782783f
4 hours ago

Not sure if we discussed this before, but as a rule let's please not force-push after the reviews have initiated, as it makes incremental reviews uncomfortatable given the current GitHub features.

jnsgruk · 2023-08-01T16:18:42Z

@anpep looks like we could do with a merge conflict fixing here, and still some outstanding feedback.

anpep · 2023-08-02T07:58:17Z

@anpep looks like we could do with a merge conflict fixing here, and still some outstanding feedback.

This PR still needs some discussion before getting in—will try to catch up rather soon.

thp-canonical · 2023-11-20T08:37:10Z

From the PR description:

The --dry option is useful for validating the Pebble layers without having to actually start the Pebble daemon.

From what I can tell looking at the code, Rockcraft currently uses Pydantic for validating the Pebble configuration it creates (implemented in canonical/rockcraft#331, in rockcraft/pebble.py).

Is this --dry feature something that Rockcraft could eventually use instead of Pydantic, or do the two implementations (--dry vs Pydantic) solve different problems and can/should coexist?

Or alternatively, would using Pydantic in *craft tools be a good alternative to running Pebble with --dry?

cc @cjdcordeiro and @tigarmo for the Rockcraft/*craft view on this, so we don't duplicate (and maintain) Pebble layer validation as two different methods in various craft tools.

cjdcordeiro · 2023-11-20T11:02:15Z

@thp-canonical Ideally, the two would coexist. Rockcraft would still use Pydantic to declare the services and checks top-level fields, but then possibly rely on --dry to validate the user's YAML input.

tigarmo · 2023-11-21T10:22:15Z

I sometimes worry about the two validations "drifting". Since Rockcraft provisions latest/stable Pebble, we kind of have to evolve in lockstep. For example: if the Pebble service layer format gets a new field, users of Rockcraft won't be able to use it until we update the Pydantic validation and release a whole new version of the tool.

benhoyt · 2024-03-11T09:15:58Z

Per discussion with @anpep, this is likely to end up in a different form later, so closing this for now.

anpep requested review from benhoyt, niemeyer, flotter and atesburak April 18, 2023 12:48

anpep self-assigned this Apr 18, 2023

benhoyt reviewed Apr 24, 2023

View reviewed changes

internal/overlord/backend.go Outdated Show resolved Hide resolved

internal/overlord/stateengine.go Outdated Show resolved Hide resolved

internal/overlord/stateengine.go Outdated Show resolved Hide resolved

internal/overlord/stateengine.go Outdated Show resolved Hide resolved

anpep added the High Priority Look at me first label Apr 24, 2023

benhoyt reviewed Apr 24, 2023

View reviewed changes

internal/overlord/stateengine.go Outdated Show resolved Hide resolved

flotter reviewed May 25, 2023

View reviewed changes

cmd/pebble/cmd_run.go Outdated Show resolved Hide resolved

internal/daemon/daemon.go Outdated Show resolved Hide resolved

anpep commented Jun 6, 2023

View reviewed changes

internal/overlord/servstate/manager.go Outdated Show resolved Hide resolved

anpep removed the High Priority Look at me first label Jun 7, 2023

Add dry run on daemon and managers

8ecc565

anpep force-pushed the overlord-prevalidation branch from 71205fb to 8ecc565 Compare June 8, 2023 11:57

anpep requested review from flotter and benhoyt June 8, 2023 11:57

benhoyt reviewed Jun 9, 2023

View reviewed changes

Minor fixes

901dcd4

* Remove dryStartCallback from sampleManager. * Unexport ErrStateEngineStopped->errStateEngineStopped. * Simplify multiError string when there's only one error. * Fix mistakenly swapped logging of dry start/ensure errors.

flotter approved these changes Jun 9, 2023

View reviewed changes

flotter approved these changes Jun 12, 2023

View reviewed changes

internals/overlord/backend.go Outdated Show resolved Hide resolved

anpep force-pushed the overlord-prevalidation branch from ca362d8 to f615dbf Compare June 12, 2023 16:08

paul-rodriguez reviewed Jun 13, 2023

View reviewed changes

paul-rodriguez approved these changes Jun 19, 2023

View reviewed changes

benhoyt approved these changes Jun 19, 2023

View reviewed changes

anpep added the High Priority Look at me first label Jun 23, 2023

atesburak approved these changes Jun 23, 2023

View reviewed changes

niemeyer requested changes Jun 23, 2023

View reviewed changes

cli: import multiError implementation from snapd

782783f

anpep force-pushed the overlord-prevalidation branch from 73f1670 to 782783f Compare June 26, 2023 08:27

niemeyer added Reviewed and removed High Priority Look at me first labels Jun 26, 2023

benhoyt mentioned this pull request Jul 12, 2023

Add support for exec "service-context" canonical/operator#957

Merged

jnsgruk removed the Reviewed label Aug 1, 2023

anpep changed the title ~~Add --dry option to pebble run~~ feat(cli): add --dry option to pebble run Aug 28, 2023

anpep added 2 commits November 10, 2023 08:38

Merge branch 'master' into overlord-prevalidation

73954cf

Merge branch 'master' into overlord-prevalidation

e973894

anpep mentioned this pull request Nov 15, 2023

feat(stateengine): merge StateStarterUp interface from snapd #327

Closed

paul-rodriguez approved these changes Nov 17, 2023

View reviewed changes

benhoyt closed this Mar 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): add `--dry` option to `pebble run` #214

feat(cli): add `--dry` option to `pebble run` #214

anpep commented Apr 18, 2023 •

edited by benhoyt

Loading

benhoyt left a comment

anpep commented Apr 24, 2023 •

edited by benhoyt

Loading

hpidcock commented May 21, 2023

anpep commented May 22, 2023

benhoyt Jun 9, 2023

anpep Jun 9, 2023

niemeyer Jun 23, 2023

anpep Nov 15, 2023

paul-rodriguez Jun 13, 2023

anpep Jun 13, 2023

niemeyer Jun 23, 2023

niemeyer left a comment

niemeyer Jun 23, 2023

anpep Jun 26, 2023

niemeyer Jun 26, 2023

niemeyer Jun 23, 2023

niemeyer Jun 23, 2023

niemeyer commented Jun 26, 2023

jnsgruk commented Aug 1, 2023

anpep commented Aug 2, 2023

thp-canonical commented Nov 20, 2023

cjdcordeiro commented Nov 20, 2023

tigarmo commented Nov 21, 2023

benhoyt commented Mar 11, 2024

-	o, err := overlord.New(s.dir, nil, nil, false)
+	o, err := overlord.New(&overlord.Opts{
+	    PebbleDir: s.dir,
+	    RestartHandler: nil,
+	    ServiceOutput: nil,
+	    Dry: false,
+	})

feat(cli): add --dry option to pebble run #214

feat(cli): add --dry option to pebble run #214

Conversation

anpep commented Apr 18, 2023 • edited by benhoyt Loading

benhoyt left a comment

Choose a reason for hiding this comment

anpep commented Apr 24, 2023 • edited by benhoyt Loading

hpidcock commented May 21, 2023

anpep commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer commented Jun 26, 2023

jnsgruk commented Aug 1, 2023

anpep commented Aug 2, 2023

thp-canonical commented Nov 20, 2023

cjdcordeiro commented Nov 20, 2023

tigarmo commented Nov 21, 2023

benhoyt commented Mar 11, 2024

feat(cli): add `--dry` option to `pebble run` #214

feat(cli): add `--dry` option to `pebble run` #214

anpep commented Apr 18, 2023 •

edited by benhoyt

Loading

anpep commented Apr 24, 2023 •

edited by benhoyt

Loading