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

bugtool: Compress and copy with resume #671

Merged
merged 2 commits into from
Jan 4, 2022

Conversation

sayboras
Copy link
Member

Description

This PR tries to address the below points mentioned in #616

  • Compress bugtool output before transfer
  • Add resume capability if there is any transfer error

Changes

Please refer to individual commit for more details

Testing

Testing was done as per below

Make sure that the existing functionalities still working
$ ls -lrth cilium-sysdump-20211224-183*
-rw-rw-r-- 1 tammach tammach 7.7M Dec 24 18:34 cilium-sysdump-20211224-183404.zip --> from master branch
-rw-rw-r-- 1 tammach tammach 7.7M Dec 24 18:34 cilium-sysdump-20211224-183417.zip --> from this branch
Testing with a very large file (~10GB before compressed, 50MB after compressed)
# Manually create the file in cilium pod
dd if=/dev/zero of=filename bs=1 count=0 seek=50G
tar -czvf cilium-bugtool-every-large-file.tar.gz filename

# hard code the file name, and run cilium sysdump
$ ls -lrth cilium-sysdump-20211224-200925.zip                            
-rw-rw-r-- 1 tammach tammach 100M Dec 24 20:09 cilium-sysdump-20211224-200925.zip

This commit is to add gzip compression for cilium-bugtool to reduce size
of transferred file. In local cluster, the size is reduced significantly
(e.g. from 21 MB to ~3MB). Once copied to local system, this file is
extracted again so that there is no change in existing behavior.

```
root@kind-control-plane:/tmp# ls -lrth cilium-bugtool-20211224-072*
-rw-r--r-- 1 root root  21M Dec 24 07:20 cilium-bugtool-20211224-072047.092+0000-UTC-1266452003.tar
-rw-r--r-- 1 root root 3.1M Dec 24 07:21 cilium-bugtool-20211224-072122.815+0000-UTC-2647778632.tar.gz
```

Relates to cilium#616 (comment)

Signed-off-by: Tam Mach <[email protected]>
This commit is to add CopyFromPod function in current k8s client, which
supports resuming copy from last byte offset. Small refactor for exec in pod
method was done to avoid copying reader multiple times.

Relates to cilium#616 (comment)

Signed-off-by: Tam Mach <[email protected]>
@sayboras sayboras temporarily deployed to ci December 24, 2021 09:47 Inactive
@sayboras sayboras marked this pull request as ready for review December 24, 2021 12:48
@sayboras sayboras requested a review from a team as a code owner December 24, 2021 12:48
@sayboras sayboras requested review from a team, ldelossa and christarazi December 24, 2021 12:48
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

LGTM with one non-blocking comment.

sysdump/sysdump.go Show resolved Hide resolved
@tklauser tklauser merged commit b44e090 into cilium:master Jan 4, 2022
@sayboras sayboras deleted the tam/transfer branch January 4, 2022 10:35
Copy link
Contributor

@twpayne twpayne left a comment

Choose a reason for hiding this comment

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

Apologies for the late review, but there are a couple of nits.

@@ -0,0 +1,46 @@
// SPDX-License-Identifier: Apache-2.0
// Copyright 2020 Authors of Cilium
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The copyright year should be 2021. This also applies to other files.

Copy link
Member Author

Choose a reason for hiding this comment

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

my bad :(

outFile, err := os.Create(destFile)
if err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is missing a defer outFile.Close() or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, I forgot about this most of the time, and relying on linters. If I am not wrong, file is having finalizer i.e. fd will be garbage collected later, not idea in case of reuse of file descriptor.

Let me make a small update on this, thanks again.

sayboras added a commit to sayboras/cilium-cli that referenced this pull request Jan 4, 2022
This commit is to correct/add header with year 2022 for new files.

Relates cilium#671 (comment)
sayboras added a commit to sayboras/cilium-cli that referenced this pull request Jan 4, 2022
This commit is to correct/add header with year 2022 for new files.

Relates cilium#671 (comment)

Signed-off-by: Tam Mach <[email protected]>
christarazi pushed a commit that referenced this pull request Jan 4, 2022
This commit is to correct/add header with year 2022 for new files.

Relates #671 (comment)

Signed-off-by: Tam Mach <[email protected]>
aditighag pushed a commit to aditighag/cilium-cli that referenced this pull request Apr 21, 2023
This commit is to correct/add header with year 2022 for new files.

Relates cilium#671 (comment)

Signed-off-by: Tam Mach <[email protected]>
michi-covalent pushed a commit to michi-covalent/cilium that referenced this pull request May 30, 2023
This commit is to correct/add header with year 2022 for new files.

Relates cilium/cilium-cli#671 (comment)

Signed-off-by: Tam Mach <[email protected]>
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.

5 participants