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 oxlog tool and library #4810

Merged
merged 19 commits into from
Jan 23, 2024
Merged

Add oxlog tool and library #4810

merged 19 commits into from
Jan 23, 2024

Conversation

andrewjstone
Copy link
Contributor

@andrewjstone andrewjstone commented Jan 12, 2024

No description provided.

This still needs a bit of work before it is ready to go.
}
}

fn load_svc_logs(dir: Utf8PathBuf, logs: &mut BTreeMap<ServiceName, SvcLogs>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is some similar logic here for finding logs during zone-bundling, it might be a good idea to share it somehow. Calling into this library in the running_zone mod seems reasonable.

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 can certainly add a helper function to do this. However, right now oxlog only looks at logs with the oxide- prefix. There are a lot more with the system-prefix. Should I make that a separate type of retrieval?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure. The system-illumos- prefix was included because there are some of our services that still use the original naming of the SMF services, like svc:/system/illumos/propolis-server. (I think some of the networking daemons are still named this way too.)

Searching for that prefix becomes more reasonable when we're only looking for logs that come from a specific, named service, which is the case of the zone bundling process. That might be too permissive for oxlog, I'm really not sure. If that's true, then maybe we can commonize the portion of the checks that make sense (say, for SMF services named like svc:/oxide/blah). It seems like you might also be trying to capture all log files for any service, not just the "primary" Oxide services in zone. In that case, the commonization might be less helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Searching for that prefix becomes more reasonable when we're only looking for logs that come from a specific, named service, which is the case of the zone bundling process. That might be too permissive for oxlog, I'm really not sure.

I think this is likely to be a common use case, but we also capture all logs in a zone. This means that we'll suck up all 'system-' prefixed logs unless we hardcode a list of services that can be named like this. I've so far avoiding hardcoding the names of any services and deducing service names by the names of the log-files. It turns out that strategy falls apart because of this dichotomy in naming. It seems to me that we would be better off changing the prefix for things like propolis to be oxide-, but in the meantime I can suck up only files that match a hardcoded set of services so we don't miss them. I specifically do not want to include all system service files as there are so many and mostly we won't want to dig through them in the common case. Again, though, we likely will want to be able to retrieve those files in some debugging scenarios though. Getting them through sled-agent integration via oxlog is preferable than having to ssh in. I will add some support for this as well.

It seems like you might also be trying to capture all log files for any service, not just the "primary" Oxide services in zone. In that case, the commonization might be less helpful.

I'm trying to gather any oxide specific service log in a zone, ideally without knowing the name of the service ahead of time. That may be too ambitious.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm trying to gather any oxide specific service log in a zone, ideally without knowing the name of the service ahead of time. That may be too ambitious.

So the way we did that in zone-bundling is to list all SMF FMRIs matching the patterns svc:/oxide/* or svc:/system/illumos/*, and then listing the logfiles associated with each of those (with svcs -L).

Based on your answer, it does really sound like you want to use the same logic as in the zone bundling, or at least, use that logic possibly in addition to some other. Note that the prefix used in the zone-bundling is system-illumos-* not system-*. The latter is definitely very permissive, and will get you a bunch of stuff unrelated to Oxide services. If the goal is Oxide-specific log files, then I think the logic can be shared with zone-bundling :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the prefix used in the zone-bundling is system-illumos-* not system-*. The latter is definitely very permissive, and will get you a bunch of stuff unrelated to Oxide services. If the goal is Oxide-specific log files, then I think the logic can be shared with zone-bundling :)

Aha! Alright, so far the only dependency for oxlog is using the filesystem. But I can definitely parse using system-illumos-* and include those files. Then we can share the logic.

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 added this support in c38d7e4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

illumos-utils now uses the method which I copied into oxlog from oxlog as of caf3cd5


/// Print only the archive log files
#[arg(short, long)]
archived: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there is a third class, rotated log files that have not yet been archived. These probably don't live very long, since the archiver runs frequently. But we may want to consider including them and a CLI switch to filter them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are actually included with the archived files right now. I"m happy to separate them out though if you think that makes sense.

@andrewjstone andrewjstone marked this pull request as ready for review January 18, 2024 05:47
@andrewjstone andrewjstone requested a review from smklein January 18, 2024 06:23
@andrewjstone andrewjstone changed the title WIP: Add oxlog tool and library Add oxlog tool and library Jan 18, 2024

impl Pools {
pub fn read() -> anyhow::Result<Pools> {
let internal = read_dir("/pool/int/")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't actually read anything from the internal_pools, but I don't think it hurts to keep this type around for now. We may end up reading if we expand this tool to grab other files.

This was one of the first bits of code I wrote for this PR and I left it through all the iterations. Happy to change/remove if desired though.

dev-tools/oxlog/src/lib.rs Outdated Show resolved Hide resolved
pub use super::oxide_smf_service_name_from_log_file_name;

#[test]
fn test_is_oxide_smf_log_file() {
Copy link
Contributor Author

@andrewjstone andrewjstone Jan 18, 2024

Choose a reason for hiding this comment

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

I copied this from the illumos_utils code along with the related method. I added an extra test at the end to ensure we don't also collect system-* along with system-illumos-* files.

@andrewjstone
Copy link
Contributor Author

I also need to clean up the packaging into the global zone as @smklein pointed out: #4840 (comment)

@andrewjstone
Copy link
Contributor Author

I also need to clean up the packaging into the global zone as @smklein pointed out: #4840 (comment)

Implemented in 09ad1b5

dev-tools/oxlog/Cargo.toml Outdated Show resolved Hide resolved
package-manifest.toml Show resolved Hide resolved
dev-tools/oxlog/src/lib.rs Show resolved Hide resolved
dev-tools/oxlog/src/lib.rs Outdated Show resolved Hide resolved
dev-tools/oxlog/src/lib.rs Outdated Show resolved Hide resolved
@andrewjstone andrewjstone enabled auto-merge (squash) January 23, 2024 21:17
@andrewjstone andrewjstone merged commit 66afddb into main Jan 23, 2024
22 checks passed
@andrewjstone andrewjstone deleted the oxlog branch January 23, 2024 22:10
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.

3 participants