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

kata: support large ConfigMaps #1023

Merged
merged 2 commits into from
Dec 13, 2024
Merged

kata: support large ConfigMaps #1023

merged 2 commits into from
Dec 13, 2024

Conversation

burgerdev
Copy link
Contributor

@burgerdev burgerdev commented Nov 21, 2024 •
edited
Loading

Small ConfigMaps are mounted watchable, but large ones are not. We do not really care enough about the exact mount source, since it's provided by the host anyway, so we allow mounting from watchable and non-watchable spaces.

@burgerdev burgerdev added the no changelog PRs not listed in the release notes label Nov 21, 2024
@burgerdev
Copy link
Contributor Author

Phase 1 done: test failed successfully: https://github.com/edgelesssys/contrast/actions/runs/11958600883/job/33338483638?pr=1023.

Now we need a fix.

@burgerdev burgerdev changed the title e2e: test ConfigMap mount kata: support large ConfigMaps Nov 22, 2024
@burgerdev burgerdev requested a review from 3u13r November 22, 2024 07:35
@burgerdev
Copy link
Contributor Author

Phase 2 done: test only fails for Redis.

@burgerdev burgerdev marked this pull request as ready for review November 22, 2024 19:00
3u13r
3u13r reviewed Nov 25, 2024
View reviewed changes
packages/by-name/kata/kata-runtime/package.nix Outdated
Comment on lines 98 to 99
# TODO(burgerdev): report upstream
# TODO(burgerdev): backport
Copy link
Member

Choose a reason for hiding this comment

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

I'd say that we at least have an upstream issue that we can link before we merge this, so that we have a reference in code when to remove the patch.

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'm still trying to reproduce this on upstream Kata. There may be a difference because of AB#4883, but it's unclear currently.

Copy link
Member

Choose a reason for hiding this comment

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

I've added some information to the ticket. Your hunch looks correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed privately, I'd prefer to merge this fix without waiting for the upstream discussion of paths to conclude. I added more info to the patch comment.

@burgerdev burgerdev added the needs: external unblock Blocked by an external cause label Nov 26, 2024
@burgerdev burgerdev removed the needs: external unblock Blocked by an external cause label Dec 12, 2024
@burgerdev burgerdev requested a review from 3u13r December 12, 2024 17:07
3u13r
3u13r approved these changes Dec 12, 2024
View reviewed changes
Copy link
Member

@3u13r 3u13r left a comment

Choose a reason for hiding this comment

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

LGTM

katexochen
katexochen approved these changes Dec 13, 2024
View reviewed changes
Copy link
Member

@katexochen katexochen left a comment

Choose a reason for hiding this comment

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

change lgtm, thanks for adding the test!

@katexochen katexochen added changelog PRs that should be part of the release notes and removed no changelog PRs not listed in the release notes labels Dec 13, 2024
@katexochen katexochen added this to the v1.2.0 milestone Dec 13, 2024
@burgerdev
Copy link
Contributor Author

Expected failures:

  • redis.yml on TDX due to known Nydus issues
  • peer-pods due to known breakage after image refactoring

@burgerdev burgerdev merged commit cb9f1bb into main Dec 13, 2024
16 of 18 checks passed
@burgerdev burgerdev deleted the burgerdev/configmap branch December 13, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katexochen katexochen katexochen approved these changes

@3u13r 3u13r 3u13r approved these changes

Assignees
No one assigned
Labels
changelog PRs that should be part of the release notes
Projects
None yet
Milestone
v1.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@burgerdev @3u13r @katexochen