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

fix(kubernetes_logs source): Start reading logs at checkpoint #4043

Merged
merged 2 commits into from
Sep 24, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion src/sources/kubernetes_logs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,20 +191,56 @@ impl Source {
let annotator = PodMetadataAnnotator::new(state_reader, fields_spec);

// TODO: maybe some of the parameters have to be configurable.

// The 16KB is the maximum size of the payload at single line for both
// docker and CRI log formats.
// We take a double of that to account for metadata and padding, and to
// have a power of two rounding. Line splitting is countered at the
// parsers, see the `partial_events_merger` logic.
let max_line_bytes = 32 * 1024; // 32 KiB
let file_server = FileServer {
// Use our special paths provider.
paths_provider,
// This is the default value for the read buffer size.
max_read_bytes: 2048,
start_at_beginning: true,
// We want to use checkpoining mechanism, and resume from where we
// left off.
start_at_beginning: false,
Copy link
Member

Choose a reason for hiding this comment

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

Why are we not relying on the Default implementation here? That would have avoided this bug in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FileServer does not implement Default.
I know you've asked this before, and I was confused by this question last time, so might have forgot to address it, sorry.

I might still be confused, in that case could you please elaborate?

Copy link
Contributor

@Hoverbear Hoverbear Sep 21, 2020

Choose a reason for hiding this comment

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

Indeed, FileServer does not have a safe constructor with defaults.

I think adding such would be a separate ticket, which I feel should be done, since right now it's very difficult to use the FileServer type in a 'default' way.

Copy link
Contributor Author

@MOZGIII MOZGIII Sep 21, 2020

Choose a reason for hiding this comment

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

I think adding such would be a separate ticket, which I feel should be done, since right now it's very difficult to use the FileServer type in a 'default' way.

I'm not sure if default for the whole thing makes sense. But we could extract a subset of parameters into a Default-constructible object and pass that.

However, even then, for something like a kubernetes_logs source, I'd assign most of the settings manually. For instance, this one should probably be set in the source level rather than being defined at the Default that may change independently of the source (and potentially break the compatibility).

Some of the options I'd definitely offload to Default, like max_read_bytes.

We also need a proper test for this behaviour at the E2E, I'll create an issue (UPD: #4047).

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I should have been clearer. We have encoded a reasonable set of default values into the Default implementation for FileConfig. It would take some tweaking, but we should really try to rely on those defaults wherever the source is reused.

Copy link
Contributor Author

@MOZGIII MOZGIII Sep 21, 2020

Choose a reason for hiding this comment

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

Ah, makes sense! FileConfig isn't exactly a good fit for the use in kubernetes_logs source, so we probably need to add a layer of indirection.
I'll create an issue for it (UPD: #4048).

// We're now aware of the use cases that would require specifying
// the starting point in time since when we should collect the logs,
// so we just disable it. If users ask, we can expose it. There may
// be other, more sound ways for users considering the use of this
// option to solvce their use case, so take consideration.
ignore_before: None,
// Max line length to expect during regular log reads, see the
// explanation above.
max_line_bytes,
// The directory where to keep the checkpoints.
data_dir,
// This value specifies not exactly the globbing, but interval
// between the polling the files to watch from the `paths_provider`.
// This is quite efficient, yet might still create some load of the
// file system, so this call is 10 times larger than the default for
// the files.
glob_minimum_cooldown: Duration::from_secs(10),
// The shape of the log files is well-known in the Kubernetes
// environment, so we pick the a specially crafted fingerprinter
// for the log files.
fingerprinter: Fingerprinter::FirstLineChecksum {
// Max line length to expect during fingerprinting, see the
// explanation above.
max_line_length: max_line_bytes,
},
// We expect the files distribution to not be a concern because of
// the way we pick files for gathering: for each container, only the
// last log file is currently picked. Thus there's no need for
// ordering, as each logical log stream is guaranteed to start with
// just one file, makis it impossible to interleave with other
// relevant log lines in the absense of such relevant log lines.
oldest_first: false,
// We do not remove the log files, `kubelet` is responsible for it.
remove_after: None,
// The standard emitter.
emitter: FileSourceInternalEventsEmitter,
};

Expand Down