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

feat(daemon): Add support for PEBBLE_COPY_ONCE. #352

Merged
merged 5 commits into from
Feb 23, 2024

Conversation

hpidcock
Copy link
Member

@hpidcock hpidcock commented Feb 9, 2024

$PEBBLE_COPY_ONCE environment variable allows pebble run to copy layers and state from another directory when seeding a new $PEBBLE directory.

The rationale for this change is to allow OCI images to specify existing layers in the default $PEBBLE location. Then at runtime, be able to run pebble with a different $PEBBLE directory. This allows pebble to be run as a non-root user or run on a read-only root filesystem.

Issue #351 outlines that pebble run has no unit or integration tests. A test should be performed manually for now.

Manual QA is as follows:

  • Create a directory, with a layers subdirectory and valid testing layers (e.g. ~/dir/with-existing-state)
  • Create another empty directory. (e.g. ~/dir/with-no-state)
  • PEBBLE_COPY_ONCE=~/dir/with-existing-state PEBBLE=~/dir/with-no-state pebble run
  • Check all the layers are loaded.
  • Check all the layers are copied.
  • Modify a layer in ~/dir/with-no-state/layers.
  • Restart pebble run and check that it doesn't crash.
  • Check that the modifications are preserved after the restart (PEBBLE_COPY_ONCE is only for an empty directory).
  • Check PEBBLE_COPY_ONCE can be set to a non-existing directory with pebble run passed --create-dirs

JU090
JUJU-5435

@hpidcock hpidcock added the Blocked Waiting for something external label Feb 9, 2024
Copy link
Contributor

@benhoyt benhoyt 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 PR. However, do we really need a 3rd party dependency for this? Given what we need to do, it's quite a large one (1500 LoC and significant API surface area). We only need to copy files, and it doesn't even need to be recursive, right? (Though that's easy with filepath.WalkDir if we need to.) Here's an implementation of (the upcoming) fs.CopyFS for reference: golang/go#62484 (comment) -- not many more lines of code than calling the 3rd party lib. :-) Happy to discuss voice if you disagree.

Regarding testing, I'd be happy with manual testing for this PR, until we get better integration/spared tests -- we don't have any for the rest of the pebble run behaviour, so it's no worse. :-) That said, most of this is just file/dir copying, which is easily go testable, right?

@benhoyt
Copy link
Contributor

benhoyt commented Feb 20, 2024

One other thought: we should document this in README.md somewhere. Or maybe in the help text?

@hpidcock hpidcock removed the Blocked Waiting for something external label Feb 22, 2024
@hpidcock hpidcock changed the title feat(daemon): Add support for PEBBLE_INIT_FROM. feat(daemon): Add support for PEBBLE_COPY_ONCE. Feb 22, 2024
`PEBBLE_COPY_ONCE` allows `pebble run` to copy layers and state from
another directory when seeding a new `PEBBLE` directory.
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

A bunch of minor comments, plus a request for a test (of maybeCopyPebbleDir, not of cmd_run).

internals/cli/cmd_run.go Outdated Show resolved Hide resolved
internals/cli/cmd_run.go Outdated Show resolved Hide resolved
internals/cli/cmd_run.go Outdated Show resolved Hide resolved
internals/cli/cmd_run.go Outdated Show resolved Hide resolved
internals/cli/cmd_run.go Outdated Show resolved Hide resolved
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Looks good to me, just requesting a couple of minor test changes, and then I'll merge.

internals/cli/cmd_run_test.go Outdated Show resolved Hide resolved
internals/cli/cmd_run_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Works for me, thanks!

@benhoyt benhoyt merged commit ff7af10 into canonical:master Feb 23, 2024
15 checks passed
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.

2 participants