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

workaround issue #61 - adding missing configs to all examples of cluster mode #76

Merged

Conversation

hbobenicio
Copy link
Contributor

@hbobenicio hbobenicio commented Jul 24, 2020

According to #61 , if you have a spec.size > 1 and you're using a file store, if you do not define a spec.config, then the operator controller will fail to define the correct cluster_node_id for the CLI command of the container. IMHO the best fix is to change that logic in order to allow empty configs to just work by setting the correct cluster_node_id anyways. But for now, docs should at least not mislead users to this "misconfiguration".

… code correctly set cluster-node-id for cluster mode
@wallyqs wallyqs self-requested a review July 25, 2020 06:33
@wallyqs wallyqs self-assigned this Jul 25, 2020
@hbobenicio
Copy link
Contributor Author

@wallyqs If you prefer to not change the spec model to require a config key and instead fix the logic, I'm happy closing this PR to open a new one with the fix. The relevant piece of code is here (from controller.go):

func stanContainerCmd(o *stanv1alpha1.NatsStreamingCluster, pod *k8scorev1.Pod) []string {
    // ...
    if o.Spec.StoreType == "SQL" {
        // ...
    }  else if o.Spec.StoreType == "MEMORY" {
        // ...
    } else {
        // ...

        // FIX ME.
        // If o.Spec.Config is nil, isClustered := false.
        // This will prevent the condition below to set --cluster_node_id properly, even though o.Spec.Size > 1.
        // May it safely assume cluster mode when o.Spec.Size > 1?
        isClustered := o.Spec.Config != nil && (o.Spec.Size > 1 || o.Spec.Config.Clustered)
        
	if isClustered && !ftModeEnabled {
	    storeArgs = append(storeArgs, "-clustered")
	    storeArgs = append(storeArgs, fmt.Sprintf("--cluster_node_id=%q", pod.Name))
	}
    }
}

@wallyqs
Copy link
Member

wallyqs commented Jul 27, 2020

@hbobenicio OK I will merge the workaround as it already helps and proper fix can be done in separate. Thanks a lot for finding the bug!

@wallyqs wallyqs closed this Jul 27, 2020
@wallyqs wallyqs reopened this Jul 27, 2020
@wallyqs wallyqs merged commit cb7c460 into nats-io:master Jul 27, 2020
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