Skip to content

feat: impl new supervisor traits k8s#2055

Merged
DavSanchez merged 19 commits intomainfrom
feat/impl-new-supervisor-traits-k8s
Jan 20, 2026
Merged

feat: impl new supervisor traits k8s#2055
DavSanchez merged 19 commits intomainfrom
feat/impl-new-supervisor-traits-k8s

Conversation

@DavSanchez
Copy link
Contributor

@DavSanchez DavSanchez commented Jan 14, 2026

Implements the new traits defined on #2048 for the Kubernetes running mode of AC. The implementation actually stops and destroys the running supervisor and starts a new supervisor under the apply method (making it a bit of a misnomer). This is mainly a matter of thread management ergonomics, but this could be reworked if desired (this PR or later iterations) by adding some data sharing.

Checklist

  • Provided a meaningful title following conventional commit style.
  • Included a detailed description for the Pull Request.
  • Documentation under docs is aligned with the change.
  • Follows guidelines for Pull Requests in CONTRIBUTING.md.
  • Follows log level guidelines.

@DavSanchez DavSanchez requested a review from a team as a code owner January 14, 2026 14:48
@DavSanchez DavSanchez force-pushed the feat/impl-new-supervisor-traits-k8s branch 2 times, most recently from bc538fd to 21b7be4 Compare January 16, 2026 08:38
where
I: IntoIterator<Item = StartedThreadContext<()>>,
{
fn stop(self) -> Result<(), ThreadContextStopperError> {
Copy link
Contributor

@danielorihuela danielorihuela Jan 16, 2026

Choose a reason for hiding this comment

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

Cool! I think it reads better, and we can improve it a little bit more I think. What about adding the logs directly to the stop_blocking function?

/// It sends a stop signal and waits until the thread handle is joined.
pub fn stop_blocking(self) -> Result<T, ThreadContextStopperError> {
    ...
    self.join_thread()
        .inspect(|_| debug!("Thread {thread_name} stopped"))
        .inspect_err(|e| error!("Error stopping thread {thread_name}: {e}"))
}

fn stop(self) -> Result<(), ThreadContextStopperError> {
    let stopped_threads = self
        .into_iter()
        .map(|ctx| ctx.stop_blocking())
        .collect::<Vec<_>>();

    stopped_threads
        .into_iter()
        // Return first err
        .try_for_each(identity)
}

Also, using some variables to differentiate steps might help. But this is purely subjective.

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 was initially against it to not change much of the existing implementation, but I can see the advantage and finally moved the logs as you suggested, thanks!

Copy link
Contributor

@danielorihuela danielorihuela Jan 19, 2026

Choose a reason for hiding this comment

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

Maybe you can have a separate PR with these improvements if you want. I think it's more work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logs were already moved to the stop_blocking and there's a bit of comment to indicate what exactly identity is for. Let me know if you'd like something else!

Copy link
Contributor

@gsanchezgavier gsanchezgavier left a comment

Choose a reason for hiding this comment

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

could you include some PR description ?

@DavSanchez DavSanchez force-pushed the feat/impl-new-supervisor-traits-k8s branch from f73b753 to 55009c1 Compare January 19, 2026 13:13
@DavSanchez
Copy link
Contributor Author

could you include some PR description ?

Certainly! 🚀

type ApplyError = ApplyError;
type StopError = ThreadContextStopperError;

fn apply(self, effective_agent: EffectiveAgent) -> Result<Self, Self::ApplyError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the new approach. We avoid having a special "try_build_new_supervisor" or something like that as before. Cool!!!

}
}

// Clone the k8s_client on each build.
Copy link
Contributor

Choose a reason for hiding this comment

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

it is an arc right? if so perhaps i would remove this comment that brings some uncertainty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's an arc, I'll delete the comment or specify we clone the reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove misleading comment c976700

Copy link
Contributor

@gsanchezgavier gsanchezgavier left a comment

Choose a reason for hiding this comment

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

Cool thanks!, I just left a question

Comment on lines +173 to +174
// Return first err (`identity` acts as a "pass-through" here,
// so the first Err interrupts the iteration)
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 is it ok to stop , wouldn't that leak threads ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the threads are all stopped in the previous binding let stopped_threads, this only selects the first error among the results (a behavior similar to the one we had before, perhaps it's time to aggregate the errors and return them all instead?)

Copy link
Contributor

Choose a reason for hiding this comment

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

ohh i just see these are the returned errors of the joins !

perhaps it's time to aggregate the errors and return them all instead?

It looks enough as it is now for me 👍

Copy link
Contributor

@gsanchezgavier gsanchezgavier left a comment

Choose a reason for hiding this comment

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

🚀

Comment on lines +173 to +174
// Return first err (`identity` acts as a "pass-through" here,
// so the first Err interrupts the iteration)
Copy link
Contributor

Choose a reason for hiding this comment

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

ohh i just see these are the returned errors of the joins !

perhaps it's time to aggregate the errors and return them all instead?

It looks enough as it is now for me 👍

@DavSanchez DavSanchez merged commit 4ceaa00 into main Jan 20, 2026
33 checks passed
@DavSanchez DavSanchez deleted the feat/impl-new-supervisor-traits-k8s branch January 20, 2026 12:00
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.

3 participants