Added logic to collect logs on bottlerocket AMI #870
Conversation
|
@suket22 - Can you please help review this? |
| if [ -d "/.bottlerocket/" ]; then | ||
| echo "Detected Bottlerocket AMI" | ||
| is_diskfull | ||
| collect_logs_bottlerocket | ||
| finished | ||
| else | ||
| collect | ||
| pack | ||
| finished |
There was a problem hiding this comment.
Could we create a dedicated log collector file for BR?
We currently separate them by OperatingSystem here so it might be helpful to create a dedicated file for BR rather than add if-else statements to the script meant for the AL2 AMI.
Also helps limit the blast radius of anything going wrong.
There was a problem hiding this comment.
Could you remind me why we want to add this script to this GitHub repository and not the BR repo? I think it's probably on them to provide users an easy way to collect and export logs from the instance.
We provide this script for collecting cni logs to our customers. As of now, we couldn't use this script on Bottlerocket AMI, so needed this logic. Will check with @jayanthvn, if we can move this to BR repo instead
There was a problem hiding this comment.
Could we create a dedicated log collector file for BR? We currently separate them by OperatingSystem here so it might be helpful to create a dedicated file for BR rather than add if-else statements to the script meant for the AL2 AMI.
Also helps limit the blast radius of anything going wrong.
Thought having a same file would make the change agnostic to Customer's AMI. But agreed, having separate file would simplify the if-else logic. We will also need to update our documentation so that customers know which script to run for log collection on BR AMI. Will update this branch.
| @@ -0,0 +1,74 @@ | |||
| export LANG="C" | |||
There was a problem hiding this comment.
missing shebang? #!/bin/sh, !/bin/bash or !/usr/bin/env bash. and Copyrights?
https://github.com/awslabs/amazon-eks-ami/blob/master/log-collector-script/linux/eks-log-collector.sh#L1-L18
|
|
||
| # If "result" is less than or equal to "threshold", fail. | ||
| if [[ "${result}" -le "${threshold}" ]]; then | ||
| die "Free space on root volume is less than or equal to $((threshold>>10))MB, please ensure adequate disk space to collect and store the log files." |
There was a problem hiding this comment.
didn't see die and warning (Line 35) definition.
Try to run with control
[ssm-user@control]$ die
sh: die: command not found
and admin
[root@admin]# die
bash: die: command not found
or sheltie
[root@admin]# sudo sheltie die
nsenter: failed to execute die: No such file or directory
| if [ 0 -eq $? ]; then # Check if previous command was successful. | ||
| echo "${INSTANCE_ID}" | ||
| else | ||
| warning "Unable to find EC2 Instance Id. Skipped Instance Id." |
There was a problem hiding this comment.
same here. bash: warning: command not found
|
|
||
| collect_logs_bottlerocket() { | ||
| echo "Fetching INSTANCE_ID" | ||
| readonly INSTANCE_ID=$(curl --max-time 10 --retry 5 http://169.254.169.254/latest/meta-data/instance-id) |
There was a problem hiding this comment.
what if IMDSv2 enforced with SCP for Org. (or IAM policy enforced)?
|
|
||
| if [ ! -d "${BOTTLEROCKET_ROOTFS}/tmp/ekslogs" ]; then | ||
| echo "Creating ekslogs directory" | ||
| mkdir ${BOTTLEROCKET_ROOTFS}/tmp/ekslogs |
There was a problem hiding this comment.
Just wondering, why not execute with sudo sheltie mkdir /tmp/ekslogs?
| fi | ||
| done | ||
|
|
||
| cp ${BOTTLEROCKET_ROOTFS}/var/log/aws-routed-eni/* ${BOTTLEROCKET_ROOTFS}/tmp/ekslogs/ |
There was a problem hiding this comment.
I believe it could be simplified as follow,
[root@admin]# sudo sheltie mkdir /tmp/ekslogs
[root@admin]# sudo sheltie cp --recursive --verbose /var/log/aws-routed-eni/ /tmp/ekslogs/
[root@admin]# sudo sheltie ls -l /tmp/ekslogs/aws-routed-eni
total 64
-rw-r--r--. 1 root root 62674 Aug 6 00:48 ipamd.log
| cp ${BOTTLEROCKET_ROOTFS}/var/log/aws-routed-eni/* ${BOTTLEROCKET_ROOTFS}/tmp/ekslogs/ | ||
| sudo sheltie logdog | ||
| sudo sheltie cp /var/log/support/bottlerocket-logs.tar.gz /tmp/ekslogs | ||
| tar -cvzf "${LOG_DIR}"/eks_"${INSTANCE_ID}"_"${CURRENT_TIME}"_"${PROGRAM_VERSION}".tar.gz "${BOTTLEROCKET_ROOTFS}"/tmp/ekslogs > /dev/null 2>&1 |
There was a problem hiding this comment.
IMHO, it should be an optional action as we already have bottlerocket-logs.tar.gz under /tmp/ekslogs?
... and we could even more cleaner, with no tar installed and no need to declare BOTTLEROCKET_ROOTFS.
| cleanup() { | ||
| # bottlerocket AMI | ||
| if [ -d "/.bottlerocket/" ]; then | ||
| rm --recursive --force {BOTTLEROCKET_ROOTFS}/tmp/ekslogs > /dev/null 2>&1 | ||
| else | ||
| echo "Unable to Cleanup as {COLLECT_DIR} variable is modified. Please cleanup manually!" | ||
| fi | ||
| } |
There was a problem hiding this comment.
How about...
cleanup() {
# bottlerocket AMI
sudo sheltie rm --recursive --force --verbose /tmp/ekslogs # verbose might be useful for debug purpose?
}
Why we need a else... block here?
also, seeing no COLLECT_DIR defined and maybe need a \$ before {COLLECT_DIR}.
There was a problem hiding this comment.
Yes, I think PR was work in progress, we are anyway not merging in this repo.
@guessi thanks for your comments. Actually the changes will be included in Bottlerocket AMI itself over here: bottlerocket-os/bottlerocket#2137 and this PR will be used only as a reference. Converted it to draft to avoid confusion |
|
@cgchinmay well noted~ thanks for your speedy response. |
|
Closing this as it's been addressed in BR's repo. |
Issue #, if available:
VPC CNI - aws/amazon-vpc-cni-k8s#1316
Description of changes:
Added logic to collect logs on bottlerocket AMI
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.