Add controller for leader cluster to GC MemberClusterAnnounce #4054
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4054 +/- ##
==========================================
+ Coverage 64.39% 65.82% +1.43%
==========================================
Files 293 307 +14
Lines 43669 44618 +949
==========================================
+ Hits 28121 29371 +1250
+ Misses 13263 12871 -392
- Partials 2285 2376 +91
*This pull request uses carry forward flags. Click here to find out more.
|
| } | ||
| } | ||
| if !exist { | ||
| klog.InfoS("Cleaning up stale MemberClusterAnnounce. It does exist in the member cluster list", "MemberClusterAnnounce", klog.KObj(&memberClusterAnnounce)) |
There was a problem hiding this comment.
You shouldn't use &memberClusterAnnounce here, you need a line to assign it to a new variable and use it in the loop. You may google why it's needed.
There was a problem hiding this comment.
I knew in the for...each range. The value always uses the same address. But the process is serial, maybe using the same address is okay? I'm not sure, added a new variable to avoid the risk.
| for _, memberClusterAnnounce := range memberClusterAnnounceList.Items { | ||
| var exist bool | ||
| var needToDelete bool | ||
| if len(clusterSetList.Items) != 0 { |
There was a problem hiding this comment.
I think you can move this check out of loop, and get the members info before this loop.
hjiajing
force-pushed
the
stale-controller
branch
from
d9e00f4
to
5061617
Compare
hjiajing
force-pushed
the
stale-controller
branch
2 times, most recently
from
213aaf8
to
1a3ad19
Compare
| needToDelete = true | ||
| } else { | ||
| lastUpdateTime, err := time.Parse(time.RFC3339, memberClusterAnnounce.Annotations[commonarea.TimestampAnnotationKey]) | ||
| if err == nil && time.Now().Sub(lastUpdateTime) < 5*time.Minute { |
There was a problem hiding this comment.
Please define a constant, like memberClusterAnnounceStaleTime
| } | ||
| } | ||
| if !exist { | ||
| klog.InfoS("Cleaning up stale MemberClusterAnnounce. It does not exist in the member cluster list", "MemberClusterAnnounce", klog.KObj(&memberClusterAnnounce)) |
There was a problem hiding this comment.
But why we delete MemberAnnounce if it is not in the ClusterSet. It is possible leader cluster has not processed the update yet?
I feel we just check timestamp to delete stale members.
There was a problem hiding this comment.
FIxed. Just check the timestamp and delete the outdated resources.
hjiajing
force-pushed
the
stale-controller
branch
from
1a3ad19
to
d43e45d
Compare
|
|
||
| for _, m := range memberClusterAnnounceList.Items { | ||
| var needToDelete bool | ||
| memberClusterAnnounce := m |
There was a problem hiding this comment.
I do not get why you need this line. If you like to use "memberClusterAnnounce" rather than "m", you can just change "m" to "memberClusterAnnounce" in line 306.
There was a problem hiding this comment.
use memberClusterAnnounce now.
There was a problem hiding this comment.
@hjiajing I recalled that you added this line because of a golang ci failure, please run make golangci to double check it locally first.
There was a problem hiding this comment.
Added the temporary variable back. And removed the next "else" block because there's a "continue" above it.
There was a problem hiding this comment.
Could I know what failure we get if we remove memberClusterAnnounce := m?
There was a problem hiding this comment.
The error will be like 'Implicit memory aliasing in for loop'. This is also one of common mistake in golang, and the wiki provide the solution like @hjiajing did here. https://github.com/golang/go/wiki/CommonMistakes#using-reference-to-loop-iterator-variable
There was a problem hiding this comment.
Ok. That makes sense to me now. Thanks for clarification!
luolanzone
changed the title
hjiajing
force-pushed
the
stale-controller
branch
from
d43e45d
to
b1d2fd7
Compare
Signed-off-by: hujiajing <hjiajing@vmware.com>
hjiajing
force-pushed
the
stale-controller
branch
from
b1d2fd7
to
c32fd04
Compare
|
@hjiajing : a reminder about my last question: #4054 (comment) |
|
@jianjuns Sorry for the late reply. If not set the |
|
/test-multicluster-e2e |
|
/test-integration |
1 similar comment
|
/test-integration |
Add a role argument for the StaleController so that it can run in both
leader and member clusters. When it runs in a member cluster, the
controller will GC Service, ACNP, and CI resources. When it runs in a
leader cluster, the controller will GC MemberClusterAnnounces.
Signed-off-by: hujiajing hjiajing@vmware.com