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.

Sign up for GitHub

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

support starting tidb-server with -advertise-address parameter #1859

Merged
merged 4 commits into from
Mar 5, 2020
Merged

support starting tidb-server with -advertise-address parameter #1859

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fce9b7f
start tidb-server with
LinuxGit Mar 4, 2020
086b832
add EnableAdvertiseAddress switch
LinuxGit Mar 4, 2020
c56bcc6
fix indent
LinuxGit Mar 4, 2020
0be7092
address comments
LinuxGit Mar 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions charts/tidb-cluster/templates/scripts/_start_tidb.sh.tpl
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,12 @@ then
tail -f /dev/null
fi

# Use HOSTNAME if POD_NAME is unset for backward compatibility.
POD_NAME=${POD_NAME:-$HOSTNAME}
LinuxGit marked this conversation as resolved.
Show resolved Hide resolved
ARGS="--store=tikv \
{{- if .Values.tidb.enableAdvertiseAddress | default false }}
--advertise-address=${POD_NAME}.${HEADLESS_SERVICE_NAME}.${NAMESPACE}.svc \
{{- end }}
--host=0.0.0.0 \
--path=${CLUSTER_NAME}-pd:2379 \
--config=/etc/tidb/tidb.toml
Expand Down
4 changes: 4 additions & 0 deletions charts/tidb-cluster/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,10 @@ tidb:
# annotations:
# cloud.google.com/load-balancer-type: Internal
separateSlowLog: true

# Add --advertise-address to TiDB's startup parameters
enableAdvertiseAddress: false

slowLogTailer:
image: busybox:1.26.2
resources:
Expand Down
4 changes: 4 additions & 0 deletions manifests/crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3782,6 +3782,10 @@ spec:
cluster-level updateStrategy if present Optional: Defaults to
cluster-level setting'
type: string
enableAdvertiseAddress:
description: 'Add --advertise-address to TiDB''s startup parameters
Optional: Defaults to false'
type: boolean
hostNetwork:
description: 'Whether Hostnetwork of the component is enabled. Override
the cluster-level setting if present Optional: Defaults to cluster-level
Expand Down
7 changes: 7 additions & 0 deletions pkg/apis/pingcap/v1alpha1/openapi_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 15 additions & 7 deletions pkg/apis/pingcap/v1alpha1/tidbcluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,14 @@ import (

const (
// defaultHelperImage is default image of helper
defaultHelperImage = "busybox:1.26.2"
defaultTimeZone = "UTC"
defaultEnableTLSCluster = false
defaultEnableTLSClient = false
defaultExposeStatus = true
defaultSeparateSlowLog = true
defaultEnablePVReclaim = false
defaultHelperImage = "busybox:1.26.2"
defaultTimeZone = "UTC"
defaultEnableTLSCluster = false
defaultEnableTLSClient = false
defaultExposeStatus = true
defaultSeparateSlowLog = true
defaultEnablePVReclaim = false
defaultEnableTiDBAdvertiseAddress = false
)

var (
Expand Down Expand Up @@ -342,6 +343,13 @@ func (tidb *TiDBSpec) IsTLSClientEnabled() bool {
return tidb.TLSClient != nil && tidb.TLSClient.Enabled
}

func (tidb *TiDBSpec) IsAdvertiseAddressEnabled() bool {
if tidb.EnableAdvertiseAddress == nil {
return defaultEnableTiDBAdvertiseAddress
}
return *tidb.EnableAdvertiseAddress
}

func (tidb *TiDBSpec) IsUserGeneratedCertificate() bool {
return tidb.IsTLSClientEnabled() && tidb.TLSClient.SecretName != ""
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/pingcap/v1alpha1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,11 @@ type TiDBSpec struct {
// +optional
BinlogEnabled *bool `json:"binlogEnabled,omitempty"`

// Add --advertise-address to TiDB's startup parameters
// Optional: Defaults to false
// +optional
EnableAdvertiseAddress *bool `json:"enableAdvertiseAddress,omitempty"`

// MaxFailoverCount limit the max replicas could be added in failover, 0 means unlimited
// Optional: Defaults to 0
// +kubebuilder:validation:Minimum=0
Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/pingcap/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 10 additions & 4 deletions pkg/manager/member/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,12 @@ then
tail -f /dev/null
fi

# Use HOSTNAME if POD_NAME is unset for backward compatibility.
POD_NAME=${POD_NAME:-$HOSTNAME}
ARGS="--store=tikv \
{{- if .EnableAdvertiseAddress }}
--advertise-address=${POD_NAME}.${HEADLESS_SERVICE_NAME}.${NAMESPACE}.svc \
{{- end }}
--host=0.0.0.0 \
--path=${CLUSTER_NAME}-pd:2379 \
--config=/etc/tidb/tidb.toml
Expand All @@ -74,10 +79,11 @@ exec /tidb-server ${ARGS}
`))

type TidbStartScriptModel struct {
ClusterName string
EnablePlugin bool
PluginDirectory string
PluginList string
ClusterName string
EnableAdvertiseAddress bool
EnablePlugin bool
PluginDirectory string
PluginList string
}

func RenderTiDBStartScript(model *TidbStartScriptModel) (string, error) {
Expand Down
41 changes: 32 additions & 9 deletions pkg/manager/member/tidb_member_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,10 +456,11 @@ func getTiDBConfigMap(tc *v1alpha1.TidbCluster) (*corev1.ConfigMap, error) {

plugins := tc.Spec.TiDB.Plugins
startScript, err := RenderTiDBStartScript(&TidbStartScriptModel{
ClusterName: tc.Name,
EnablePlugin: len(plugins) > 0,
PluginDirectory: "/plugins",
PluginList: strings.Join(plugins, ","),
ClusterName: tc.Name,
EnableAdvertiseAddress: tc.Spec.TiDB.IsAdvertiseAddressEnabled(),
EnablePlugin: len(plugins) > 0,
PluginDirectory: "/plugins",
PluginList: strings.Join(plugins, ","),
})
if err != nil {
return nil, err
Expand Down Expand Up @@ -580,6 +581,7 @@ func getNewTiDBHeadlessServiceForTidbCluster(tc *v1alpha1.TidbCluster) *corev1.S
func getNewTiDBSetForTidbCluster(tc *v1alpha1.TidbCluster, cm *corev1.ConfigMap) *apps.StatefulSet {
ns := tc.GetNamespace()
tcName := tc.GetName()
headlessSvcName := controller.TiDBPeerMemberName(tcName)
baseTiDBSpec := tc.BaseTiDBSpec()
instanceName := tc.GetInstanceName()
tidbConfigMap := controller.MemberConfigMapName(tc, v1alpha1.TiDBMemberType)
Expand Down Expand Up @@ -712,10 +714,22 @@ func getNewTiDBSetForTidbCluster(tc *v1alpha1.TidbCluster, cm *corev1.ConfigMap)
slowLogFileEnvVal = slowQueryLogFile
}
envs := []corev1.EnvVar{
{
LinuxGit marked this conversation as resolved.
Show resolved Hide resolved
Name: "NAMESPACE",
ValueFrom: &corev1.EnvVarSource{
FieldRef: &corev1.ObjectFieldSelector{
FieldPath: "metadata.namespace",
},
},
},
{
Name: "CLUSTER_NAME",
Value: tc.GetName(),
},
{
Name: "HEADLESS_SERVICE_NAME",
Value: headlessSvcName,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

any changes on the statefulset will trigger rolling upgrading when people upgrade tidb-operator 1.1 from 1.0. if we require no upgrades in upgrading operator, we should find a new way.

Copy link
Contributor

Choose a reason for hiding this comment

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

like hostNetwork feature, add a switch?

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think? cc @weekface @Yisaer @aylei

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I've added side effects in PR description. I'd like to add a switch if we need backward compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

another solution is that operator 1.1 doesn't apply these new changes on tidb clusters managed by operator 1.0. to achieve this, we must maintain a version label on tidb cluster CR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, this behavior depends on the tidb version, which is available in the tc, can we check the version of tidb and decide if the sts.spec should be updated? @cofyc @LinuxGit

Copy link
Contributor

Choose a reason for hiding this comment

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

TiDB has this flag since 2.1, I'm not sure if we have clients using the TiDB before 2.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cofyc I think I could use https://godoc.org/github.com/coreos/go-semver/semver pkg to compare version. latest is not a valid semantic versioning, I need to deal with it. I'm not sure if there're other tags that can't compare.
I could set a env variable. So startup script in Helm doesn't need to compare version.
@tennix @weekface Do you think it need to be compatible with versions before v2.1.0?

Copy link
Member

Choose a reason for hiding this comment

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

The tidb 2.1 is not actively maintained, so I think it's ok to remove support for tidb 2.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, the TiDB 2.1 has this flag. TiDB before 2.1 is not supported.

{
Name: "TZ",
Value: tc.Spec.Timezone,
Expand All @@ -734,6 +748,20 @@ func getNewTiDBSetForTidbCluster(tc *v1alpha1.TidbCluster, cm *corev1.ConfigMap)
if tc.IsTLSClusterEnabled() {
scheme = corev1.URISchemeHTTPS
}

podSpec := baseTiDBSpec.BuildPodSpec()
if baseTiDBSpec.HostNetwork() {
podSpec.DNSPolicy = corev1.DNSClusterFirstWithHostNet
envs = append(envs, corev1.EnvVar{
LinuxGit marked this conversation as resolved.
Show resolved Hide resolved
Name: "POD_NAME",
ValueFrom: &corev1.EnvVarSource{
FieldRef: &corev1.ObjectFieldSelector{
FieldPath: "metadata.name",
},
},
})
}

containers = append(containers, corev1.Container{
Name: v1alpha1.TiDBMemberType.String(),
Image: tc.TiDBImage(),
Expand Down Expand Up @@ -766,16 +794,11 @@ func getNewTiDBSetForTidbCluster(tc *v1alpha1.TidbCluster, cm *corev1.ConfigMap)
},
})

podSpec := baseTiDBSpec.BuildPodSpec()
podSpec.Containers = containers
podSpec.Volumes = vols
podSpec.SecurityContext = podSecurityContext
podSpec.InitContainers = initContainers

if baseTiDBSpec.HostNetwork() {
podSpec.DNSPolicy = corev1.DNSClusterFirstWithHostNet
}

tidbLabel := label.New().Instance(instanceName).TiDB()
podAnnotations := CombineAnnotations(controller.AnnProm(10080), baseTiDBSpec.Annotations())
stsAnnotations := getStsAnnotations(tc, label.TiDBLabelVal)
Expand Down