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

Add "Using Pebble" documentation to README #162

Merged
merged 4 commits into from
Nov 21, 2022

Conversation

benhoyt
Copy link
Contributor

@benhoyt benhoyt commented Oct 26, 2022

This PR adds a large section of documentation to the README about how to use Pebble, and how some of its major features work. This is intended as a mini user manual; several people have asked about how these things work, and I've previously had to say "sorry, it's documented in the code". :-)

Copy link
Contributor

@barrettj12 barrettj12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really nice addition. What do you think about putting it in a separate file though? This README is already getting quite bloated - I'd even support moving some of the stuff currently in the README to separate files.

Copy link
Member

@tmihoc tmihoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a bunch of comments, mainly regarding the structure.

@@ -9,7 +9,7 @@ designed with unique features that help with more specific use cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first few lines should say (1) what Pebble is, (2) what it does, (3) what problem it solves, and (4) who it's useful for. The current write-up is great but seems to cover only point (3).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking at the doc as a whole, and I agree this could use improvement. However, the scope of this PR is just adding the "Using Pebble" documentation, so I'm going to leave this for another PR.

@@ -118,26 +118,248 @@ services:
command: cmd
```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the configuration examples: They are great but should be given in addition to the spec, not instead of it. Otherwise we find ourselves wanting to supplement as in "Some details worth highlighting". So, please consider giving the actual spec, answering questions like "What are all the fields, features, values, etc., possible in this file?" You could adopt a format like https://juju.is/docs/sdk/metadata-yaml or, maybe better, https://juju.is/docs/sdk/charmcraft-yaml .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you actually do give the full spec starting with line 359. It should be higher up and the examples should be nested under it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's right, the YAML spec is under "Layer specification" in the format of https://juju.is/docs/sdk/metadata-yaml -- I put it below "Using Pebble" as I felt reference material should be after the general "using Pebble" section.

README.md Outdated
* `tcp`: opening the given TCP port must be successful
* `exec`: executing the specified command must yield a zero exit code

Checks are configured in the layer configuration using the top-level field `checks`. Below is an example layer showing the three different types of checks:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This belongs in the layer configuration spec section. You can reference it from here. Or maybe have the generic spec there and a link to that + a concrete example here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize there's a bit of duplication here, however, I think it's important to know the check types up-front. But I've added a phrase and link to the full spec below.

@@ -9,7 +9,7 @@ designed with unique features that help with more specific use cases.

- [General model](#general-model)
- [Layer configuration examples](#layer-configuration-examples)
- [Running pebble](#running-pebble)
- [Using pebble](#using-pebble)
- [Layer specification](#layer-specification)
Copy link
Member

@tmihoc tmihoc Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Table of Contents / the organization of the sections, please consider using Diataxis. That is, you should the 5 main sections below, with your existing sections nested within along the following lines (at least, at first glance):

  • Home Introduction (the intro lines I mentioned above)
  • Tutorial
  • How-to guides
    • Install (i.e., How to install Pebble)
    • Set up (i.e., How to set up the Pebble directory)
    • Use (i.e., How to use Pebble)
    • Run the daemon (i.e., How to run the Pebble daemon)
    • Start and stop services (i.e., How to start and stop Pebble services)
  • Reference
    • General model
    • Layer configuration spec + examples
    • pebble CLI
      • List of pebble CLI commands
    • Service dependencies
    • Health checks
    • Changes and tasks
    • ...
  • Explanation

At first glance, all the materials you have are either How-to guides or Reference, so you can leave out the Tutorial and Explanation for now. As for how to organize How-to guides vs. Reference: I'd put any material that defines things into Reference and any material that gives instructions into How-to guides. I'd also try to organize the Reference docs around primitive concepts and bits and then the How-to guides also around that where, for a product that has a CLI, the how-to guides are usually about manipulating a primitive entity in various ways using various CLI commands. See, for example, https://juju.is/docs/olm/the-juju-client and https://juju.is/docs/olm/manage-juju, or https://juju.is/docs/olm/network-spaces and https://juju.is/docs/olm/manage-spaces , or https://juju.is/docs/olm/action and https://juju.is/docs/olm/manage-actions .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good discussion, however, it's a big structural change to the rest of the document -- I'm going to leave this for future work.

@benhoyt
Copy link
Contributor Author

benhoyt commented Oct 26, 2022

What do you think about putting it in a separate file though? This README is already getting quite bloated - I'd even support moving some of the stuff currently in the README to separate files.

@barrettj12 I thought about this, but on balance I think I prefer it in a single file. If anything I think we could split out "Using Pebble" into manual.md or something, and "Layer specification" into layer-spec.md. Also interested to hear @niemeyer's opinion here.

Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good idea to improve our docs, but the proposal feels a bit too dense and too shallow at the same time. Some points related to this:

README.md Outdated Show resolved Hide resolved
README.md Outdated
...
```

The daemon's service manager stores the most recent stdout and stderr from each service (using a 100KB ring buffer per service). These are accessible via the logs API or using `pebble logs`. If you want to also write service logs to Pebble's own stdout, run the daemon with `--verbose`:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be going too fast into debugging. Instead, we should be talking about the model: statuses, plan, changes, etc. Otherwise this smells like a boring process runner, instead of a nicely structured service support system.

To improve this, I suggest cutting off right before the run command output, continuing through the rest of the experience, and only later mentioning the ability to see logs.

Also, before we start/stop and change the state, let's talk about reading the current state that took place right after running.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the logging stuff was going too deep (and tangential) here -- fixed. I've added a separate sub-section "Logs" at the bottom of the "Using Pebble" section. Regarding "statuses, plan, changes, etc": We talk about the plan/config in the first section as mentioned, and changes and tasks in their own section below.

README.md Outdated
$ pebble stop srv1 # stop one service
```

When starting a service, Pebble records a "Start" [change](#changes-and-tasks) for the operation, executes the service's `command`, and waits 1 second to ensure the command doesn't exit too quickly. Assuming the command doesn't exit within 1 second, the start is considered successful, otherwise `pebble start` will exit with an error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems premature to dive so deeply into the details of process management here. I'd reserve such details for an advanced section, and even then probably avoid promising things like exact timings as that can easily change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm refactored this section a little, but I disagree that this is too much detail here. This README is what we have for a user manual right now, so I think it should explain it, not try to stuff important details in an "advanced" section.

I at first removed the exact timings, but the current timings are part of said user manual, and as such should be documented. I think it's important, otherwise if we say something like "waits a short time" users will be thinking, "Okay, but how long is short?"

README.md Outdated
* `backoff`: in a [backoff-restart loop](#service-auto-restart)
* `error`: in an error state (currently: exited and exit action is "ignore")

Implementation note: Pebble's service manager uses a [state machine](https://github.com/canonical/pebble/blob/a1f52239079e58cb6e3b6c7472ef5438b91eefab/internal/overlord/servstate/handlers.go#L67-L82) to manage the life cycle of a service. You can see these internal states and their transitions in the [state diagram](https://raw.githubusercontent.com/canonical/pebble/master/internal/overlord/servstate/state-diagram.svg).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the above point, we don't want to be documenting internal details of the implementation and pointing people to the code in a section that is intended for a new user that has never seen Pebble.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair -- I've removed this paragraph.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes. There's more work to do in the README, but I also wonder how much we should improve in until it becomes worthy of its own site.

@niemeyer niemeyer merged commit e255465 into canonical:master Nov 21, 2022
@benhoyt benhoyt deleted the readme-manual branch November 21, 2022 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants