Skip to content
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.

Sign up for GitHub

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

feat: simplify and generalise get_image() #405

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: simplify and generalise get_image() #405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f50c228
feat: simplify and generalise get_image()
stuartgrace-bbc Jul 1, 2024
94a3018
feat: can supply a manifest file to image_loader
stuartgrace-bbc Jul 2, 2024
a2cb02c
fix: correct image names in repo
stuartgrace-bbc Jul 9, 2024
c4b76e1
fix: image_loader arg renamed to --images-manifest
stuartgrace-bbc Aug 14, 2024
31874a9
Merge branch 'vexxhost:main' into simplify-get-image
stuartgrace-bbc Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 24 additions & 13 deletions magnum_cluster_api/cmd/image_loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import shutil
import subprocess
import tempfile
import yaml

import click
import platformdirs
Expand Down Expand Up @@ -57,6 +58,10 @@
required=True,
help="Target image repository",
)
@click.option(
"--images-manifest",
help="YAML file containing image manifest",
)
@click.option(
"--parallel",
default=8,
Expand All @@ -67,7 +72,7 @@
is_flag=True,
help="Allow insecure connections to the registry.",
)
def main(repository, parallel, insecure):
def main(repository, images_manifest, parallel, insecure):
"""
Load images into a remote registry for `container_infra_prefix` usage.
"""
Expand All @@ -79,18 +84,24 @@ def main(repository, parallel, insecure):
https://github.com/google/go-containerregistry/blob/main/cmd/crane/README.md#installation"""
)

# NOTE(mnaser): This list must be maintained manually because the image
# registry must be able to support a few different versions
# of Kubernetes since it is possible to have multiple
# clusters running different versions of Kubernetes at the
# same time.
images = set(
_get_all_kubeadm_images()
+ _get_calico_images()
+ _get_cilium_images()
+ _get_cloud_provider_images()
+ _get_infra_images()
)
if images_manifest:
with open(images_manifest, 'r') as file:
manifest_data = yaml.safe_load(file)

images = set(manifest_data['images'])

else:
# NOTE(mnaser): This list must be maintained manually because the image
# registry must be able to support a few different versions
# of Kubernetes since it is possible to have multiple
# clusters running different versions of Kubernetes at the
# same time.
images = set(
_get_all_kubeadm_images()
+ _get_calico_images()
+ _get_cloud_provider_images()
+ _get_infra_images()
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of keeping this code, could we supply a default manifest that's equivalent so people don't have to look into two different places (code and manifest) to figure this out?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would extend the scope of this PR as we would have to keep the default manifest in the source tree, manage its installation to a known location and then load it by default. I am conscious that we are making very little progress toward getting this merged so would like to keep the scope contained.

An extracted default manifest would be missing the images found through running kubeadm so I think that this might need more consideration, and should probably be a follow-up PR if anyone were wanting to do that work.


with concurrent.futures.ThreadPoolExecutor(max_workers=parallel) as executor:
future_to_image = {
Expand Down
30 changes: 5 additions & 25 deletions magnum_cluster_api/image_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,31 +56,11 @@ def get_image(name: str, repository: str = None):
if not repository:
return name

new_image_name = name
if name.startswith("docker.io/calico"):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are all of these hardcoded rules still needed here? I'm thinking they should error out if they are specified incorrectly.

Copy link
Contributor

@jrosser jrosser Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR removes all of the hardcoded rules that are not a simple replacement. The remaining ones do not conform to that pattern and as I understand it need to remain for compatibility with the registry/repository structure expected elsewhere in atmosphere(?). @mnaser may be able to give some more context on why things are like this.

new_image_name = name.replace("docker.io/calico/", f"{repository}/calico/")
return name.replace("docker.io/calico/", f"{repository}/calico/")
if name.startswith("quay.io/cilium"):
new_image_name = name.replace("quay.io/cilium/", f"{repository}/cilium/")
if name.startswith("docker.io/k8scloudprovider"):
new_image_name = name.replace("docker.io/k8scloudprovider", repository)
if name.startswith("registry.k8s.io/sig-storage"):
new_image_name = name.replace("registry.k8s.io/sig-storage", repository)
if name.startswith("registry.k8s.io/provider-os"):
new_image_name = name.replace("registry.k8s.io/provider-os", repository)
if name.startswith("gcr.io/k8s-staging-sig-storage"):
new_image_name = name.replace("gcr.io/k8s-staging-sig-storage", repository)
if new_image_name.startswith(f"{repository}/livenessprobe"):
return new_image_name.replace("livenessprobe", "csi-livenessprobe")
if new_image_name.startswith("registry.k8s.io/coredns"):
return new_image_name.replace("registry.k8s.io/coredns", repository)
if new_image_name.startswith("registry.k8s.io/autoscaling"):
return new_image_name.replace("registry.k8s.io/autoscaling", repository)
if (
new_image_name.startswith("registry.k8s.io/etcd")
or new_image_name.startswith("registry.k8s.io/kube-")
or new_image_name.startswith("registry.k8s.io/pause")
):
return new_image_name.replace("registry.k8s.io", repository)
return name.replace("quay.io/cilium/", f"{repository}/cilium/")
if name.startswith(f"{repository}/livenessprobe"):
return name.replace("livenessprobe", "csi-livenessprobe")

assert new_image_name.startswith(repository) is True
return new_image_name
return repository + "/" + name.split("/")[-1]