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.

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

Add 'make airgap-package' target and associated scripts for bundling images for air gap #475

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

squizzi
Copy link

@squizzi squizzi commented Oct 11, 2024

This adds new scripts for creating a bundle for airgap customers. We can easily add a step to the release CI as well, but the resulting tarball of saved images is 4.2G so I decided to not include that for now since we'll need to figure out the best place to host the tarball.

At this time this script deploys a management cluster, waits for all the providers to come up and be ready then gets a list of all the images used and pulls/saves those.

It then iterates helm template ... output across each of the k0s extensions we deploy and pulls/saves images affiliated with the extensions. I've also updated helm-package to pull in these extensions during it's packaging process.

$ IMG=ghcr.io/mirantis/hmc/controller-ci:v0.0.1-287-gd9c38c3 make bundle-images
...
Done! Images bundled into /home/squizzi/go/src/github.com/Mirantis/hmc/bin/images/hmc-images-v0.0.2-14-gf02c79b.tgz

$ docker load < ./bin/images/hmc-images-v0.0.2-14-gf02c79b.tgz
...
49dc5e73a32a: Loading layer [==================================================>]  70.34MB/70.34MB
Loaded image: mcr.microsoft.com/oss/kubernetes/azure-cloud-node-manager:v1.30.4
c2453148319b: Loading layer [==================================================>]  85.39MB/85.39MB
Loaded image: quay.io/k0sproject/k0smotron:v1.0.4
4a2377d5d202: Loading layer [==================================================>]   1.69MB/1.69MB
d9486140fcec: Loading layer [==================================================>]  76.06MB/76.06MB
Loaded image: registry.k8s.io/provider-aws/cloud-controller-manager:v1.30.3
a7c367db8420: Loading layer [==================================================>]  61.15MB/61.15MB
Loaded image: registry.k8s.io/sig-storage/csi-snapshotter:v7.0.2
e4d8c7836c02: Loading layer [==================================================>]  73.34MB/73.34MB
...

Closes #423

@squizzi squizzi marked this pull request as draft October 11, 2024 00:10
@squizzi squizzi changed the title Add 'make bundle-images' target and associated script for bundling images for air gap Add 'make airgap-package' target and associated scripts for bundling images for air gap Oct 11, 2024
@squizzi squizzi marked this pull request as ready for review October 15, 2024 09:50
@squizzi squizzi force-pushed the airgapped-bundle branch 3 times, most recently from 93428f8 to 36e5228 Compare October 22, 2024 23:02
Copy link
Contributor

@a13x5 a13x5 left a comment

Choose a reason for hiding this comment

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

Several issues with make targets
The scripts should be revisited.

One general important note regarding bash syntax:

There is a convention that local scope variable names should be always lower snake case. Uppercase names are usually reserved only for env variables (in rare cases for global ones in scope of script)
Ditching this rule makes script harder to read and reason about. Please fix

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
scripts/airgap-push.sh Outdated Show resolved Hide resolved
scripts/bundle-images.sh Outdated Show resolved Hide resolved
scripts/bundle-images.sh Outdated Show resolved Hide resolved
scripts/bundle-images.sh Outdated Show resolved Hide resolved
@squizzi squizzi force-pushed the airgapped-bundle branch 2 times, most recently from d41d942 to 11bc596 Compare October 31, 2024 19:59
@squizzi
Copy link
Author

squizzi commented Oct 31, 2024

EXPORT_ALL_VARIABLES does not appear to play nicely with other Makefile targets and export does not appear to actually populate the variables so I unfortunately can't seem to make that work without just populating the vars prior to script execution. Not sure the best way to do this.

@squizzi squizzi requested review from a13x5 and s3rj1k October 31, 2024 22:42
@squizzi squizzi force-pushed the airgapped-bundle branch 2 times, most recently from 128ad6d to d2bda51 Compare November 1, 2024 15:49
Copy link
Contributor

@a13x5 a13x5 left a comment

Choose a reason for hiding this comment

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

EXPORT_ALL_VARIABLES does not appear to play nicely with other Makefile targets and export does not appear to actually populate the variables

export should work at least with GNU Make 4.4 (especially if .EXPORT_ALL_VARIABLES works) (see manual)

Anyway if it's became too complex I will not block this PR. Issues with Makefile will be fixed in scope of #589

A couple of small issues should be fixed in the script however (Note that NIT comment are optional to fix)

scripts/bundle-images.sh Outdated Show resolved Hide resolved
scripts/bundle-images.sh Outdated Show resolved Hide resolved
@squizzi squizzi requested a review from a13x5 November 1, 2024 22:41
@squizzi
Copy link
Author

squizzi commented Nov 1, 2024

Anyway if it's became too complex I will not block this PR. Issues with Makefile will be fixed in scope of #589

Ya, I'm not sure what's going on, I've configured the exports just as the docs suggest with:

export var = value

and they end up empty in the script invocation, debugged with set -x.

@squizzi squizzi force-pushed the airgapped-bundle branch 2 times, most recently from ecd1adf to 9da196e Compare November 1, 2024 22:44
@squizzi
Copy link
Author

squizzi commented Nov 1, 2024

I think I need to bring back the custom_tag logic, for example kube-vip when templated with our values produces the following error:

helm template --repo https://kube-vip.github.io/helm-charts kube-vip -f values.yaml
Error: execution error at (kube-vip/templates/daemonset.yaml:33:22): A valid config.address required!

It's because we're trying to remove the config.address entry prior to templatizing it. Sure we could work around this instance with something like --set config.address "12345" but do we potentially want to avoid custom logic like this? The custom_tag logic, although a little quirky didn't have to deal with these potential custom validations that chart maintainers may add since it templates the chart with default values.

@squizzi squizzi force-pushed the airgapped-bundle branch 2 times, most recently from 02ef954 to b154696 Compare November 4, 2024 20:04
@squizzi
Copy link
Author

squizzi commented Nov 4, 2024

Found a bug in the airgap-push.sh script, it wasn't dealing with multiple tags of images well, that's been fixed.

Copy link
Contributor

@a13x5 a13x5 left a comment

Choose a reason for hiding this comment

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

I tested the scripts on the airgapped ubuntu machine.
While testing I had multiple issues with airgapped k0s (some irrelevant to this PR).
I added several comments related to the issues I experienced.

Regarding the kube-vip issue. It's caused by the empty value we're providing via values, so it's not related to kube-vip chart itself. Thus I suggest adding a special case for it either adding --debug (so it will output some yaml) or by removing empty value from the values we're providing.
custom_tag logic also could be considered, but IMO it looks more flaky.

scripts/bundle-images.sh Outdated Show resolved Hide resolved
scripts/airgap-push.sh Show resolved Hide resolved
@squizzi
Copy link
Author

squizzi commented Nov 5, 2024

Regarding the kube-vip issue. It's caused by the empty value we're providing via values, so it's not related to kube-vip chart itself. Thus I suggest adding a special case for it either adding --debug (so it will output some yaml) or by removing empty value from the values we're providing.

Outputting --debug if the helm template fails seems to do the trick for this instance.

Copy link
Contributor

@a13x5 a13x5 left a comment

Choose a reason for hiding this comment

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

Issue with hmc-controller image
With --insecure-skip-tls-verify flag we're not covering http registries, but it's out of scope of this PR and should be covered in the docs.

scripts/bundle-images.sh Outdated Show resolved Hide resolved
@squizzi
Copy link
Author

squizzi commented Nov 7, 2024

Looks like kube-vip still didn't want to render out, for now I've just added some logic to workaround this case, I kept the --debug logic in there though for other instances where that may be viable.

@squizzi squizzi closed this Nov 7, 2024
@squizzi squizzi reopened this Nov 7, 2024
@squizzi squizzi requested a review from a13x5 November 7, 2024 21:53
* Pull charts referenced by providers k0s.extensions during helm-package

  In order to support airgap customers will need to place all of the charts
  a ManagedCluster may pull upon into a registry.  This adds support to our
  'helm-package' Makefile to ensure we package up all of the charts our templates
  reference so that they can be included in an airgap bundle.

* Place k0s.extensions images in a seperate image tarball

  For the first iteration of airgap we're using k0s airgap
  bundle for the management cluster but the ManagedClusters we
  deploy will still need the extensions images that the extension
  charts reference.

  This patch modifies the bundle-images target so it creates two
  bundles, one that will serve as the management cluster bundle
  and one that will serve as the ManagedCluster bundle.

* Add airgap-push script

  This script can be used to push images and Helm charts needed for an
  airgapped ManagedCluster deployment to a given image registry and chart
  repository.

* Package the airgap-push script with the bundle

Signed-off-by: Kyle Squizzato <[email protected]>
@squizzi
Copy link
Author

squizzi commented Nov 7, 2024

I've squashed the changes down to prepare for merge/carrying of this PR since this is my last day at Mirantis and I'll be taking some time off for a bit.

Copy link
Contributor

@a13x5 a13x5 left a comment

Choose a reason for hiding this comment

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

LGTM

@a13x5 a13x5 merged commit 70419b1 into Mirantis:main Nov 7, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Determine best way to bundle and utilize charts and images for airgapped customers
3 participants