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

Remove MemberClusterAnnounce if ClusterSet deleted #4026

Merged
merged 1 commit into from
Jul 21, 2022

Conversation

luolanzone
Copy link
Contributor

@luolanzone luolanzone commented Jul 18, 2022

Add a step to ensure that member ClusterSet controller will
clean up MemberClusterAnnounce in the leader cluster if
the ClusterSet in the member is deleted.

@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

@codecov
Copy link

codecov bot commented Jul 18, 2022

Codecov Report

Merging #4026 (ab2eaa5) into main (1b4d55e) will increase coverage by 1.62%.
The diff coverage is 47.52%.

❗ Current head ab2eaa5 differs from pull request most recent head 390dc13. Consider uploading reports for the commit 390dc13 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4026      +/-   ##
==========================================
+ Coverage   64.34%   65.96%   +1.62%     
==========================================
  Files         294      309      +15     
  Lines       43634    44125     +491     
==========================================
+ Hits        28076    29107    +1031     
+ Misses      13282    12693     -589     
- Partials     2276     2325      +49     
Flag Coverage Δ *Carryforward flag
e2e-tests 60.36% <62.27%> (?)
kind-e2e-tests 51.32% <36.09%> (+0.79%) ⬆️ Carriedforward from 40e5277
unit-tests 44.26% <14.83%> (-0.08%) ⬇️ Carriedforward from 40e5277

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
...ter/apis/multicluster/v1alpha1/clusterset_types.go 100.00% <ø> (ø)
...icluster/cmd/multicluster-controller/controller.go 55.17% <ø> (+46.83%) ⬆️
pkg/agent/agent_linux.go 5.36% <0.00%> (+0.22%) ⬆️
pkg/agent/cniserver/pod_configuration.go 54.59% <0.00%> (ø)
pkg/agent/proxy/endpoints.go 78.57% <ø> (ø)
pkg/agent/util/ndp/ndp.go 0.00% <0.00%> (ø)
pkg/agent/util/net.go 48.19% <0.00%> (-4.19%) ⬇️
pkg/agent/util/net_linux.go 29.67% <0.00%> (-4.53%) ⬇️
pkg/features/antrea_features.go 11.11% <ø> (ø)
pkg/ovs/ovsconfig/ovs_client.go 46.39% <25.92%> (-1.41%) ⬇️
... and 86 more

@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

@luolanzone luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Jul 18, 2022
@@ -92,6 +92,9 @@ func (r *MemberClusterSetReconciler) Reconcile(ctx context.Context, req ctrl.Req
}
klog.InfoS("Received ClusterSet delete", "clusterset", req.NamespacedName)
if r.remoteCommonArea != nil {
if err := r.deleteMemberClusterAnnounce(ctx); err != nil {
return ctrl.Result{}, err
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to prepend context to the returned err (or log the failure with context here), otherwise the log by the caller wont indicate what fails.

e.g. fmt.Errorf("failed to delete MemberClusterAnnounce in the leader cluster: %v", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Add a step to ensure that member ClusterSet controller will
clean up MemberClusterAnnounce in the leader cluster if
the ClusterSet in the member is deleted.

Signed-off-by: Lan Luo <luola@vmware.com>
@luolanzone luolanzone force-pushed the memberclusterannounce-cleanup branch from 390dc13 to 3afba07 Compare July 19, 2022 01:02
@luolanzone
Copy link
Contributor Author

@jianjuns Could you help to double check this PR if it's Ok to merge? @hjiajing will do a rebase when this is merged.

@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

@jianjuns
Copy link
Contributor

/skip-all
The change affects only mc-controller.

@jianjuns jianjuns merged commit 9f76f8e into antrea-io:main Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/multi-cluster Issues or PRs related to multi cluster.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants