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

talosctl cluster create command refactor #10289

Open
Orzelius opened this issue Feb 4, 2025 · 1 comment · May be fixed by #10334
Open

talosctl cluster create command refactor #10289

Orzelius opened this issue Feb 4, 2025 · 1 comment · May be fixed by #10334

Comments

@Orzelius
Copy link
Contributor

Orzelius commented Feb 4, 2025

related: #10133, #10287

Current shortcomings

User facing

  • Often no error is provided when wrong provider's flag are passed
  • Sometimes it's hard to tell which flags are applicable for which providers

Developer side

  • Both docker and qemu clusters are created via the same flow
    • Same options are crafted and passed in both cases
    • Both providers take the same clusterRequest, but result in different clusters
  • Currently the logic that crafts the request and executes on it is tightly coupled making it hard to test
  • No unified validation step exists making it hard to tell which options are yet to be checked

Proposed solution

  • Group options and flags as follows: common, qemu-specific, docker-specific
  • Split out common logic between both providers
  • Only pass applicable options to relevant providers
  • Split out logic that does validation and the creation of the request data and create tests for it

Here's my go at it #10287

Please share any ideas or thoughts about the implementation.
I've already wrapped my head around most of the bits around the cluster creation logic so I'd love to work on this :)

@Orzelius
Copy link
Contributor Author

Orzelius commented Feb 5, 2025

Here's my RFC for the implementation:

Please let me know if you have any thoughts or ideas on how to implement this further. The proposed changes only effect the front end cli and require no changes to the provision package. cc @smira

1. Split options and flags as follows: common, qemu-specific and docker-specific

// CommonOps are the options common across all the providers.
type CommonOps struct {
	Talosconfig               string
	KubernetesVersion         string
        ...
}
// qemuOps are the qemu provider specific options
type qemuOps struct {
	nodeInstallImage             string
	nodeVmlinuzPath              string
        ...
}
// dockerOps are the options specific to the docker provider
type dockerOps struct {
	dockerHostIP      string
        ...
}

// Same for flags

// Result:
type createOps struct {
	common *CommonOps
	docker *dockerOps
	qemu   *qemuOps
}

type createFlags struct {
	common *pflag.FlagSet
	docker *pflag.FlagSet
	qemu   *pflag.FlagSet
}

2. Add two more commands under create
a. create qemu that only takes qemu flags and only creates a qemu cluster
b. create docker that only takes docker flags and only creates a docker cluster
c. (optional)create qemu-basic or something similar. Only takes basic qemu flags and only creates a qemu cluster
The create command itself can be kept as is for backwards compatibility for a while.

3. Split common logic (which is now encapsulated into the CommonOps type)

// partialClusterRequest is the provision.ClusterRequest with only common provider options applied.
// partialClusterRequest can be modified and then has to be handed to CreateCluster.
type partialClusterRequest = provision.ClusterRequest

type clusterMaker struct {
	options      CommonOps
	provisioner  provision.Provisioner
	talosVersion string

	request           provision.ClusterRequest
	provisionOpts     []provision.Option
	cfgBundleOpts     []bundle.Option
	genOpts           []generate.Option
	cidr4             netip.Prefix
	ips               [][]netip.Addr
	verionContract    *config.VersionContract
	inClusterEndpoint string
}

func NewClusterMaker(ops CommonOps, provisioner provision.Provisioner, talosVersion string) (
	clusterMaker, error,
) {
	cm := clusterMaker{}

	// Initialize clusterMaker and create the inital request based on the common options

	return cm, nil
}

func (cm *clusterMaker) GetPartialClusterRequest() partialClusterRequest {
	return cm.request
}

func (cm *clusterMaker) AddGenOps(genOpts ...generate.Option) {
}

func (cm *clusterMaker) AddProvisionOps(provisionOpts ...provision.Option) {
}

func (cm *clusterMaker) AddCfgBundleOpts(provisionOpts ...bundle.Option) {
}

// SetInClusterEndpoint allows to optionally change the inClusterEndpoint.
func (cm *clusterMaker) SetInClusterEndpoint(endpoint string) {
}

// CreateCluster finalizes the clusterRequest by rendering and applying configs.
// Then it creates the cluster and runs post create logic.
func (cm *clusterMaker) CreateCluster(request partialClusterRequest) error {
	// Apply config to the nodes in the partialClusterRequest
	// Create the cluster via the Provisioner
	// Run postcreate
	return nil
}

4. Split docker and qemu create logic
For example the docker cluster creation would be reduced to this.
Notice how the Docker specific logic only gets passed dockerOps as all common logic is handled via the clusterMaker.
Anything that happends before the clusterMaker.CreateCluster call can be easily unit tested.

func getDockerClusterMaker(ctx context.Context, cOps CommonOps, dOps dockerOps, provisioner provision.Provisioner) (
	clusterMaker, error,
) {
	talosversion := cOps.TalosVersion
	if talosversion == "" {
		parts := strings.Split(dOps.nodeImage, ":")
		talosversion = parts[len(parts)-1]
	}

	return NewClusterMaker(cOps, provisioner, talosversion)
}

func createDockerCluster(dOps dockerOps, cm clusterMaker) error {
	clusterReq := cm.GetPartialClusterRequest()
	cm.AddProvisionOps(provision.WithDockerPortsHostIP(dOps.dockerHostIP))

	portList := []string{}
	if dOps.ports != "" {
		portList = strings.Split(dOps.ports, ",")
		cm.AddProvisionOps(provision.WithDockerPorts(portList))
	}

	clusterReq.Image = dOps.nodeImage
	clusterReq.Network.DockerDisableIPv6 = dOps.dockerDisableIPv6

	for _, n := range clusterReq.Nodes {
		n.Mounts = dOps.mountOpts.Value()
		n.Ports = portList
	}

	return cm.CreateCluster(clusterReq)
}

edit: change to pointer receiver

@Orzelius Orzelius linked a pull request Feb 11, 2025 that will close this issue
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 a pull request may close this issue.

1 participant