-
Notifications
You must be signed in to change notification settings - Fork 2
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
Version upgrade of influxDB for 1.x to 2.x #27
Conversation
Updates - New InfluxClient added to support 2.x - INIT env variables added to docker compose file to intial setup of influx db - Telegraf version upgrade - Env variables added in circleCI config - output.conf updated with outputs.influxdb_v2
.circleci/config.yml
Outdated
@@ -10,11 +10,11 @@ commands: | |||
- run: | |||
name : Take a nap | |||
command: | | |||
sleep 15 | |||
sleep 120 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this sleep
for? Generally, it's better to wait for the condition to become true instead of adding random delays that are unlikely to work in all situations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@@ -34,15 +35,36 @@ def assert_await(fxn, timeout=120, interval=10): | |||
|
|||
if last_assert: | |||
raise last_assert | |||
|
|||
def getClient(): | |||
influxToken = os.getenv("INFLUXDB_TOKEN", 'none') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test should fail fast if this is not set as there is no way to authenticate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I provide token as the default value ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If INFLUXDB_TOKEN
environment variable is not found, we should just fail the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test will fail automatically with the unauthenticated error message.
from nuodb_test_class import * | ||
from .nuodb_test_class import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is needed?
Dockerfile
Outdated
echo "deb https://repos.influxdata.com/debian stretch stable" > /etc/apt/sources.list.d/influxdb.list && \ | ||
# influxdata-archive_compat.key GPG Fingerprint: 9D539D90D3328DC7D6C8D3B9D8FF8E1F7DF8B07E | ||
curl -s https://repos.influxdata.com/influxdata-archive_compat.key > influxdata-archive_compat.key && \ | ||
echo '393e8779c89ac8d958f81f942f9ad7fb82a25e133faddaf92e15b16e6ac9ce4c influxdata-archive_compat.key' | sha256sum -c && cat influxdata-archive_compat.key | gpg --dearmor | tee /etc/apt/trusted.gpg.d/influxdata-archive_compat.gpg > /dev/null && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this 393e8779c89ac8d958f81f942f9ad7fb82a25e133faddaf92e15b16e6ac9ce4c
value coming from and why do we have to hardcode it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.influxdata.com/telegraf/v1.21/introduction/installation/#installation Provides these values
Dockerfile
Outdated
# influxdata-archive_compat.key GPG Fingerprint: 9D539D90D3328DC7D6C8D3B9D8FF8E1F7DF8B07E | ||
curl -s https://repos.influxdata.com/influxdata-archive_compat.key > influxdata-archive_compat.key && \ | ||
echo '393e8779c89ac8d958f81f942f9ad7fb82a25e133faddaf92e15b16e6ac9ce4c influxdata-archive_compat.key' | sha256sum -c && cat influxdata-archive_compat.key | gpg --dearmor | tee /etc/apt/trusted.gpg.d/influxdata-archive_compat.gpg > /dev/null && \ | ||
echo 'deb [signed-by=/etc/apt/trusted.gpg.d/influxdata-archive_compat.gpg] https://repos.influxdata.com/debian stable main' | tee /etc/apt/sources.list.d/influxdata.list && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to see the output of this command? Otherwise, just redirect it to the file without using tee
.
conf/outputs.conf
Outdated
# [[outputs.file]] | ||
# ## Files to write to, "stdout" is a specially handled file. | ||
# files = ["stdout"] | ||
## Destination bucket to write into. | ||
bucket = "${INFLUXDB_BUCKET}" | ||
|
||
# ## Data format to output. | ||
# ## Each data format has its own unique set of configuration options, read | ||
# ## more about them here: | ||
# ## https://github.com/influxdata/telegraf/blob/master/docs/DATA_FORMATS_OUTPUT.md | ||
# data_format = "json" | ||
## The value of this tag will be used to determine the bucket. If this | ||
## tag is not set the 'bucket' option is used as the default. | ||
bucket_tag = "db_tag" | ||
|
||
# [[outputs.prometheus_client]] | ||
# ## Address to listen on. | ||
# listen = ":9273" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the commented out plugins were here to serve as an example, and we should keep it (unless they don't work anymore).
docker-compose.yml
Outdated
@@ -30,11 +30,21 @@ services: | |||
command: ["nuodocker", "start", "te", "--db-name", "hockey", "--server-id", "nuoadmin1", | |||
"--servers-ready-timeout", "60"] | |||
influxdb: | |||
image: influxdb:1.8 | |||
image: influxdb:2.7.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we drop the patch version and just use 2.7
? The tag <major>.<minor>
tag will be set to the latest patch release within that release stream.
scripts/setup_influx.sh
Outdated
@@ -0,0 +1,6 @@ | |||
#! /bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to
#!/bin/sh
It doesn't look like we have use any bash-specific syntax, so might as well avoid the dependency.
tests/nuodb_test_class.py
Outdated
query = f""" | ||
import \"influxdata/influxdb/schema\" | ||
|
||
schema.measurements(bucket: \"{bucket}\") | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a Python3-ism? I'm guessing the f
prefix is what causes bucket
to be injected into the format string without have to write .format(bucket=bucket)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. F strings are sort of improved way of string formatting. It was introduced in Python 3.6
tests/nuodb_test_class.py
Outdated
|
||
def getMeasurenment(client, bucket): | ||
query = f""" | ||
import \"influxdata/influxdb/schema\" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the quotes here and and below escaped for InfluxDB or for Python? When I test this, it looks like the escape character is actually gratuitous, since the multi-line string is delimited by """
and "
is just a normal character:
>>> print("""
... "test"
... """)
"test"
>>> print("""
... \"test\"
... """)
"test"
DOCKER_INFLUXDB_INIT_BUCKET=[your_value] | ||
DOCKER_INFLUXDB_INIT_ADMIN_TOKEN=[your_value] | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to also document the environment variables expected by nuodb-collector
docker container in section Running NuoDB Collector.
INFLUXDB_TOKEN=...
INFLUXDB_BUCKET=...
INFLUXDB_ORG=...
@@ -14,7 +14,7 @@ commands: | |||
- run: | |||
name : Configure and run tests | |||
command: | | |||
pip install -r test_requirements.txt | |||
pip3 install -r test_requirements.txt | |||
pytest --junitxml=test_results/result.xml | |||
echo "Done" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try setting the environment variable PYTHONUNBUFFERED=1
and reverting the change to start up YCSB?
See https://docs.python.org/3/using/cmdline.html#envvar-PYTHONUNBUFFERED for more info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Thanks for processing this change!
Updates