Enable BPF without disruption #3183
Merged
Conversation
This file contains 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
song-jiang
force-pushed
the
enable-bpf
branch
from
505c932 to
41003a0
Compare
song-jiang
force-pushed
the
enable-bpf
branch
from
5ec79e4 to
09c5e40
Compare
song-jiang
changed the title
caseydavenport
approved these changes
Feb 26, 2024
There was a problem hiding this comment.
Two very minor comments, but otherwise this LGTM - thanks @song-jiang!
| return nil | ||
| } | ||
|
|
||
| // If the Installation resource has been patched to dataplane: BPF then the |
There was a problem hiding this comment.
Just a nit, but the function comments should start with the function name. e.g.,
isRolloutCompleteWithBPFVolumes checks if the calico-node DaemonSet . . .
| @@ -1489,6 +1489,17 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile | |||
| // Tell the status manager that we're ready to monitor the resources we've told it about and receive statuses. | |||
| r.status.ReadyToMonitor() | |||
|
|
|||
| // BPF Upgrade without disruption: | |||
| // Delegate logic implementation here using the state of the installation and dependent resources. | |||
There was a problem hiding this comment.
I suggest we modify this comment to something like this:
If eBPF is enabled in the operator API, patch FelixConfiguration to enable it within Felix.
tomastigera
reviewed
Apr 9, 2024
| if fc.Spec.BPFEnabled == nil || *fc.Spec.BPFEnabled { | ||
| err := setBPFEnabledOnFelixConfiguration(fc, bpfEnabledOnInstall) | ||
| if err != nil { | ||
| reqLogger.Error(err, "Unable to enable eBPF data plane") |
There was a problem hiding this comment.
Is this correct? bpfEnabledOnInstall == false so arent's we trying to explicitly disable?
There was a problem hiding this comment.
Good catch! Raised PR to fix it.
@caseydavenport PTAL. Thanks.
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.
Description
This PR is based on #2921. It should been seen as a continuation of the original work with the following modifications:
For PR author
make gen-filesmake gen-versionsFor PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bugif this is a bugfix.kind/enhancementif this is a a new feature.enterpriseif this PR applies to Calico Enterprise only.