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

Optimize memory usage of unpacking FBC from pod logs #22

Closed
joelanford opened this issue Mar 31, 2023 · 5 comments · Fixed by #145
Closed

Optimize memory usage of unpacking FBC from pod logs #22

joelanford opened this issue Mar 31, 2023 · 5 comments · Fixed by #145

Comments

@joelanford
Copy link
Member

joelanford commented Mar 31, 2023

In these lines, we copy the logs to a byte buffer, and then turn around and create a reader from the byte buffer.

We may be able to avoid the buffer entirely by feeding the podLog stream directly into declcfg.LoadReader.

Acceptance Criteria:

@everettraven
Copy link
Collaborator

FWIW I attempted to resolve this with 7abf80b#diff-025e5ad2e9e8e55336b0ed5c705b28593c6e6fa342ca89c0ace5e784cd708b2bL298-R394 and the result was that not all objects from the FBC were read before returning. This may indicate that we need to make a change in operator-registry to ensure that when loading from a reader we are reading until we have read everything

@everettraven
Copy link
Collaborator

@joelanford I don't think this is an issue anymore due to the change to using the Unpacker interface and implementation that was done in #65 - are we okay to close this now?

@joelanford
Copy link
Member Author

It seems like we still have a similar problem in the unpacker implementation for images:

defer logReader.Close()
buf := &bytes.Buffer{}
if _, err := io.Copy(buf, logReader); err != nil {
return nil, err
}

However, I think the solution is to replace the pod-based image unpacker with a registry client implementation. Shall we go ahead and embark on that path? I created #143 to describe the idea.

@anik120
Copy link
Collaborator

anik120 commented Sep 15, 2023

This should be superseded by #143 right?

@joelanford
Copy link
Member Author

Yeah, #143 lists this issue (and some others) as issues that will be closed out or obsolete when we implement an image registry client. I'll add "fixes" comments for all of those issues in #145

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 a pull request may close this issue.

3 participants