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

update multus chart to add optional dhcp daemonset #5146

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

thomasferrandiz
Copy link
Contributor

Linked issue: #3917

@thomasferrandiz thomasferrandiz requested a review from a team as a code owner December 20, 2023 13:07
@@ -20,7 +20,7 @@ charts:
- version: 2.11.100-build2023051511
filename: /charts/rke2-metrics-server.yaml
bootstrap: false
- version: v4.0.2-build2023081103
- version: v4.0.2-build2023081104
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any new images that come with the changes to the chart, or does the DHCP daemonset use an existing image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no new images, it's using busybox

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we should include busybox in the list of images for air-gap

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we for some reason specifically excluded the busybox image from the chart image validation scripts. If we're going to use busybox, we probably shouldn't exclude it from the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I excluded busybox from the validation script because it's used in several charts but is never added in build-images so I assumed this was done on purpose.
Also several charts use the latest busybox instead of set version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

busybox added to build-images

@@ -83,6 +83,7 @@ xargs -n1 -t docker image pull --quiet << EOF > build/images-multus.txt
${REGISTRY}/rancher/hardened-sriov-network-resources-injector:v1.5-build20231009
${REGISTRY}/rancher/hardened-sriov-network-webhook:v1.2.0-build20230607
${REGISTRY}/rancher/hardened-whereabouts:v0.6.2-build20231009
${REGISTRY}/library/busybox:latest
Copy link
Contributor

@brandond brandond Dec 22, 2023

Choose a reason for hiding this comment

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

We can't use the latest tag, there are different semantics for pulling when that is used, and it is not suitable for airgap use. We need to use a fixed tag. It sounds like this might require another change to the chart though?

Also, everything needs to come from ${REGISTRY}/rancher/, we can't consume things outside that project. I suggest you pick one of the mirrored busybox tags:
https://github.com/rancher/image-mirror/blob/master/images-list#L461

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the chart to use the mirrored busybox from rancher

@thomasferrandiz thomasferrandiz merged commit 9674104 into rancher:master Jan 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants