Skip to content
This repository has been archived by the owner on Dec 1, 2018. It is now read-only.

change label seperator from default comma to options #1367

Conversation

andyxning
Copy link
Contributor

@andyxning andyxning commented Nov 6, 2016

Currently, we use hard code comma as the label seperator for concatnating key-value labels. The label string is like:

labels:beta.kubernetes.io/arch:amd64,beta.kubernetes.io/os:linux,kubernetes.io/hostname:127.0.0.1

This is fine until we use Bosun as the alert system and use the group by labels to search for labels.

As currently, bosun use comma to split queried tag key and tag value. For example if the query expression used for query InfluxDB from Bosun is something like this:

$limit = avg(influx("k8s", '''SELECT mean(value) as value FROM "memory/limit" WHERE type = 'node' GROUP BY nodename, labels''', "${INTERVAL}s", "", ""))
$allocatable = avg(influx("k8s", '''SELECT mean(value) as value FROM "memory/node_allocatable" WHERE type = 'node' GROUP BY nodename, labels''', "${INTERVAL}s", "", ""))

Then, we will get tagkey and tag value string like this:

nodename=127.0.0.1,labels=beta.kubernetes.io/arch:amd64,beta.kubernetes.io/os:linux,kubernetes.io/hostname:127.0.0.1

When split by a comma, something wrong happened. we split it wrongly to this:

nodename=127.0.0.1
labels=labels:beta.kubernetes.io/arch:amd64
beta.kubernetes.io/os.linux
kubernetes.io/hostname:127.0.0.1

the last two tag key-value pairs is wrong. This will make bosun confused and panic with something like "panic: opentsdb: bad tag: beta.kubernetes.io/os:linux".

This PR will add a new command line --label-seperator for heapster. And, in order to keep backward compatibility, the default value for --label-seperator is comma. We can specify semicolon as the seperator to walk around the bosun restriction


This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands will still work. Regular contributors should join the org to skip this step.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

@andyxning
Copy link
Contributor Author

andyxning commented Nov 6, 2016

@piosz @AlmogBaku @huangyuqi

@andyxning andyxning mentioned this pull request Nov 7, 2016
@piosz piosz self-assigned this Nov 23, 2016
@piosz
Copy link
Contributor

piosz commented Nov 23, 2016

LGTM, please squash commits before merge

@piosz piosz added the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit b088903. Full PR test history.

The magic incantation to run this job again is @k8s-bot test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@andyxning andyxning force-pushed the change_labels_seperator_from_default_comma_to_options branch from b088903 to eeb137a Compare November 23, 2016 21:36
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 23, 2016
@andyxning andyxning force-pushed the change_labels_seperator_from_default_comma_to_options branch from eeb137a to c5ba5c3 Compare November 23, 2016 21:48
@andyxning
Copy link
Contributor Author

andyxning commented Nov 23, 2016

@piosz Done. PTAL.

@AlmogBaku
Copy link
Contributor

AlmogBaku commented Nov 23, 2016 via email

@piosz
Copy link
Contributor

piosz commented Nov 24, 2016

+1 to #1367 (comment)

@andyxning andyxning force-pushed the change_labels_seperator_from_default_comma_to_options branch from c5ba5c3 to b853f10 Compare November 29, 2016 12:09
@andyxning andyxning force-pushed the change_labels_seperator_from_default_comma_to_options branch from b853f10 to bdfa6cc Compare November 29, 2016 12:27
@andyxning
Copy link
Contributor Author

andyxning commented Nov 29, 2016

@AlmogBaku @piosz Doc about --label-seperator command line has been added to docs/storage-schema.md. PTAL.

@piosz
Copy link
Contributor

piosz commented Nov 29, 2016

@k8s-bot ok to test

@piosz
Copy link
Contributor

piosz commented Nov 29, 2016

Thanks @andyxning. I'll merge once I'll be able to make tests green.

@piosz piosz merged commit e19c9fb into kubernetes-retired:master Nov 29, 2016
@andyxning andyxning deleted the change_labels_seperator_from_default_comma_to_options branch November 29, 2016 15:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants