-
Notifications
You must be signed in to change notification settings - Fork 7
node-images: enable copr repo #93
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
Open
HarshwardhanPatil07
wants to merge
1
commit into
bootc-dev:main
Choose a base branch
from
HarshwardhanPatil07:copr-bootc-composefs-fix
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+2
−0
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do understand why you directly add here. However, I'm reluctant to include this by default. Possibly, what we could is to add a new flavor of the disk image for composefs and this fix for the time it lands in fedora.
We could introduce a new argument that if sets will enable the copr repository and then we add a new GHA that will build the composefs disk image including bootc with this fix. This could helps us to eventually add the reverse dependencies job and test. See bootc-dev/bootc-operator#70
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interested in your hesitation to just unconditionally add this. Yes, we're special-casing bootc here, but... bootc is special for us so it seems warranted.
Note also that this only affects the buildroot; the COPR repo is not enabled by default in the final node image built.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are overwriting the default bootc from fedora. So, does it mean we want always to rely from one of the latest bootc version instead of the default one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, yes.
I think what's muddling the waters here is the fact that the node image definition lives in this repo even though bink itself is designed to be decoupled from the actual node image. It makes sense for bink to have a "reference default image", but the main consumer of bink and that node image today is the bootc-operator. And there I think it makes sense for CI to always test against latest bootc. So that in-tree node image definition plays double-duty.
I wouldn't be against having a separate set of node images specifically tailored for bootc-operator (which could e.g. live in that repo instead), but I don't think we're at that point yet where it's worth maintaining separate sets.
So my strawman is to do this and accommodate the bootc-operator use case. If we keep mutating the node image for bootc-operator's goals in a way that differs from what we think the reference default bink node image should be, we can always pay that cost later. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A deeper thing going on here is that it's not clear to me what that "reference node image" should even prioritize/value. The main goal of bink today is to easily provide a K8s cluster based on real bootc systems.
There's already so many mini-K8s tools out there that exist for the different gradations of how real enough you want it to be. And we're not trying to compete with all those for the "general easy K8s" app development flow. Worth noting also that minikube supports VMs (and perhaps one angle long-term is to try to somehow add a new e.g. bcvk backend there...). So "real bootc systems" is really the main differentiator here.
And therefore, the choice of such a reference image tracking latest bootc doesn't seem obviously wrong to me. At least not yet. (I can imagine bink users that would value slower bootc to be clear; this is more about what the default should be.)
Definitely interested in your take on this! But anyway, my main take overall is that this is something we can easily tweak as we discover more where we want it to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My major concern is that every build could pull a different bootc version which might break the CI. Hence, a change in bootc might let the CI failing on new code changes, but that's not th fault of that particular PR. It introduces a new variant.
I'm not against building bink images with latest bootc, but I don't want to put it as bink default. Having said that, we can always set the image in bootc-operator to point to the latest bootc images. Or have a job that test both the stable one in fedora and the latest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a valid concern, but it's already something we face today. There's lots of other packages in the current image that may break us on any update (and in fact did).
Definitely not opposed to adding some locking + PR bump workflow if we'd like (I think an easy way to do this is probably to use the versioned Fedora compose URLs and Renovate to bump it, similar to what e.g. is done for some of the bootc images building in Konflux today -- though that wouldn't work as easily for the bootc COPR). It's always a matter of tradeoffs with these things.
I'm also not opposed to blocking this PR on doing that first. But at the same time, because it's bootc, and we're a part of that project now, I think any issues we hit will in practice be dealt with quite quickly (either by other bootc members or us! :)). And if not, we can always freeze if needed.