Altinity / clickhouse-operator

Altinity Kubernetes Operator for ClickHouse creates, configures and manages ClickHouse clusters running on Kubernetes
https://altinity.com
Apache License 2.0
1.88k stars 459 forks source link

About Helm and some question about chop #620

Open TCeason opened 3 years ago

TCeason commented 3 years ago

@TCeason main parts of operator code contains in cmd and pkg directories, I you want to help you can try to help us with Helm chart feel free continue work with https://github.com/Altinity/clickhouse-operator/pull/358

Originally posted by @Slach in https://github.com/Altinity/clickhouse-operator/issues/619#issuecomment-747246135

This pr https://github.com/Altinity/clickhouse-operator/pull/358 is closed, so I will open a new issue to describe my question and just associated it so that will not annoy the committer.

TCeason commented 3 years ago

Yes, I saw some code in cmd/operator is the ch-op's entry function.

And some of the code in pkg dir is generated by code-generator.

And zz_generated.deepcopy.go is generated. But when the code was be generated? And I can not find the usage of dev/run_code_generator.sh

pkg šŸ‘† git:(master)  tree -L 1
.
ā”œā”€ā”€ apis   # some 
ā”œā”€ā”€ chop
ā”œā”€ā”€ client
ā”œā”€ā”€ controller
ā”œā”€ā”€ model
ā”œā”€ā”€ util
ā””ā”€ā”€ version

Now in my mind:

I'd like to try, but I'm just at the learning stage. Best wishes.

TCeason commented 3 years ago

https://github.com/Altinity/clickhouse-operator/issues/262

TCeason commented 3 years ago

In code

// isCHITemplateExt returns true in case specified file has proper extension for a CHI template config file
func (config *OperatorConfig) isCHITemplateExt(file string) bool {
    switch util.ExtToLower(file) {
    case ".yaml":
        return true
    case ".json":
        return true
    }
    return false
}
// Read CHI template files
config.CHITemplateFiles = util.ReadFilesIntoMap(config.CHITemplatesPath, config.isCHITemplateExt)

the config.CHITemplatesPath in yaml is chiTemplatesPath: templates.d and the file in templates.d is end with exaple

/etc/clickhouse-operator/templates.d # ls
001-templates.json.example             default-pod-template.yaml.example      default-storage-template.yaml.example  readme

So maybe the config.CHITemplateFiles is nil?

I find the code, the def of useTemplates is copy from chi.Spec.UseTemplates. And Is it defines at there? So how to use it?

        if len(chi.Spec.UseTemplates) > 0 {
        useTemplates = make([]chiv1.ChiUseTemplate, len(chi.Spec.UseTemplates))
        copy(useTemplates, chi.Spec.UseTemplates)

        // UseTemplates must contain reasonable data, thus has to be normalized
        n.normalizeUseTemplates(&useTemplates)
    }
TCeason commented 3 years ago

@Slach Hi, could you help me with this question? Iā€™m eager to receive your feedback.

In code

// isCHITemplateExt returns true in case specified file has proper extension for a CHI template config file
func (config *OperatorConfig) isCHITemplateExt(file string) bool {
  switch util.ExtToLower(file) {
  case ".yaml":
      return true
  case ".json":
      return true
  }
  return false
}
// Read CHI template files
config.CHITemplateFiles = util.ReadFilesIntoMap(config.CHITemplatesPath, config.isCHITemplateExt)

the config.CHITemplatesPath in yaml is chiTemplatesPath: templates.d and the file in templates.d is end with exaple

/etc/clickhouse-operator/templates.d # ls
001-templates.json.example             default-pod-template.yaml.example      default-storage-template.yaml.example  readme

So maybe the config.CHITemplateFiles is nil?

I find the code, the def of useTemplates is copy from chi.Spec.UseTemplates. And Is it defines at there? So how to use it?

        if len(chi.Spec.UseTemplates) > 0 {
      useTemplates = make([]chiv1.ChiUseTemplate, len(chi.Spec.UseTemplates))
      copy(useTemplates, chi.Spec.UseTemplates)

      // UseTemplates must contain reasonable data, thus has to be normalized
      n.normalizeUseTemplates(&useTemplates)
  }
Slach commented 3 years ago

Hello @TCeason

dev/run_code_generator.sh

this script run manually if you change something in pkg/apis

build manifest via build-clickhouse-operator-install-yaml.sh

yes it's a good idea, I think we need to add a separate script deploy/helm/build-clickhouse-operator-helm.sh where we create separate templates/*yaml.tpl with go-templates placeholders

the main goal of clickhouse-operator Helm chart should just install and configure clickhouse-operator CRD and related deployment, service etc. and nothing else

I think we need to create a separate ClickHouse installation Helm chart (builded with dev/build-clickhouse-clusters-helm.sh which should require main helm chart, for manipulation of kind: ClickhouseInstallation and kind: ClickhouseInstallationTemplate custom resources

also, I think we can use https://github.com/mikefarah/yq to generate Helm chart templates/*.yaml.tpl from deploy/operator/*.yaml and put some go-templates

/etc/clickhouse-operator/conf.d and /etc/clickhouse-operator/config.d, /etc/clickhouse-operator/users.d/

these folders used for Clickhouse server configuration files users.d/ will merge with /etc/clickhouse-server/users.xml config.d/ will merge with /etc/clickhouse-server/config.xml conf.d/ will merge with both, and doesn't use now

look to https://github.com/ClickHouse/ClickHouse/blob/master/programs/server/Server.cpp#L356 https://github.com/ClickHouse/ClickHouse/blob/40aa97d1cf1e97ba3e9860394c5604676d99ee8e/docs/en/operations/configuration_files.md

/etc/clickhouse-operator/templates.d

this folder use for store kind: ClickHouseInstallationTemplate yaml / json files which will apply directly to a kubernetes API server after each clickhouse-operator process start currently, this folder contains only examples and apply nothing to kubeapi URL

yes, we can use .ClickHouseInstallationTemplate.metadata.name from teamplates.d files in useTemplates section in kind: ClickHouseInstallation custom resource instances

TCeason commented 3 years ago

Dear Klimov, thanks for your suggestion . I will have a look at yq.

TCeason commented 3 years ago
helm lint 
==> Linting .
[INFO] Chart.yaml: icon is recommended
[ERROR] templates/clickhouse-operator-install-crd.yaml: the kind "apiextensions.k8s.io/v1beta1 CustomResourceDefinition" is deprecated in favor of "apiextensions.k8s.io/v1 CustomResourceDefinition"

Is there have some problems that we can not support k8s 1.16+?

The default field can be set when the Defaulting feature is enabled, which is the case with apiextensions.k8s.io/v1 CustomResourceDefinitions. 

Defaulting is in GA since 1.17 (beta since 1.16 with the CustomResourceDefaulting feature gate enabled, which is the case automatically for many clusters for beta features).

https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/

TCeason commented 3 years ago

And In file build-clickhouse-operator-install-yaml.sh, build part templated manifests twice.

https://github.com/Altinity/clickhouse-operator/blob/master/deploy/operator/build-clickhouse-operator-install-yaml.sh#L50

https://github.com/Altinity/clickhouse-operator/blob/master/deploy/operator/build-clickhouse-operator-install-yaml.sh#L98

And not generate a rbac.yaml just generate a template-rbac.yaml.

Is there has some other sense?

Now I want to use this arch:

.
ā”œā”€ā”€ Chart.yaml
ā”œā”€ā”€ templates
ā”‚   ā””ā”€ā”€ clickhouse-operator-install.yaml
ā””ā”€ā”€ values.yaml

And templates/clickhouse-operator-install.yaml will be generated from deploy/operator/clickhouse-operator-install.yaml.

What do you think about it? @Slach

Slach commented 3 years ago

I agree with your file layout and think your file layour should be present in deploy/helm/operator and the second chart should present in 'deploy/helm/cluster-management`

@sunsingerus could you explain to us differences between https://github.com/Altinity/clickhouse-operator/blob/master/deploy/operator/build-clickhouse-operator-install-yaml.sh#L50 and https://github.com/Altinity/clickhouse-operator/blob/master/deploy/operator/build-clickhouse-operator-install-yaml.sh#L98?

from my side it looks as copy\pasted code ;) but little differences look like files generated twice, first time without namespace, and with templated namespace as \$OPERATOR_NAMESPACE at the second time

TCeason commented 3 years ago
helm lint 
==> Linting .
[INFO] Chart.yaml: icon is recommended
[ERROR] templates/clickhouse-operator-install-crd.yaml: the kind "apiextensions.k8s.io/v1beta1 CustomResourceDefinition" is deprecated in favor of "apiextensions.k8s.io/v1 CustomResourceDefinition"

Is there have some problems that we can not support k8s 1.16+?

The default field can be set when the Defaulting feature is enabled, which is the case with apiextensions.k8s.io/v1 CustomResourceDefinitions. 

Defaulting is in GA since 1.17 (beta since 1.16 with the CustomResourceDefaulting feature gate enabled, which is the case automatically for many clusters for beta features).

https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/

will chop support apiextensions.k8s.io/v1?

alex-zaitsev commented 3 years ago

will chop support apiextensions.k8s.io/v1?

Do you have any issues with v1beta1? If we switch CRD to v1, that will mean dropping compatibility with k8s 1.15 and earlier.

TCeason commented 3 years ago

will chop support apiextensions.k8s.io/v1?

Do you have any issues with v1beta1? If we switch CRD to v1, that will mean dropping compatibility with k8s 1.15 and earlier.

Yes. Now I use helm 3.0 runs helm lint and then it returns an error.

[ERROR] templates/clickhouse-operator-install-crd.yaml: the kind "apiextensions.k8s.io/v1beta1 CustomResourceDefinition" is deprecated in favor of "apiextensions.k8s.io/v1 CustomResourceDefinition"

I think there are some compatibility issues.