Skip to content
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

Loader reconciliation preparatory changes #31773

Merged

Conversation

dylandreimerink
Copy link
Member

We are slowly moving towards a loader that is resilient and reconciles whenever device changes are detected. Currently we have datapath logic to detect device changes, this needs to be reported up all the way to the Daemon which then pushes Reinitilization requests all the way down to the loader. This PR contains a number of preparatory changes that will make it easier for a followup PR to make the datapath / loader reconcile without going through the daemon, based on the devices table.

This PR removes the BaseProgramOwner which was one big layer violation. All dependencies the loader needs are now passed via Hive. Only the actual Reinitialize calling still exists, which will be replaced by device based reconciliation in a followup.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 4, 2024
@dylandreimerink dylandreimerink added the release-note/misc This PR makes changes that have no direct user impact. label Apr 4, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 4, 2024
@dylandreimerink dylandreimerink force-pushed the feature/loader-reconciliation-2 branch 8 times, most recently from 63596e2 to 094dce4 Compare April 5, 2024 15:35
@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink dylandreimerink force-pushed the feature/loader-reconciliation-2 branch from 094dce4 to 2ec3fbb Compare April 9, 2024 09:03
@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink dylandreimerink force-pushed the feature/loader-reconciliation-2 branch from 2ec3fbb to 6e94d9f Compare April 9, 2024 11:32
@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink dylandreimerink force-pushed the feature/loader-reconciliation-2 branch 2 times, most recently from 86afd5b to f16011c Compare April 10, 2024 08:38
@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink dylandreimerink marked this pull request as ready for review April 10, 2024 11:47
@dylandreimerink dylandreimerink requested review from a team as code owners April 10, 2024 11:47
@dylandreimerink
Copy link
Member Author

/test

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Looks good for @cilium/ci-structure (re-adding review requests for the other code owners which I didn't cover).

@tklauser tklauser requested review from a team April 23, 2024 11:11
Copy link
Contributor

@gentoo-root gentoo-root left a comment

Choose a reason for hiding this comment

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

Great cleanup of dependencies!

pkg/datapath/types/loader.go Outdated Show resolved Hide resolved
daemon/cmd/config.go Outdated Show resolved Hide resolved
pkg/proxy/cell.go Show resolved Hide resolved
Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

LGTM for the proxy files.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 24, 2024
@ti-mo
Copy link
Contributor

ti-mo commented Apr 25, 2024

A few unresolved conversations that should be addressed before merging. Looks like ci-e2e-upgrade is not required at the moment, but there's one connection interruption during upgrade. Could it be related?

@ti-mo ti-mo removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 25, 2024
@dylandreimerink
Copy link
Member Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 29, 2024
This commit introduces the orchestrator package and type. The
orchestrator is going to be responsible for the startup and shutdown of
the datapath.

Currently the loaders `Reinitialize` is responsible for
creating and reconciling a lot of datapath state. This commit places
the orchestrator in the call path as a proxy, this will allow us to
move the logic from the loader into the orchestrator in the following
commits.

Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
This commit modularizes the prefilter. The loader no longer creates it
internally, but gets a reference to is via hive. The daemon likewise
gets its reference the same way. This allows us to remove the prefilter
passing from the program base owner.

To preserve the existing API behavior, the prefilter now can be enabled
or disabled based on settings instead of being nil when disabled.

Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
We have this odd layer break where the Daemon holds a mutex which lock
the loader, a very low level component. As prep for an upcoming change
where the loader doesn't need a reference to the Daemon, we create a
new type called the CompilationLock which is just an embedded mutex.
This new type can be depended on by the daemon so it can still satify
interfaces required for the endpoint package. The loader can also
depend on the new type directly, this allows us to remove the
compilation lock getter from the base program owner.

Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
This commit removes the `BaseProgramOwner` interface. All values it
provided can now be access via Hive. This means the loader can now
function without abstracted pointers to the daemon which opens the way
for us to implement reconciliation without going through the daemon.

Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
The orchestrator now has a Reinitialize function which only takes a
context. All other types are now provided via Hive. This will make it
possible in a future changeset to have the orchestrator call the loader
whenever it wants to reconcile.

Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
@dylandreimerink dylandreimerink removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 29, 2024
@dylandreimerink
Copy link
Member Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 29, 2024
@dylandreimerink dylandreimerink added this pull request to the merge queue Apr 29, 2024
Merged via the queue into cilium:main with commit 579445b Apr 29, 2024
62 of 64 checks passed
@dylandreimerink dylandreimerink deleted the feature/loader-reconciliation-2 branch April 29, 2024 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants