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

Level driven controller #85

Merged
merged 7 commits into from
Nov 18, 2022
Merged

Level driven controller #85

merged 7 commits into from
Nov 18, 2022

Conversation

maiqueb
Copy link
Collaborator

@maiqueb maiqueb commented Nov 17, 2022

What this PR does / why we need it:
Re-write the controller as level driven, instead of edge-triggered.

This makes user interaction more resilient, since the controller will try to materialize the desired state (network-selection-elements) into the current state (network-status).

There is one behavior that needs to be highlighted: the existing interfaces need to always feature the interface name in the desired state (network selection elements); take the following example:

  • initially, there is a secondary interface in the pod for which the user did not name explicitly.
    • at t=1, the user attempts to hotplug interfaces; these will fail
    • at t=2, the user corrects the desired state, manually specifying the name of the interface for the existing attachment
    • the controller will reconcile the pod state, and will now be able to hotplug the new interfaces, specified at t=2

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #48
Fixes #66

Special notes for your reviewer (optional):
Depends-on: #46

@maiqueb maiqueb requested a review from kmabda November 17, 2022 16:00
@maiqueb maiqueb force-pushed the level-driven-controller branch 2 times, most recently from 6ff8cef to 8373cd1 Compare November 17, 2022 17:03
Copy link
Collaborator

@kmabda kmabda left a comment

Choose a reason for hiding this comment

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

The approach looks good to me.

I do see though we're missing handling the case where an update is made to the networks annotation while the controller is not present:

  • t=1 while the controller is not started or crashed or restarting after a rollup upgrade etc., a user changed a running pod annotation to hotplug an interface
  • t=2 controller start

To reconcile, on startup we need to do call pnc.podsLister.List() to add all Running pods on the controller's node that are not in the hostNetwork to the pnc.workqueue

pkg/controller/pod.go Outdated Show resolved Hide resolved
pkg/controller/pod.go Outdated Show resolved Hide resolved
When converging the desired / current state differences we must always
omit the default network, but when we are computing the current state
**after** deleting interfaces, the default network's state must always
be present.

Signed-off-by: Miguel Duarte Barroso <[email protected]>
@maiqueb maiqueb force-pushed the level-driven-controller branch from 8373cd1 to a42614b Compare November 18, 2022 11:01
Signed-off-by: Miguel Duarte Barroso <[email protected]>
@maiqueb
Copy link
Collaborator Author

maiqueb commented Nov 18, 2022

The approach looks good to me.

I do see though we're missing handling the case where an update is made to the networks annotation while the controller is not present:

  • t=1 while the controller is not started or crashed or restarting after a rollup upgrade etc., a user changed a running pod annotation to hotplug an interface
  • t=2 controller start

To reconcile, on startup we need to do call pnc.podsLister.List() to add all Running pods on the controller's node that are not in the hostNetwork to the pnc.workqueue

Good point; would you mind opening an issue and pushing a PR to address this @kmabda ? I do not know exactly where that instruction should be put.

@maiqueb maiqueb merged commit 786ca28 into main Nov 18, 2022
@maiqueb maiqueb deleted the level-driven-controller branch November 18, 2022 11:59
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.

[BUG] updates to an existing attachment should be rejected [RFE] refactor the pod controller as level driven
2 participants