Container logs can be retrieved #135
Conversation
There was a problem hiding this comment.
I've left some comments, nothing that I'd consider a blocker though, so I'll approve this, but am happy to discuss the comments of course.
Regarding the "missing" timestamp issue I've opened krustlet/krustlet#591 to make processing of additional parameters possible.
|
|
||
| if let Some(line_count) = sender.tail() { | ||
| journal.seek_tail()?; | ||
| let skipped = journal.previous_skip(line_count as u64 + 1)?; |
There was a problem hiding this comment.
More a question out of interest, what happens to the reader when we skip more lines than there are in the log here? Is that defined behavior? If so, can we potentially get rid of the conditional seek_head() ?
There was a problem hiding this comment.
If line_count is greater than the number of journal entries then previous_skip sets the cursor to the first entry. The following code assumes that the cursor is set to one position before the next entry. Therefore if the beginning of the journal is reached then the cursor must be set to one position before the first entry which is done by seek_head.
I will try to make this more clear in the code.
There was a problem hiding this comment.
I hope it is clearer now.
| ) -> Result<()> { | ||
| let mut sent = 0; | ||
| let mut message_available = true; | ||
| while sent != count && message_available { |
There was a problem hiding this comment.
Is there a reason you are using != instead of < ?
I agree with you that this should never break, but out of instinct usually take the "safe" route..
There was a problem hiding this comment.
If sent could become greater then count then it would be a programming error. If I would use < instead of != the programming error would be masked and a potentially incorrect program state would be propagated. If an error occurs further down the line then it would be difficult to find the root cause. Therefore I prefer offensive programming over defensive programming.
If we decide to go the other way, I am also okay.
Actually I am not happy with this code:
- I did not use (somehow) the type system to make the state where
sent > countunrepresentable. - I use a counter.
- The reading and sending of messages is intertwined.
But this is probably how it is done in Rust without overcomplicating things.
| @@ -45,10 +49,116 @@ mod systemdmanager; | |||
| /// Provider-level state shared between all pods | |||
| #[derive(Clone)] | |||
| pub struct ProviderState { | |||
| handles: Arc<RwLock<PodHandleMap>>, | |||
There was a problem hiding this comment.
It might just be me, so this is really just a note, feel free to leave as is.
I found it very hard to grok what exactly a PodHandleMap is due to the mixture of structs and types that are defined here as well as in the kubelet.
It might make sense to insert a brief comment that shows this..or at least explains it.
There was a problem hiding this comment.
I extended the documentation of PodHandleMap.
src/provider/mod.rs
Outdated
| ) -> anyhow::Result<()> { | ||
| debug!("Logs requested"); |
There was a problem hiding this comment.
Maybe add pod and container information to the log message?
Closes #37