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: expose the ability to set Pebble log targets #1074

Merged

Conversation

tonyandrewmeyer
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer commented Nov 22, 2023

Exposes the log-targets section of the Pebble layer spec, to allow charms to configure log forwarding in Pebble without having to provide layers as plain YAML.

New types:

  • LogTargetDict: a typed dictionary similar to ServiceDict and CheckDict.
  • LogTarget: similar to Service and Check.

Changed types:

  • LayerDict and PlanDict have (not required) log-targets keys.
  • Layer and Plan have log_targets attributes (Plan is read-only, as with checks and services).

Fixes #1073

ops/pebble.py Outdated Show resolved Hide resolved
Copy link

@kayra1 kayra1 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 fast response!

Copy link
Collaborator

@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.

Nice, thanks! A few minor comments.

Could you also please do one quick test against a real Pebble instance with a log-targets and make sure it's coming through okay? I'd be okay with just a manual test hacking test/pebble_cli.py or something, but an automated test in test_real_pebble.py would be fine too if it's straight-forward.

ops/pebble.py Outdated Show resolved Hide resolved
ops/pebble.py Outdated Show resolved Hide resolved
test/test_pebble.py Outdated Show resolved Hide resolved
@tonyandrewmeyer
Copy link
Contributor Author

Could you also please do one quick test against a real Pebble instance with a log-targets and make sure it's coming through okay? I'd be okay with just a manual test hacking test/pebble_cli.py or something, but an automated test in test_real_pebble.py would be fine too if it's straight-forward.

Sure. I had wondered about a test_real_pebble addition, but when looking through what that tests, it seemed to be more the actual Pebble functionality (pushing and pulling, running health checks, exec'ing) rather than just config. It seems complicated to do that with log forwarding, since I assume a Loki server would be required. What would you like a real_pebble test to do? Just check that particular config is appropriately created?

Also for this: the CI real pebble test gets the latest version of (main) pebble. Is it ok to assume that anyone running tox -e pebble also has the latest version, or should the tests check the version and skip appropriately?

@benhoyt
Copy link
Collaborator

benhoyt commented Nov 23, 2023

What would you like a real_pebble test to do? Just check that particular config is appropriately created?

Yeah, I wasn't thinking it'd test the Loki part -- just let the log forwarding itself fail. Just test that adding a layer with a log target can be queried and comes through with appropriate attributes in get_plan or something.

Also for this: the CI real pebble test gets the latest version of (main) pebble. Is it ok to assume that anyone running tox -e pebble also has the latest version, or should the tests check the version and skip appropriately?

Yeah, I think it's fine to assume the dev is on Pebble latest, no need for version checks.

@tonyandrewmeyer
Copy link
Contributor Author

@benhoyt the tests fail because the workflow uses pebble@latest. Is it ok to change that to pebble@master? (They pass locally for me in that case). I'm not sure whether we want to verify that we're compatible with the latest released pebble or the next version of pebble.

If we don't change it, then I can disable the labels until the next pebble release.

test/test_real_pebble.py Outdated Show resolved Hide resolved
@benhoyt
Copy link
Collaborator

benhoyt commented Nov 23, 2023

@tonyandrewmeyer Yep, I think it's fine to have the real Pebble tests work off @master. We own the project and the master branch is (should be!) stable.

Copy link
Collaborator

@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.

Approved. Just one minor comment about the HACKING.md change now that we're using @master.

HACKING.md Outdated Show resolved Hide resolved
@benhoyt benhoyt merged commit b410730 into canonical:main Nov 23, 2023
20 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the log-forwarding-pebble-layer-1073 branch November 23, 2023 22:51
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.

Layer object support for log-forwarding as a field
3 participants