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

Daemon: Remove redundant call to AddClusterMember #104

Closed
wants to merge 1 commit into from

Conversation

roosterfish
Copy link
Contributor

@roosterfish roosterfish commented Apr 10, 2024

When joining a new node to the cluster this action is already performed as part of the joinWithToken function on the control endpoint: https://github.com/canonical/microcluster/blob/main/internal/rest/resources/control.go#L104.

If the first node gets bootstrapped, trust for itself is added as part of StartAPI here https://github.com/canonical/microcluster/blob/main/internal/daemon/daemon.go#L374.

When joining a new node to the cluster this action is already performed
as part of the joinWithToken function on the control endpoint.

Signed-off-by: Julian Pelizäus <[email protected]>
@roosterfish
Copy link
Contributor Author

@masnax opening this as a draft PR because I am not sure if I am missing something else here.

@masnax
Copy link
Contributor

masnax commented Apr 10, 2024

Ah yes, I was wondering when someone would raise an eyebrow at this :)

This is part of what's going to be addressed by #89

Before we had so many hooks PreJoin, PostJoin, OnNewMember, etc., the main pattern I had used to actually run the hooks was to set the cluster notification header and re-use the same endpoint, but handle it differently.

The cluster notification header is set with the final argument here which applies this function as the proxy function for the client. This is sort of hinted at with the comment here.

Then, when we call AddClusterMember, we forego the default actions and instead run this block which executes the OnNewMember hook.

@masnax
Copy link
Contributor

masnax commented Apr 10, 2024

The final solution in #89 isn't ideal either, and I've been debating redoing that PR and just moving all the hooks over to their own set of endpoints to make things clear and distinct.

@roosterfish
Copy link
Contributor Author

Appreciate the detailed response, so with the change in this PR it doesn't anymore send around the notification so it won't run the OnNewMemberHook(s) on the other cluster members?

This explains why I could still form a functioning cluster, but this is breaking the hooks then, haven't tested this.

I'll close it and keep an eye on #89. Let me know when you need a review.

@roosterfish
Copy link
Contributor Author

This is sort of hinted at with the comment here.

Yeah I saw this but got confused by the "upgrade" in there.

@roosterfish roosterfish deleted the fix_duplicate branch April 10, 2024 16:04
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