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

compatible: add system logging #136

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

compatible: add system logging #136

wants to merge 10 commits into from

Conversation

phvalguima
Copy link
Contributor

@phvalguima phvalguima commented Jan 26, 2024

DPE-3397: Add support to capture journalctl logs in case of failures.

Adds sosreport support to our pipelines, in case of test failures.

As outputs, we should have now both:

  • juju overview with debug-log + status
  • sos report from the host and each LXC unit

The former provides a detailed view of the model, including with logs from units' charms that were gone at test end. The latter provides a view on the system itself: services running, kernel, etc; at the moment the test has ended.

Unfortunately, neither sos report's juju plugin collects equivalent debug-log, nor juju-crashdump's sosreport plugin collects meaningful sos reports... Therefore, we should keep both.

Bug-Fixes: #133

@phvalguima phvalguima marked this pull request as ready for review January 31, 2024 10:56
@phvalguima phvalguima changed the title compatible(DPE-3397 - Add system logging) compatible: add system logging Jan 31, 2024
Copy link

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

Thank you for pushing sosreport forward. We need to polish it, document and teach users to use them reporting us their issues. We should also include there somehow the versions of components in use... king of juju status --all-models (due to COS versions variations, etc)? Anyway it is a good start. Keep the final decision here for Carl.

@phvalguima
Copy link
Contributor Author

@taurus-forever agreed. I think this is the first of some PRs where we add plugins and configuration for sosreport.

@carlcsaposs-canonical
Copy link
Contributor

@phvalguima have you tested this? if so, can you link to where you tested it so I can see what the output looks like?

Copy link
Contributor

@carlcsaposs-canonical carlcsaposs-canonical left a comment

Choose a reason for hiding this comment

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

some open questions about secret redaction (discussing in mattermost)

(github artifact upload will bypass github's automatic secret redaction. Need to be careful to not leak github secrets [e.g. aws/gcp secrets])

- name: Run SOS reports
if: ${{ failure() && steps.tests.outcome == 'failure' }}
run: |
sudo snap install sosreport --channel=latest/stable --classic
Copy link
Contributor

Choose a reason for hiding this comment

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

we should maybe install from apt

I'd like to use the newer version, but the snap publisher isn't verified

and given that we might be passing github secrets to sosreport, might be a security risk if the snap maintainer is compromised

Comment on lines +345 to +346
- name: Run SOS in LXCs if Needed
if: ${{ inputs.cloud == 'lxd' && (failure() && steps.tests.outcome == 'failure') }}
Copy link
Contributor

Choose a reason for hiding this comment

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

could you separate this step into another PR?

I'd like to get the other changes merged quickly & I think there's some complexity/subtleties (especially around secret redaction) with sosreport inside the lxc container that will hold up the other changes

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.

Add the service {snap, k8s} app logging to our test run outputs
3 participants