Allow for additional disc_formats #374
Conversation
stephenmoloney
force-pushed
the
fix/disc_format
branch
from
cb19922
to
6a041c8
Compare
| @@ -5,6 +5,7 @@ kubeadm_token = "your-kubeadm-token" # Autogenerated kubeadm token (don't change | |||
| floating_ip_pool = "your-pool-name" | |||
| external_network_uuid = "external-net-uuid" # The uuid of the external network in the OpenStack tenancy | |||
| boot_image = "kubenow-v050" | |||
| disc_format = "raw" | |||
There was a problem hiding this comment.
I should probably change the default format to "qcow2" - for now it's raw while I'm testing.. but I can change this at the end to "qcow2"
- changed to qcow2
bin/openstack/functions.sh
Outdated
| image_size_number=$(echo "${image_size::-1}") | ||
|
|
||
| if [[ "${disc_format}" == "raw" ]]; then | ||
| min_disc=${image_size_number}; |
There was a problem hiding this comment.
I'm not sure if min_disc needs to be reset or not based on the new image size ?
I'm seeing issues with insufficient ram as the image is very big (20GB) but i'm not 100% sure yet. I'm investigating this further at the moment.
stephenmoloney
force-pushed
the
fix/disc_format
branch
2 times, most recently
from
03b9f28
to
ce35d29
Compare
stephenmoloney
changed the title
There was a problem hiding this comment.
Looks good so far! Just some minor comments. I really appreciate the effort you have made in refactorying the script with the helper functions. I suggest @andersla to review in detail, as he has mostly worked to this script.
| @@ -35,6 +35,7 @@ RUN apt-get update -y && apt-get install -y \ | |||
| libffi-dev \ | |||
| openssl \ | |||
| python-pip \ | |||
| qemu-utils \ | |||
There was a problem hiding this comment.
Could you check how much this affects the Docker image size?
There was a problem hiding this comment.
sure.. this is how it reads from docker image ls
kubenow/provisioners master 8b17915e9767 2 days ago 1.09GB
kubenow/provisioners 0.3.2 dc845cc33fea 7 days ago 1.02GB
bin/image-create-openstack.sh
Outdated
| set -e | ||
|
|
||
| source ./bin/openstack/functions.sh |
There was a problem hiding this comment.
Brilliant! We wanted to this a while ago, but we never had time. Maybe we could call the file ./bin/helpers/openstack.sh? In this way we could have similar refactory for the other import scripts in the future (btw, if you feel like refactorying the other providers as well, it would be much appreciated).
There was a problem hiding this comment.
Hey, I've changed the filename as suggested. I was thinking that it's a bit annoying having one provider use this pattern and the rest do not... but I don't have time to refactor the others for now anyways... thanks for the offer though ! 😃 :
| @@ -5,6 +5,7 @@ kubeadm_token = "your-kubeadm-token" # Autogenerated kubeadm token (don't change | |||
| floating_ip_pool = "your-pool-name" | |||
| external_network_uuid = "external-net-uuid" # The uuid of the external network in the OpenStack tenancy | |||
| boot_image = "kubenow-v050" | |||
| disc_format = "raw" | |||
There was a problem hiding this comment.
this should be commented, or defaulting to qcow
There was a problem hiding this comment.
yes, I will default it to "qcow2" .. changing.
stephenmoloney
force-pushed
the
fix/disc_format
branch
3 times, most recently
from
9346e55
to
5311bc6
Compare
stephenmoloney
changed the title
|
Note: I made some final changes this weekend. It's ready for review now. |
bin/helpers/openstack.sh
Outdated
|
|
||
| function get_host_uploaded_image_checksum() { | ||
| local image_name=$1 | ||
| local disc_format=$2 |
There was a problem hiding this comment.
To avoid confusion with image format on image downloaded change "disk_format" to "glance_disk_format"
There was a problem hiding this comment.
👍 - i did this for all disc_format - i presume that is what you mean ?
bin/helpers/openstack.sh
Outdated
|
|
||
| function verify_uploaded_image() { | ||
| local image_name=$1 | ||
| local disc_format=$2 |
There was a problem hiding this comment.
To avoid confusion with image format on image downloaded change "disk_format" to "glance_disk_format"
bin/helpers/openstack.sh
Outdated
|
|
||
| function upload_image() { | ||
| local image_name=$1 | ||
| local disc_format=$2 |
There was a problem hiding this comment.
To avoid confusion with image format on image downloaded change "disk_format" to "glance_disk_format"
bin/helpers/openstack.sh
Outdated
|
|
||
| function post_image_upload_checks() { | ||
| local image_name=$1 | ||
| local disc_format=$2 |
There was a problem hiding this comment.
To avoid confusion with image format on image downloaded change "disk_format" to "glance_disk_format"
bin/helpers/openstack.sh
Outdated
|
|
||
| function maybe_convert_image(){ | ||
| local image_name=$1 | ||
| local disc_format=$2 |
There was a problem hiding this comment.
To avoid confusion with image format on image downloaded change "disk_format" to "glance_disk_format"
bin/image-create-openstack.sh
Outdated
| awk -F "|" '{print $3;}' | | ||
| tr -d '[:space:]')" | ||
| done | ||
| maybe_download_image "${image_name}" "${disc_format}" "${bucket_url}" |
There was a problem hiding this comment.
first check if image is present on openstack - if it is there skip everything (no need to download image to local host if it is on server already)
There was a problem hiding this comment.
good catch - i've made this change as suggested.
bin/helpers/openstack.sh
Outdated
| echo "Starting the image conversion from qcow2 to ${disc_format}..." | ||
| echo -e "\n" | ||
|
|
||
| qemu-img convert -q -f qcow2 -O "${disc_format}" "${download_filepath}" "${upload_filepath}" |
There was a problem hiding this comment.
add echo "Image convertion complete"
There was a problem hiding this comment.
added something quite similar echo "${image_name}.qcow2 conversion to ${upload_filepath} complete"
bin/helpers/openstack.sh
Outdated
| qemu-img convert -q -f qcow2 -O "${disc_format}" "${download_filepath}" "${upload_filepath}" | ||
| fi | ||
| else | ||
| echo "${upload_filepath} already exists and so does not require conversion, moving onto upload step" |
There was a problem hiding this comment.
delete ", moving onto upload step
(We don't really know what function will be called after this one)
There was a problem hiding this comment.
thanks. very much agree. changed.
bin/image-create-openstack.sh
Outdated
| tr -d '[:space:]')" | ||
| done | ||
| maybe_download_image "${image_name}" "${disc_format}" "${bucket_url}" | ||
| maybe_convert_image "${image_name}" "${disc_format}" "${bucket_url}" |
There was a problem hiding this comment.
skip call to "maybe_convert_image" in "image-create-openstack" since it will be called in "maybe_upload_image" anyway
There was a problem hiding this comment.
I've removed maybe_upload_image now ... because now the function is wrapped in a conditional is_image_already_uploaded - make sense?
bin/helpers/openstack.sh
Outdated
| if [[ "${is_image_uploaded}" == "false" ]]; then | ||
| maybe_convert_image "${image_name}" "${disc_format}" "${image_url}" | ||
| upload_image "${image_name}" "${disc_format}" "${image_url}" | ||
| else |
There was a problem hiding this comment.
only call "post_image_upload_checks" from function "upload_image" (don't call if image is uploaded already previously - checksum could be different since same version image can have different checksums due to security updates)
(remove else block)
There was a problem hiding this comment.
-
this function is removed bu the logic for it has moved to
image-create-openstack.sh. -
I've moved
post_image_upload_checksinto upload image. -
If we remove the else block then this line
echo "${image_name}.${glance_disc_format} is already present - no need to upload"also gets removed - perhaps it's better to keep this as it signals to the user that the entire process for uploading the openstack image will be bypassed ?
|
Hi Anders, thanks for the review, going to read the review soon... |
stephenmoloney
force-pushed
the
fix/disc_format
branch
8 times, most recently
from
dc33a7e
to
e7459f0
Compare
|
@andersla can you review again, I made most of.the suggested changes. Thanks |
stephenmoloney
force-pushed
the
fix/disc_format
branch
2 times, most recently
from
2d805ee
to
dc70951
Compare
stephenmoloney
force-pushed
the
fix/disc_format
branch
from
dc70951
to
6571e5c
Compare
|
Hi Stephen, I haven't forgotten you - I am just in a lot of other work right now:-) |
Motivation
Description
Style