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

Explicitly set bootstrap flag #327

Closed
wants to merge 5 commits into from
Closed

Conversation

masnax
Copy link
Contributor

@masnax masnax commented Jun 20, 2024

We often infer whether or not we are bootstrapping by checking the systems map for the local system. However now that we can re-use clusters from existing systems, we can't always rely on the local member being the bootstrapper.

Instead, this PR explicitly sets whether or not we are bootstrapping based on the command we are running.

Additionally, there is a commit that cleans up the network questions. Before, we used to apply the FAN network first and then overwrite it with the OVN network. This is a bit inefficient and will break with adding services (#260) so this PR splits the network questions into two functions. First we try to create the OVN network (or return nil). Then if weren't able to do that, we try to create the FAN network.

@@ -148,8 +148,7 @@ func (c *CmdControl) askAddress(autoSetup bool, listenAddr string) (string, *net
return listenAddr, iface, subnet, nil
}

func (c *CmdControl) askDisks(sh *service.Handler, systems map[string]InitSystem, autoSetup bool, wipeAllDisks bool) error {
_, bootstrap := systems[sh.Name]
func (c *CmdControl) askDisks(sh *service.Handler, systems map[string]InitSystem, autoSetup bool, wipeAllDisks bool, bootstrap bool) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is totally a nit, but the sum of these two/three (autoSetup and bootstrap (and wipeAllDisks)) seems like it's being passed around a lot here. It would be more readable at the call site if this were factored into some kind of AskFlags structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's true. A lot of this config (even the systems map) should probably be part of the receiver of these methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added 2 commits here (though it might not be the best place for it) to add a new initConfig struct.

This holds all the arguments that are passed to every helper, so that they can all be receivers of that type and access the fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, this looks better.

@masnax masnax force-pushed the ovn-reorg branch 3 times, most recently from 69ff3a8 to 8c5033e Compare June 21, 2024 22:01
Relying on the system map having the local entry to determine if we need
to bootstrap is not always accurate. So instead we can just set what we
expect given the command we are running.

The exception is the preseed case where the system map is not modified
from what is supplied by the preseed.

Signed-off-by: Max Asnaashari <[email protected]>
It was a bit weird that we generate a FAN network and then overwrite it.
Especially now that we need to check existing cluster members,
this is a bit more expensive.

So this refactors askNetwork into two helpers:
askOVNNetwork which sets up the OVN network, then if no network is
created by that helper, we fallback to the FAN implementation.

Signed-off-by: Max Asnaashari <[email protected]>
@masnax masnax closed this Jun 25, 2024
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