-
Notifications
You must be signed in to change notification settings - Fork 908
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
Collect logs improvements #5619
Collect logs improvements #5619
Conversation
* Collect sensitive data by default since we ask for it more often than not * Glob most of /run/cloud-init, /etc/cloud, and /var/lib/cloud * Stop creating empty directories in the tarball * Require running as root given that the logs are root read-only
Looks like this also fixes #5297
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheRealFalcon nice work here, I've left a couple of comments inline but overall this looks good to me. Running locally I see the following files grabbed:
# tree
.
├── dmesg.txt
├── dpkg-version
├── etc
│ └── cloud
│ ├── build.info
│ ├── cloud.cfg
│ └── cloud.cfg.d
│ ├── 05_logging.cfg
│ ├── 90_dpkg.cfg
│ └── README
├── journal.txt
├── run
│ └── cloud-init
│ ├── cloud-id
│ ├── cloud-id-lxd
│ ├── cloud-init-generator.log
│ ├── cloud.cfg
│ ├── combined-cloud-config.json
│ ├── ds-identify.log
│ ├── enabled
│ ├── instance-data-sensitive.json
│ ├── instance-data.json
│ ├── result.json
│ └── status.json
├── var
│ ├── lib
│ │ └── cloud
│ │ ├── data
│ │ │ ├── instance-id
│ │ │ ├── previous-datasource
│ │ │ ├── previous-hostname
│ │ │ ├── previous-instance-id
│ │ │ ├── python-version
│ │ │ ├── result.json
│ │ │ ├── set-hostname
│ │ │ └── status.json
│ │ └── instance
│ │ ├── boot-finished
│ │ ├── cloud-config.txt
│ │ ├── datasource
│ │ ├── network-config.json
│ │ ├── obj.pkl
│ │ ├── user-data.txt
│ │ ├── user-data.txt.i
│ │ ├── vendor-data.txt
│ │ ├── vendor-data.txt.i
│ │ ├── vendor-data2.txt
│ │ └── vendor-data2.txt.i
│ └── log
│ ├── cloud-init-output.log
│ └── cloud-init.log
└── version
from cloudinit.stages import Init | ||
from cloudinit.subp import ProcessExecutionError, subp | ||
from cloudinit.temp_utils import tempdir | ||
from cloudinit.util import ( | ||
chdir, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that you didn't write this, but we shouldn't need this helper at all. This is the only callsite of util.chdir()
, yet tar
(including busybox tar) has -C
which can switch directories before running the operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alas, it is still used in bbdeb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alas, it is still used in bbdeb.
I'm pretty strongly opposed to shipping dead code in cloud-init that exists only for use in external utilities, but I'm also fine with saying that it's not worth dealing with right now.
@holmanb , I believe I addressed all of your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, @TheRealFalcon. Tested, thanks for this PR!
Proposed Commit Message
Additional Context
Test Steps
Merge type