feat: update Firecracker client to v1.14#2415
Conversation
Bump the swagger file for Firecracker to v1.14 and regenerate APIs/models. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Add support for Firecracker v1.14 and make it the default. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit 49ad274. Bugbot is set up for automated code reviews on this repo. Configure here. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d497f3ce7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
packages/shared/pkg/grpc/template-manager/template-manager.pb.go:699-705— The FreePageReporting field (*bool, field 17) was added directly to the generated template-manager.pb.go without updating the source template-manager.proto. If pb.go is ever regenerated from the proto source via protoc, this field will be silently dropped, breaking all callers that depend on it. Fix by adding optional bool freePageReporting = 17; to packages/orchestrator/template-manager.proto and regenerating.Extended reasoning...
What the bug is and how it manifests
The PR adds FreePageReporting *bool (protobuf field 17) to the TemplateConfig struct in packages/shared/pkg/grpc/template-manager/template-manager.pb.go (lines 699-705). However, the source packages/orchestrator/template-manager.proto file was never updated — its TemplateConfig message still ends at field 16 (teamID). The .pb.go carries the canonical header "// Code generated by protoc-gen-go. DO NOT EDIT. source: template-manager.proto", indicating it is supposed to be machine-generated from the proto, not hand-edited.
The specific code path that triggers it
The struct field was added manually at line 702 with tag varint,17,opt,name=freePageReporting,proto3,oneof. The binary raw descriptor (file_template_manager_proto_rawDesc) was also modified in the diff (the length prefix byte changed from 0x84,0x05 to 0xcd,0x05), confirming the pb.go was regenerated from a modified proto that was never committed to the repository.
Why existing code does not prevent it
There is no CI check enforcing that pb.go is in sync with its source .proto. The proto file is not included in the PR diff, so the source-of-truth (.proto) does not contain the new field. Additionally, the generated pb.go is missing a GetFreePageReporting() accessor method — every other optional field in the struct has one (e.g. GetForce(), GetTeamID()) — which is a further sign this was manually edited rather than properly regenerated.
What the impact would be
If any engineer regenerates template-manager.pb.go from the current .proto source (e.g. via buf generate or protoc), field 17 will be silently absent from the output. All code that sets or reads FreePageReporting — currently balloon.go and any orchestrator path that passes the config to the template manager — will compile but the field will never be serialized over gRPC, causing silent data loss or unexpected default behavior at runtime.
How to fix it
Add the field to the proto source: optional bool freePageReporting = 17; inside TemplateConfig in packages/orchestrator/template-manager.proto, then regenerate template-manager.pb.go. This will also produce the missing GetFreePageReporting() accessor.
Step-by-step proof
- Open packages/orchestrator/template-manager.proto — the highest field number in TemplateConfig is teamID = 16. No freePageReporting field exists.
- Open the PR diff for template-manager.pb.go. Line 702 introduces FreePageReporting *bool with tag varint,17.
- Search the entire pb.go for GetFreePageReporting — zero results. Every other optional field has a generated getter; this one does not.
- The rawDesc byte slice was also changed (size prefix updated), proving the file was at some point regenerated from a modified proto — but that proto was not committed.
- Consequence: running protoc or buf generate today against the committed .proto will produce a pb.go without field 17, silently discarding any FreePageReporting values set by callers.
ValentaTomas
force-pushed
the
feat/firecracker-v1.14
branch
from
fce22c3 to
737f601
Compare
1. Proto source: add freePageReporting field (=17) to template-manager.proto (was only in generated pb.go) 2. Deprecated field: migrate EnableDiffSnapshots → TrackDirtyPages in fc/client.go (EnableDiffSnapshots deprecated in FC v1.14) 3. Generated code bug: fix return nil → continue in contextValidate loop functions in cpu_config.go (4 places) and cpuid_leaf_modifier.go (1 place). These caused the loop to exit on the first zero-valued element, skipping validation of all subsequent elements. 4. Typos: fix "Reuturn" → "Return" and "Identificator" → "Identifier" in firecracker.yml and propagated generated files.
The contextValidate return nil→continue fix and typo fixes in cpu_config.go, cpuid_leaf_modifier.go, pmem.go, and operations_client.go were wrong to apply manually — these are "DO NOT EDIT" generated files that will be overwritten on regeneration. The actual fixes remain: - firecracker.yml typos fixed (source spec, not generated) - template-manager.proto field added (source, not generated) - EnableDiffSnapshots → TrackDirtyPages (our code, not generated) The go-swagger return nil bug and typo propagation should be fixed by regenerating from the corrected firecracker.yml source.
The freePageReporting field (=17) was added to the generated template-manager.pb.go but not to the source .proto file. This would cause the field to be silently dropped on regeneration. - Added `optional bool freePageReporting = 17` to template-manager.proto - Regenerated template-manager.pb.go (now includes GetFreePageReporting accessor) - Reverted unrelated changes (fc/client.go, firecracker.yml) that were not part of this fix
This was landed on feat/firecracker-v1.14 ahead of schedule; free page reporting is gated by feat/free-page-reporting which is not ready to merge yet. Drop the proto field (=17 is now free again) and regenerate the pb.go. The field will come back via the feature branch when it lands.
go-swagger generates additively, so renamed/removed spec entities leave orphan files behind. Add an rm -rf step to the go:generate directive and remove the leftover models/memory_dirty.go (superseded by dirty_memory.go).
4 participants
Summary
Test plan