Project

General

Profile

Actions

Feature #18772

closed

add https://github.com/awslabs/amazon-ebs-autoscale to compute image builder scripts

Added by Ward Vandewege 5 months ago. Updated 4 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
02/22/2022
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

Use the LVM support, not btrfs.


Subtasks 1 (0 open1 closed)

Task #18777: ReviewResolvedTom Clegg02/22/2022

Actions
Actions #1

Updated by Ward Vandewege 5 months ago

  • Description updated (diff)
Actions #3

Updated by Peter Amstutz 5 months ago

  • Assigned To set to Peter Amstutz
Actions #4

Updated by Peter Amstutz 5 months ago

  • Assigned To changed from Peter Amstutz to Ward Vandewege
Actions #5

Updated by Ward Vandewege 4 months ago

  • Status changed from New to In Progress

Ready for review at 5eef457a768ec4c8426eaa9fd252d77c0a2eddde on branch 18772-ebs-autoscale

Tests at developer-run-tests: #2926

Ready for review at f7988a9bc87718adf0519e4dd330859b29354826 on branch 18772-ebs-autoscale-2

Tests at developer-run-tests: #2927

Actions #6

Updated by Tom Clegg 4 months ago

In the create-ebs-volume patch, the relationship between $device and the new $nvme_device is mysterious.
  • If there's no available $device, we error out, but if there's no available $nvme device we continue with nvme_device=""
  • We call "attach-volume --device $device" but then we wait for $nvme_device to appear

My impression is that the amazon-provided script is already very fragile and I don't think we expect to fix that here, just hoping to avoid making it worse with our patch. It looks like the recommended way to find the device id for a known volume ID is lsblk -o +SERIAL. https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/nvme-ebs-volumes.html

It looks like usr-local-bin-ensure-encrypted-partitions-aws-ebs-autoscale.sh does not actually do encrypted partitions... if that's true, it seems like bad form to still call it "ensure encrypted partitions", although I see how that makes it convenient to install. Is this something worth mentioning in the docs, too?

IamInstanceProfile should be IAMInstanceProfile

Autoscaler bit in docs should mention you need to use an IAMInstanceProfile.

Actions #7

Updated by Ward Vandewege 4 months ago

Tom Clegg wrote:

My impression is that the amazon-provided script is already very fragile and I don't think we expect to fix that here, just hoping to avoid making it worse with our patch. It looks like the recommended way to find the device id for a known volume ID is lsblk -o +SERIAL. https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/nvme-ebs-volumes.html

Ah, yeah, thanks, that's a nicer way to do it, I've updated the patch.

In the create-ebs-volume patch, the relationship between $device and the new $nvme_device is mysterious.
  • If there's no available $device, we error out, but if there's no available $nvme device we continue with nvme_device=""
  • We call "attach-volume --device $device" but then we wait for $nvme_device to appear

These are moot now that I've refactored the patch.

It looks like usr-local-bin-ensure-encrypted-partitions-aws-ebs-autoscale.sh does not actually do encrypted partitions... if that's true, it seems like bad form to still call it "ensure encrypted partitions", although I see how that makes it convenient to install. Is this something worth mentioning in the docs, too?

Well, the volume that is attached is actually encrypted, but at the AWS level, because we use the default config for ebs-autoscale:

{
    "mountpoint": "/tmp",
    "filesystem": "lvm.ext4",
    "lvm": {
      "volume_group": "autoscale_vg",
      "logical_volume": "autoscale_lv" 
    },
    "volume": {
        "type": "gp3",
        "iops": 3000,
        "encrypted": 1
    },
    "detection_interval": 2,
    "limits": {
        "max_ebs_volume_size": 1500,
        "max_logical_volume_size": 8000,
        "max_ebs_volume_count": 16
    },
    "logging": {
        "log_file": "/var/log/ebs-autoscale.log",
        "log_interval": 300
    }
}

I've added that configuration to the documentation to make that clear.

IamInstanceProfile should be IAMInstanceProfile

Fixed, thanks. It is unfortunate that the AWS sdk calls it IamInstanceProfile.

Autoscaler bit in docs should mention you need to use an IAMInstanceProfile.

Yes! I've added that to the documentation, thanks.

Ready for another look at f7c83a41f84033e4ea9e570dd85e0152f0d81aab on branch 18772-ebs-autoscale-2

developer-run-tests: #2928

Actions #8

Updated by Tom Clegg 4 months ago

Just a couple of nitpicks:

It looks like we could do this and the "set +e" / "set -e" thing wouldn't be needed:

-        LSBLK=`lsblk -o +SERIAL |grep $valid_volume_id`
-        if [[ $? -eq 0 ]]; then
+        if LSBLK=`lsblk -o NAME,SERIAL |grep $valid_volume_id`; then

xargs seems kind of unnecessary here (although to be fair, polishing bash code doesn't always feel worthwhile):

-            nvme_device=`echo $LSBLK|cut -f1 -d' '|xargs -I {} echo "/dev/{}"`
+            nvme_device=/dev/`echo $LSBLK|cut -f1 -d' '`

LGTM, thanks!

Actions #9

Updated by Ward Vandewege 4 months ago

Tom Clegg wrote:

Just a couple of nitpicks:

It looks like we could do this and the "set +e" / "set -e" thing wouldn't be needed:
xargs seems kind of unnecessary here (although to be fair, polishing bash code doesn't always feel worthwhile):

Thanks, those are great improvements, merged with those included.

Actions #10

Updated by Ward Vandewege 4 months ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados-private:commit:arvados|e4a68851e521c0e3152f9790683d8cdb1b3923df.

Actions #11

Updated by Ward Vandewege 4 months ago

  • Release set to 49
Actions

Also available in: Atom PDF