Skip to content

Zone instances and IDs#528

Draft
bal-e wants to merge 2 commits into
mainfrom
new-instances
Draft

Zone instances and IDs#528
bal-e wants to merge 2 commits into
mainfrom
new-instances

Conversation

@bal-e
Copy link
Copy Markdown
Contributor

@bal-e bal-e commented Mar 17, 2026

This PR introduces zone/instance.rs for tracking zone instances. It introduces {Loaded,Signed}InstanceID as a way to uniquely identify loaded and signed instances of zones, respectively; unlike SOA serial numbers, instance IDs are guaranteed to be unique (even if two instances have the same SOA serial, or if an instance is rejected). The rest of the codebase has been integrated with this system, so IDs are correctly available at every step (except review to some extent; I will work on it next).

The new state for instances will eventually replace the .{unsigned,signed} fields of ZoneState. They will accumulate more instance-specific data, e.g. loader/signer statistics, diffs (for persistence, rollback, and IXFR out), maybe even logs/history.


  • If you are changing Rust code or integration tests (Cargo.*, crates/, etc/, integration-tests/, src/):
    • Did you run the integration tests with act through the act-wrapper (as described in TESTING.md)?

@bal-e bal-e added this to the 0.1.0-beta1 milestone Mar 17, 2026
@bal-e bal-e requested a review from ximon18 March 17, 2026 12:56
@bal-e bal-e self-assigned this Mar 17, 2026
@bal-e bal-e marked this pull request as draft March 17, 2026 15:00
@bal-e
Copy link
Copy Markdown
Contributor Author

bal-e commented Mar 17, 2026

Converting to draft while I consider adding some more state.

@bal-e bal-e marked this pull request as ready for review March 18, 2026 11:11
This will replace 'ZoneState::{signed,unsigned}' with a dedicated module
and track more information there.
@bal-e bal-e marked this pull request as draft March 18, 2026 12:02
Instance IDs are now threaded through all relevant code. This will serve
as a better replacement for serial numbers for review.
@ximon18
Copy link
Copy Markdown
Member

ximon18 commented Mar 19, 2026

@bal-e: My first thought when looking at this is that there seems to be a strong coupling between builder and instance state changes yet the two are decoupled, with the potential for forgetting to update one when changing the other, as evidenced even by the "// TODO: Update something in 'handle.state.instances.?" comment in the code.

As an example, if we look at the loader::zone::start(builder: LoadedZoneBuilder) case it calls start_load() on instances to get an ID, and the builder was obtained by. the caller via self.zone().storage().start_load() where StorageZoneHandle also has access to ZoneState - could the storage().start_load() call not invoke instances.start_load() and associate the instance ID with the builder, and then later handle.state.instances.abandon_load(id) and handle.storage().abandon_load(builder) could be combined to use the id from the builder to update the instance state?

I haven't looked far enough into the changes to understand if this pattern applies universally or not, could it be an idea to couple these concepts to avoid foot guns involving forgetting to update the instance and the storage at the same time?

@bal-e bal-e mentioned this pull request Mar 19, 2026
5 tasks
@ximon18
Copy link
Copy Markdown
Member

ximon18 commented Mar 27, 2026

except review to some extent; I will work on it next

In this PR, or in another PR?

Copy link
Copy Markdown
Member

@ximon18 ximon18 left a comment

Choose a reason for hiding this comment

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

I'm having trouble reviewing this as I'm wondering why this introduces another state machine, and how that relates to the storage and zone state machines. The actual code is not that complex, but I think it would be good to sit with @bal-e and review it together to better understand the thinking behind this PR.

Comment thread src/zone/instance.rs
#[derive(Debug, Default)]
pub struct Instances {
/// The ID of the next loaded instance.
pub next_loaded_id: u64,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should these fields be pub? If we assume that the functions on this type manage this state, by being pub a user (with mut access) could change the state without going via the functions, is that wise?

@bal-e
Copy link
Copy Markdown
Contributor Author

bal-e commented Mar 30, 2026

I'm having trouble reviewing this as I'm wondering why this introduces another state machine, and how that relates to the storage and zone state machines. The actual code is not that complex, but I think it would be good to sit with @bal-e and review it together to better understand the thinking behind this PR.

Yes, I haven't touched this PR in a while because I need to rethink my approach. It took me a while to realize that I was accidentally building another state machine. I will rebase it on top of main, with Terts' merged state machine PR; hopefully that will clarify things for me.

@bal-e bal-e mentioned this pull request May 1, 2026
2 tasks
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.

2 participants