Refine Node controller to support Gateway HA #4069
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
|
/test-multicluster-e2e |
Codecov Report
@@ Coverage Diff @@
## main #4069 +/- ##
==========================================
+ Coverage 56.09% 60.75% +4.66%
==========================================
Files 371 392 +21
Lines 53327 53927 +600
==========================================
+ Hits 29913 32766 +2853
+ Misses 21071 18716 -2355
- Partials 2343 2445 +102
|
|
@luolanzone : could you explain any real issue can happen without the changes? If there were indeed issues, please describe them in the commit description too. |
luolanzone
force-pushed
the
refine-node-controller
branch
from
0a46d83
to
3b9b20d
Compare
|
/test-multicluster-e2e |
luolanzone
changed the title
luolanzone
force-pushed
the
refine-node-controller
branch
from
3b9b20d
to
2b4e9d4
Compare
jianjuns
reviewed
Aug 22, 2022
luolanzone
force-pushed
the
refine-node-controller
branch
2 times, most recently
from
df38938
to
3f94ecb
Compare
jianjuns
reviewed
Aug 23, 2022
| if err := r.Client.List(ctx, nodeList, &client.ListOptions{}); err != nil { | ||
| return err | ||
| } | ||
| // Empty the cache in case of initializeActiveGateway failed and rerun. |
There was a problem hiding this comment.
I do not see you added code to do the cleanup? Maybe simpler to move the code to the end of func before returning without no error.
There was a problem hiding this comment.
Done, add the candidates initialization before return nil.
luolanzone
force-pushed
the
refine-node-controller
branch
from
3f94ecb
to
204f359
Compare
jianjuns
reviewed
Aug 25, 2022
| } | ||
| klog.ErrorS(err, "Failed to get Node", "node", req.Name) | ||
| return ctrl.Result{}, err | ||
| return ctrl.Result{}, client.IgnoreNotFound(err) |
There was a problem hiding this comment.
We should skip the previous log if not found, so I would return in line 103.
| isActiveGateway := r.activeGateway == node.Name | ||
| toRecreate := false | ||
| if hasGWAnnotation { | ||
| r.gatewayCandidates[req.Name] = true |
There was a problem hiding this comment.
We should delete the node from gatewayCandidates if no annotation.
luolanzone
force-pushed
the
refine-node-controller
branch
from
204f359
to
24da6e0
Compare
luolanzone
commented
Aug 25, 2022
jianjuns
reviewed
Aug 25, 2022
luolanzone
force-pushed
the
refine-node-controller
branch
2 times, most recently
from
8ee2fee
to
bca6819
Compare
jianjuns
reviewed
Aug 29, 2022
luolanzone
force-pushed
the
refine-node-controller
branch
from
bca6819
to
df49837
Compare
jianjuns
previously approved these changes
Aug 30, 2022
| } | ||
| existingGW.GatewayIP = newGateway.GatewayIP | ||
| existingGW.InternalIP = newGateway.InternalIP | ||
| if err := r.Client.Update(ctx, existingGW, &client.UpdateOptions{}); err != nil { |
There was a problem hiding this comment.
Could we add a comment about if the Gateway version in the client cache is stale, the update operation will fail?
luolanzone
force-pushed
the
refine-node-controller
branch
from
df49837
to
5ee993b
Compare
|
@luolanzone : please check if you can improve coverage to meet the target. |
luolanzone
force-pushed
the
refine-node-controller
branch
from
5ee993b
to
73ad778
Compare
|
/test-multicluster-e2e |
In order to support Gateway active-standby mode HA, the Node controller has been refined to handle Node readiness change. There will be at most one Gateway in a given Namespace, and the Gateway controller is updated correspondingly. 1. Check Node readiness and create a Gateway CR if Node is ready and there is no existing Gateway. 2. Initilize active Gateway and Gateway candidate Nodes with annotation 'multicluster.antrea.io/gateway'. 3. Add Gateway webhook to allow at most one Gateway in a given Namespace. Signed-off-by: Lan Luo <[email protected]>
luolanzone
force-pushed
the
refine-node-controller
branch
from
73ad778
to
49ceb32
Compare
|
/test-multicluster-e2e |
|
@jianjuns Could you help to check again? I added a few more tests, looks like the result of |
jianjuns
approved these changes
Sep 2, 2022
|
/skip-all |
heanlan
pushed a commit
to heanlan/antrea
that referenced
this pull request
Mar 29, 2023
…#4069) In order to support Gateway active-standby mode HA, the Node controller has been revised to handle Node readiness changes. There will be at most one Gateway in a single Namespace. 1. Check Node readiness and create a Gateway CR if Node is ready and there is no existing Gateway. 2. Initialize active Gateway and Gateway candidate Nodes with annotation 'multicluster.antrea.io/gateway'. 3. Add Gateway webhook to allow at most one Gateway in a single Namespace. Signed-off-by: Lan Luo <[email protected]>
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 order to support Gateway active-standby mode HA, the Node
controller has been refined to handle Node readiness changes.
There will be at most one Gateway in a given Namespace, and the
Gateway controller is updated correspondingly.
and there is no existing Gateway.
'multicluster.antrea.io/gateway'.
Namespace.
For #3754
Signed-off-by: Lan Luo [email protected]