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

fix common area bug #25

Closed

Conversation

luolanzone
Copy link
Collaborator

@luolanzone luolanzone commented Oct 8, 2021

  1. fix the issue that remote cluster is actually added to a local variable instead of r.RemoteClusterManager
  2. fix the issue that RemoteClusterManager is set to nil immediately after it's start
    Signed-off-by: Lan Luo luola@vmware.com

Signed-off-by: Lan Luo <luola@vmware.com>
@luolanzone
Copy link
Collaborator Author

@abhiraut I found a few bugs during common area integration, so I submit this PR in case you need this fix to work on integration for importer.

@@ -206,18 +206,16 @@ func (r *ClusterSetReconciler) updateMultiClusterSetOnMemberCluster(clusterSet *
if r.RemoteClusterManager == nil {
r.RemoteClusterManager = internal.NewRemoteClusterManager(r.clusterSetID, r.Log, r.clusterID)
go func() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@aravindakidambi I am wondering why we need a go routine here? since I noticed that there will be an inconsistent issue if r.RemoteClusterManager is initialized and used in other controller like serviceexporter controller, but after a while, it might become nil when start() failed. I suppose we can start it without go routine?

@abhiraut
Copy link
Owner

abhiraut commented Oct 8, 2021

@abhiraut I found a few bugs during common area integration, so I submit this PR in case you need this fix to work on integration for importer.

thanks! could you add the fix description in the PR :)

@luolanzone
Copy link
Collaborator Author

@abhiraut sure, I have updated the description of the issue.

@abhiraut
Copy link
Owner

@aravindakidambi could you take one look, so we can merge this ASAP

@abhiraut
Copy link
Owner

@luolanzone could you rebase and we will merge after that

@luolanzone
Copy link
Collaborator Author

@abhiraut I think we can close this one since this fix is already in PR22 which you have merged.

@luolanzone luolanzone closed this Oct 14, 2021
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.

2 participants