Conversation
bobbyiliev
force-pushed
the
add-lgalloc-support
branch
from
94ca04c to
009912f
Compare
bobbyiliev
force-pushed
the
add-lgalloc-support
branch
2 times, most recently
from
f95c785 to
c93cd38
Compare
bobbyiliev
force-pushed
the
add-lgalloc-support
branch
from
c93cd38 to
f04c0eb
Compare
| # Disk support configuration | ||
| disk_config = { | ||
| install_openebs = var.enable_disk_support ? lookup(var.disk_support_config, "install_openebs", true) : false | ||
| run_disk_setup_script = var.enable_disk_support ? lookup(var.disk_support_config, "run_disk_setup_script", true) : false |
There was a problem hiding this comment.
Could we just base this one entirely on var.enable_disk_support and not have this extra var?
There was a problem hiding this comment.
Yes, the idea was that users only set enable_disk_support, and the rest use defaults. I added disk_support_config just for extra flexibility if we ever need to override things. Happy to simplify if we want to keep it more opinionated though, up to you!
There was a problem hiding this comment.
This part of the PR is more or less a copy and paste from the AWS implementation.
There was a problem hiding this comment.
ok.... let's just keep this
| # Install required tools | ||
| if command -v apt-get >/dev/null 2>&1; then | ||
| apt-get update | ||
| apt-get install -y lvm2 | ||
| elif command -v yum >/dev/null 2>&1; then | ||
| yum install -y lvm2 | ||
| else | ||
| echo "No package manager found. Please install required tools manually." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
We should definitely version lock this, or if there aren't dependencies, install from a binary.
There was a problem hiding this comment.
Just pinned this to a specific version.
Regarding the dependencies:
Reading package lists...
+ apt-get install -y lvm2=2.03.11-2.1
Reading package lists...
Building dependency tree...
Reading state information...
The following additional packages will be installed:
dmeventd dmsetup libaio1 libbsd0 libdevmapper-event1.02.1 libdevmapper1.02.1
libedit2 libexpat1 liblvm2cmd2.03 libmd0 thin-provisioning-tools
There was a problem hiding this comment.
This is extremely unlikely to work long-term. If you want to pin it, you need to bake it into an image. The upstream repos will likely not contain that specific version for long.
There was a problem hiding this comment.
Sure, but I thought that we did not want to maintain our own image because of security concerns?
There was a problem hiding this comment.
Well, we can't pin the version this way, and the security concerns don't go away just because you're using someone else's image.
We can either:
- Unpin both the lvm2 package version and use a moving target debian image tag.
- Maintain our own image and use dependabot (or similar) to keep the image up to date.
There was a problem hiding this comment.
Just removed the version pin.
When I was originally working on that custom container, I got pushback because of concerns that a lot of vulnerability scanner noise could come from bootstrap containers.
But if we are all fine with this, I am happy to work on that bootstrap Docker image.
bobbyiliev
force-pushed
the
add-lgalloc-support
branch
from
cb553bc to
8f767b2
Compare
| resources { | ||
| limits = { | ||
| cpu = "200m" | ||
| memory = "256Mi" | ||
| } | ||
| requests = { | ||
| cpu = "100m" | ||
| memory = "128Mi" | ||
| } | ||
| } |
There was a problem hiding this comment.
Memory requests and limits should always be equal.
CPU limits are generally frowned upon, as CPU can be returned extremely quickly. In the case of this bootstrap script, it will likely be idle for most of the time, so limiting CPU is likely not helpful.
|
|
||
| init_container { | ||
| name = "disk-setup" | ||
| image = "debian:bullseye-20250407-slim" |
There was a problem hiding this comment.
We should probably bake LVM into an image, or find one that comes with it.
We may be able to simplify this module quite a bit if we also bake the bootstrap script into the image.
| memory = "128Mi" | ||
| } | ||
| requests = { | ||
| cpu = "100m" |
There was a problem hiding this comment.
We still want at least some cpu request.
| @@ -221,10 +221,6 @@ variable "disk_support_config" { | |||
| openebs_namespace = optional(string, "openebs") | |||
| storage_class_name = optional(string, "openebs-lvm-instance-store-ext4") | |||
| storage_class_provisioner = optional(string, "local.csi.openebs.io") | |||
There was a problem hiding this comment.
The storage_class_provisioner should be constant.
alex-hunt-materialize
left a comment
alex-hunt-materialize
left a comment
There was a problem hiding this comment.
We should probably bake our own image. Some customers may block outbound access to package repos, which will prevent installation of LVM.
3 participants
Open to suggestions around the bootstrap script and the daemonset!