job: Append jobs added before starting to lifecycle #51
Conversation
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
4d77c87 to
854353c
Compare
pippolo84
approved these changes
May 22, 2025
854353c to
c5bd9c4
Compare
In cilium/cilium the job groups were provided for each module and its
lifecycle hooks appended when constructed. This is problematic when the
job group is used multiple times as then the dependencies of the later
uses won't influence the order in which start hooks run, e.g. if a later
job depends on X it might happen that the job is started before X is started.
For example:
cell.Module(...,
cell.Provide(newX),
// First use of the job.Group will construct it and append hooks to
// lifecycle.
cell.Invoke(func(jg job.Group, ...) { jg.Add( ... )}),
// This second use will construct X and append its hooks, but these
// are now *after* the job.Group's hooks and hence we might start the
// job before X has been started.
cell.Invoke(func(jg job.Group, x *X) { jg.Add( ... )}),
)
To fix this, let's use the lifecycle to start the jobs that are added
before the job group is started. This way the start order will respect
the order in which [job.Group.Add] is called and thus be arranged according
to the dependency order.
This is a breaking change in the interface, but there's not that many
direct uses of "NewGroup" in cilium/cilium and thus fairly easy to fix.
Signed-off-by: Jussi Maki <jussi.maki@isovalent.com>
c5bd9c4 to
9abb9c0
Compare
joamaki
added a commit
to cilium/statedb
that referenced
this pull request
May 22, 2025
This pulls in cilium/hive#51 and fixes the usage. Signed-off-by: Jussi Maki <jussi.maki@isovalent.com>
joamaki
added a commit
to cilium/statedb
that referenced
this pull request
May 22, 2025
This pulls in cilium/hive#51 and fixes the usage. Signed-off-by: Jussi Maki <jussi.maki@isovalent.com>
joamaki
added a commit
to joamaki/cilium
that referenced
this pull request
May 22, 2025
This pulls in the new Hive version that has a safer API for jobs: cilium/hive#51. Code is fixed as both Hive and StateDB had breaking API changes. Signed-off-by: Jussi Maki <jussi@isovalent.com>
joamaki
added a commit
to joamaki/cilium
that referenced
this pull request
May 22, 2025
This pulls in the new Hive version that has a safer API for jobs: cilium/hive#51. Code is fixed as both Hive and StateDB had breaking API changes. Signed-off-by: Jussi Maki <jussi@isovalent.com>
joamaki
added a commit
to joamaki/cilium
that referenced
this pull request
May 23, 2025
This pulls in the new Hive version that has a safer API for jobs: cilium/hive#51. Code is fixed as both Hive and StateDB had breaking API changes. Signed-off-by: Jussi Maki <jussi@isovalent.com>
github-merge-queue bot
pushed a commit
to cilium/cilium
that referenced
this pull request
May 25, 2025
This pulls in the new Hive version that has a safer API for jobs: cilium/hive#51. Code is fixed as both Hive and StateDB had breaking API changes. Signed-off-by: Jussi Maki <jussi@isovalent.com>
github-merge-queue bot
pushed a commit
to cilium/cilium
that referenced
this pull request
May 25, 2025
This pulls in the new Hive version that has a safer API for jobs: cilium/hive#51. Code is fixed as both Hive and StateDB had breaking API changes. Signed-off-by: Jussi Maki <jussi@isovalent.com>
zzuckerfrei
pushed a commit
to zzuckerfrei/cilium
that referenced
this pull request
May 29, 2025
This pulls in the new Hive version that has a safer API for jobs: cilium/hive#51. Code is fixed as both Hive and StateDB had breaking API changes. Signed-off-by: Jussi Maki <jussi@isovalent.com>
joamaki
added a commit
to joamaki/cilium
that referenced
this pull request
Jun 2, 2025
[ upstream commit a719911 ] [ backporter note: disabled the experimental tests as they're not relevant to run for v1.17 and these old versions were flaky ] This pulls in the new Hive version that has a safer API for jobs: cilium/hive#51. Code is fixed as both Hive and StateDB had breaking API changes. Signed-off-by: Jussi Maki <jussi@isovalent.com>
github-merge-queue bot
pushed a commit
to cilium/cilium
that referenced
this pull request
Jun 4, 2025
[ upstream commit a719911 ] [ backporter note: disabled the experimental tests as they're not relevant to run for v1.17 and these old versions were flaky ] This pulls in the new Hive version that has a safer API for jobs: cilium/hive#51. Code is fixed as both Hive and StateDB had breaking API changes. Signed-off-by: Jussi Maki <jussi@isovalent.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In cilium/cilium the job groups were provided for each module and its lifecycle hooks appended when constructed. This is problematic when the job group is used multiple times as then the dependencies of the later uses won't influence the order in which start hooks run, e.g. if a later job depends on X it might happen that the job is started before X is started.
For example:
To fix this, let's use the lifecycle to start the jobs that are added before the job group is started. This way the start order will respect the order in which [job.Group.Add] is called and thus be arranged according to the dependency order.
This is a breaking change in the interface, but there's not that many direct uses of "NewGroup" in cilium/cilium and thus fairly easy to fix.