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

Allow passing additional configuration during bootstrap #72

Merged
merged 10 commits into from
Nov 23, 2023

Conversation

markylaing
Copy link
Contributor

This PR adds an argument to (*Microcluster).NewCluster of the form map[string]string which is passed into the OnBootstrap hook. This allows clients to pass additional configuration that might be required at bootstrap.

@masnax
Copy link
Contributor

masnax commented Nov 22, 2023

Since we're taking this approach, I suppose we can rip the bandaid off and add a force flag to the remove hooks as well.

Not necessarily in this PR

Copy link
Contributor

@masnax masnax left a comment

Choose a reason for hiding this comment

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

Since not every service using microcluster is going to have MicroCloud's neat and tidy automatic join process, we should probably make this config a more general InitConfig and pass it to all of OnBootstrap, PreJoin, and PostJoin so that when going through the join setup, user specified config can get propagated there as well.

@markylaing
Copy link
Contributor Author

Since we're taking this approach, I suppose we can rip the bandaid off and add a force flag to the remove hooks as well.

Not necessarily in this PR

We don't necessarily need to take this approach. It's still up for discussion :) That being said, I do think this is better than the alternative, which (I think) seems to require the client to make two api calls, one to bootstrap the microcluster, and the other to initialise ceph... which kind of defeats the purpose of the hooks.

@markylaing
Copy link
Contributor Author

Since not every service using microcluster is going to have MicroCloud's neat and tidy automatic join process, we should probably make this config a more general InitConfig and pass it to all of OnBootstrap, PreJoin, and PostJoin so that when going through the join setup, user specified config can get propagated there as well.

Yes. Another thought I had was that in it's current form, this PR does the microcluster bootstrap part before the hook is called. This means that any validation of the config that happens server side will be happening too late. Maybe a PreBootstrap hook is necessary now?

@masnax
Copy link
Contributor

masnax commented Nov 22, 2023

Since not every service using microcluster is going to have MicroCloud's neat and tidy automatic join process, we should probably make this config a more general InitConfig and pass it to all of OnBootstrap, PreJoin, and PostJoin so that when going through the join setup, user specified config can get propagated there as well.

Yes. Another thought I had was that in it's current form, this PR does the microcluster bootstrap part before the hook is called. This means that any validation of the config that happens server side will be happening too late. Maybe a PreBootstrap hook is necessary now?

Unlike with PreJoin where we actually might have to reconcile information with the existing cluster, there wouldn't be any pre-setup that would have to happen exclusively on the daemon prior to bootstrapping since we won't even have a database or any internal setup, so that would make a PreBootstrap hook effectively just a validateMyConfig hook, and even then the validation would be limited to sanity checks because we won't have any reference point for what is or isn't a valid config. So I think it's fair to expect the caller to validate the config before sending it through.

Copy link

@UtkarshBhatthere UtkarshBhatthere left a comment

Choose a reason for hiding this comment

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

LGTM!

@markylaing
Copy link
Contributor Author

@masnax I think I've made the changes you've requested.

Copy link
Contributor

@masnax masnax left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@masnax masnax merged commit 031b9c1 into canonical:main Nov 23, 2023
2 checks passed
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.

3 participants