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

Jump to bottom

helm: Replace wait-for-it with a busybox script #24959

Merged
merged 2 commits into from May 2, 2023
Merged

helm: Replace wait-for-it with a busybox script #24959

merged 2 commits into from May 2, 2023

Conversation

meyskens
Copy link
Member

Builds on #24765

The wait-for-it image has problems in supporting some CPUs.
The image has a very simple functionality that is only needed to
avoid the spire agent to go into a crash loop.
This we recreated in busybox an image already present in our
setup for spire. This also avoids adding new dependencies.

Fixes: #24897

Replace wait-for-it in SPIRE setup with a busybox script

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 18, 2023
@sayboras sayboras added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 19, 2023
@sayboras
Copy link
Member

last commit is really nice, I think you can rebase and mark for review :D

@meyskens meyskens marked this pull request as ready for review April 19, 2023 13:53
@meyskens meyskens requested review from a team as code owners April 19, 2023 13:53
@meyskens
Copy link
Member Author

/test

@meyskens meyskens added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. release-note/misc This PR makes changes that have no direct user impact. and removed dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. labels Apr 19, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Apr 19, 2023
sayboras
sayboras approved these changes Apr 20, 2023
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

While the changes in this PR look simple, there are a lot of thinking and knowledge embedded on the why. Thanks for always helping me 🦃 🎖️

@sayboras
Copy link
Member

/test

@meyskens
Copy link
Member Author

Test seems to fail on a non related issue

 	 The Service "echo-b" is invalid: spec.ports[0].nodePort: Invalid value: 31414: provided port is already allocated

kaworu
kaworu approved these changes Apr 21, 2023
View reviewed changes
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @meyskens!

Test seems to fail on a non related issue

 	 The Service "echo-b" is invalid: spec.ports[0].nodePort: Invalid value: 31414: provided port is already allocated

Please take the time to search for a ci/flake issue so we can track how many time we hit them. Here CI hit #13071.

install/kubernetes/cilium/templates/spire/agent/daemonset.yaml Show resolved Hide resolved
@kaworu
Copy link
Member

kaworu commented Apr 21, 2023

/test-1.26-4.19

@meyskens meyskens requested a review from a team as a code owner April 24, 2023 07:35
@meyskens meyskens requested a review from bimmlerd April 24, 2023 07:35
kaworu
kaworu reviewed Apr 24, 2023
View reviewed changes
.github/renovate.json5 Outdated Show resolved Hide resolved
bimmlerd
bimmlerd reviewed Apr 24, 2023
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

Let's find a way to be loud about the initContainer change - I think at least we need to make it more explicit in the release note.

install/kubernetes/cilium/templates/spire/agent/daemonset.yaml Show resolved Hide resolved
.github/renovate.json5 Outdated Show resolved Hide resolved
@aanm aanm removed the release-note/misc This PR makes changes that have no direct user impact. label Apr 24, 2023
@meyskens meyskens requested a review from a team as a code owner April 24, 2023 14:15
@maintainer-s-little-helper
Copy link

Commit d36a898c028e7d9c4c1052e3068c28c773b377e6 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 24, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 24, 2023
@meyskens
Copy link
Member Author

Sorry for the last push, did a commit on the wrong branch

@meyskens
Copy link
Member Author

/test

@meyskens
Copy link
Member Author

/test

@meyskens
Copy link
Member Author

/test-k8s-1.27-kernel-net-next

(build provision failed)

@meyskens
Copy link
Member Author

/test-k8s-1.27-kernel-net-next

@meyskens
Copy link
Member Author

/test

@meyskens
Copy link
Member Author

Rebased for 1.27 revert on main

@sayboras sayboras added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 29, 2023
@meyskens
Replace wait-for-it with a busybox script
743ffef 
The wait-for-it image has problems in supporting some CPUs.
The image has a very simple functionality that is only needed to
avoid the spire agent to go into a crash loop.
This we recreated in busybox an image already present in our
setup for spire. This also avoids adding new dependencies.

Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>
@meyskens
Add renovate rules to update busybox in spire
0a35855 
Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>
@meyskens meyskens removed the request for review from a team May 2, 2023 06:59
@meyskens meyskens removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label May 2, 2023
@aanm
Copy link
Member

aanm commented May 2, 2023

/test

@sayboras
Copy link
Member

sayboras commented May 2, 2023

Changes are verified manually, the required reviews are in, full CI is not required as we don't have any tests in Jenkins related to spiffee now, marking this ready to merge.

@sayboras sayboras added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 2, 2023
@sayboras sayboras changed the title Replace wait-for-it with a busybox script helm: Replace wait-for-it with a busybox script May 2, 2023
@sayboras sayboras added area/helm Impacts helm charts and user deployment experience area/servicemesh GH issues or PRs regarding servicemesh labels May 2, 2023
@meyskens
Copy link
Member Author

meyskens commented May 2, 2023

runtime Ci failure hit #25178

@joestringer joestringer merged commit d3fc228 into cilium:main May 2, 2023
55 of 56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaworu kaworu kaworu approved these changes

@joamaki joamaki joamaki approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@sayboras sayboras sayboras approved these changes

Assignees
No one assigned
Labels
area/helm Impacts helm charts and user deployment experience area/servicemesh GH issues or PRs regarding servicemesh ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SPIRE initContainer with wait-for-it image needs to be updated for more CPU arches
7 participants
@meyskens @sayboras @kaworu @aanm @joamaki @bimmlerd @joestringer