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

Jump to bottom

feat(upgrade): extending upgrade command skeleton #19641

Merged
merged 1 commit into from
Oct 1, 2020
Merged

feat(upgrade): extending upgrade command skeleton #19641

merged 1 commit into from
Oct 1, 2020

Conversation

vlastahajek
Copy link
Contributor

@vlastahajek vlastahajek commented Sep 25, 2020 •
edited
Loading

This PR is 1st of 4 upgrade feature PRs and prepares common code for others.

Mainly introduces cli interface and options checks.

Minimal command line includes parameters for creating admin: influxd upgrade -b my-bucket -o my-org -u my-user -p my-password. If at least one of those options are not set, the interactive mode is launched and user is asked for entering values of missing options.

Complete help:

    Upgrades a 1.x version of InfluxDB by performing following actions:
      1. Reads 1.x config file and creates 2.x config file with matching options. Unsupported 1.x options are reported.
      2. Copies and upgrades 1.x database files.
      3. Creates a script for creating 1.x users and their permissions. This scripts needs to be revised and run manually after starting 2.x.

    If config file is not available, 1.x db folder (--v1-dir options) is taken as an input.
    Target 2.x database dir is specified by the --engine-path option. If changed, bolt path should to be changed as well.

Usage:
  influxd upgrade [flags]
  influxd upgrade [command]

Available Commands:
  v1-dump-meta Dump InfluxDB 1.x meta.db
  v2-dump-meta Dump InfluxDB 2.x influxd.bolt

Flags:
Flags:
  -m, --bolt-path string         path for boltdb database (default "/home/ubuntu/.influxdbv2/influxd.bolt")
  -b, --bucket string            primary bucket name
      --config-file string       optional: Custom InfluxDB 1.x config file path, else the default config file
  -e, --engine-path string       path for persistent engine files (default "/home/ubuntu/.influxdbv2/engine")
  -f, --force                    skip confirmation prompt
  -h, --help                     help for upgrade
      --log-path string          optional: custom log file path (default "/home/ubuntu/upgrade.log")
  -o, --org string               primary organization name
  -p, --password string          password for username
  -r, --retention string         optional: duration bucket will retain data. 0 is infinite. Default is 0.
      --security-script string   optional: generated security upgrade script path (default "/home/ubuntu/influxd-upgrade-security.sh")
  -t, --token string             optional: token for username, else auto-generated
  -u, --username string          primary username
      --v1-dir string            path to source 1.x db directory containing meta, data and wal sub-folders (default "/home/ubuntu/.influxdb")
  -v, --verbose                  verbose output (default true)

Use "influxd upgrade [command] --help" for more information about a command.

V1 and v2 dump commands are also extended about more info.

@vlastahajek vlastahajek changed the title feat: extending upgrade command skeleton feat(upgrade): extending upgrade command skeleton Sep 25, 2020
@vlastahajek vlastahajek marked this pull request as draft September 25, 2020 15:48
@vlastahajek
Copy link
Contributor Author

vlastahajek commented Sep 25, 2020 •
edited
Loading

As requested in the last comment of #19308 interactive mode will be added, but comments to actual code are welcome

@alespour alespour mentioned this pull request Sep 25, 2020
Merged
3 tasks
stuartcarnie
stuartcarnie requested changes Sep 28, 2020
View reviewed changes
Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

This is much easier to review, thank you!

cmd/influxd/upgrade/config.go Outdated
"go.uber.org/zap"
)

// Backups existing config file and updates it with upgraded config.
Copy link
Contributor

Choose a reason for hiding this comment

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

Per Effective Go, the convention for function doc commentary is to start the help string with the name of the function:

Doc comments work best as complete sentences, which allow a wide variety of automated presentations. The first sentence should be a one-sentence summary that starts with the name being declared.

// upgradeConfig will backup the existing config file...

cmd/influxd/upgrade/security.go
)

// Generates security upgrade script.
func generateSecurityScript(v1 *influxDBv1, dbBuckets map[string][]string, log *zap.Logger) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, start the doc comment with the name of the function

cmd/influxd/upgrade/upgrade.go Outdated
Comment on lines 105 to 109
if runtime.GOOS == "windows" {
defaultSsPath = filepath.Join(homeOrAnyDir(), "influxd-upgrade-security.cmd")
} else {
defaultSsPath = filepath.Join(homeOrAnyDir(), "influxd-upgrade-security.sh")
}
Copy link
Contributor

@stuartcarnie stuartcarnie Sep 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We are skipping windows for now, so I would not add any additional complexity to support it

cmd/influxd/upgrade/upgrade.go Outdated
if err != nil {
return err
}
log.Info("starting InfluxDB 1.x upgrade")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Log messages should be complete sentences and start with a capital. Please see the logging style guide

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, thanks for the info about the log guide. I don't remember where I got the wrong idea about starting with lower case.

cmd/influxd/upgrade/upgrade.go Outdated
if err != nil {
//remove all files
log.Info("database upgrade error, removing data")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Log messages should be complete sentences and start with a capital. Please see the logging style guide

cmd/influxd/upgrade/upgrade.go Outdated
// 3. create database in v2.meta
// v2.meta.CreateDatabase(newBucket.ID.String())
// copy shard info from v1.meta
log.Info("upgrade successfully completed. Start service now")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Log messages should be complete sentences and start with a capital. Please see the logging style guide

@vlastahajek vlastahajek force-pushed the feat/upgrade branch 3 times, most recently from 65b5c74 to 34ba3de Compare September 28, 2020 18:58
@vlastahajek vlastahajek marked this pull request as ready for review September 28, 2020 20:01
@alespour alespour mentioned this pull request Sep 29, 2020
Merged
3 tasks
@stuartcarnie
Copy link
Contributor

@vlastahajek golint is failing due to the following:

cmd/influxd/upgrade/upgrade_test.go:26:2: this value of err is never used (SA4006)

@vlastahajek
Copy link
Contributor Author

@stuartcarnie, I'm sorry, in the flood of emails about failed e2e tests I missed the one about build :(. It's fixed now.

stuartcarnie
stuartcarnie requested changes Sep 29, 2020
View reviewed changes
Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

This is much closer!

cmd/influx/bucket.go Outdated
@@ -3,6 +3,7 @@ package main
import (
"context"
"fmt"
internal2 "github.com/influxdata/influxdb/v2/internal"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: There is no reason to alias this to internal2 as there are no conflicts

cmd/influx/secret.go Outdated
@@ -3,6 +3,7 @@ package main
import (
"context"
"fmt"
internal2 "github.com/influxdata/influxdb/v2/internal"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no reason to alias this to internal2 as there are no conflicts.

cmd/influx/setup.go Outdated
@@ -4,18 +4,19 @@ import (
"context"
"errors"
"fmt"

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove space and run goimports -w

internal/input.go Outdated
@@ -0,0 +1,206 @@
package internal
Copy link
Contributor

Choose a reason for hiding this comment

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

Given these functions are only used by the cmd package, I'd prefer these be moved to

cmd/internal

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

internal/input.go Outdated
case input.ErrInterrupted:
os.Exit(1)
case errPasswordIsTooShort:
ui.Writer.Write(PromptWithColor("Password too short - minimum length is 8 characters!\n\r", ColorRed))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no need to add exclamation at the end: !

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, This came from original cmd/influx/setup.go :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I figured it probably did 👍

cmd/influxd/upgrade/upgrade_test.go Outdated
"path/filepath"
"testing"

"github.com/docker/docker/pkg/testutil/assert"
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid the proliferation of differing assert packages, please use stretchr/testify/assert

Suggested change
"github.com/docker/docker/pkg/testutil/assert"
"github.com/stretchr/testify/assert"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This, accidental import, no intention :(

cmd/influxd/upgrade/upgrade_test.go Outdated
Comment on lines 46 to 50
if err := cmd.Execute(); err == nil {
t.Fatal("Must fail")
} else {
assert.Contains(t, err.Error(), "1.x metadb error")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can additionally import "github.com/stretchr/testify/require" and then use:

Suggested change
if err := cmd.Execute(); err == nil {
t.Fatal("Must fail")
} else {
assert.Contains(t, err.Error(), "1.x metadb error")
}
err = cmd.Execute()
require.Error(t, err)
assert.Contains(t, err.Error(), "1.x metadb error")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I'm a bit confused from the InfluxDB tests code style. I've seen a lot of tests that prefer to use this than the require package. But, great, I personally like the require package. 👍

Copy link
Contributor

@stuartcarnie stuartcarnie Sep 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@vlastahajek there is a difference between require and assert.

When a require condition fails, the test will abort. When an assert condition fails, it will fail the test but continue execution. To put it another way, require is like t.Fatal and assert is like t.Error.

It is clear here, that we require an error and then following that we assert the error is the correct value.

If instead we wrote:

assert.Error(t, err)
assert.Contains(t, err.Error(), "1.x metadb error")

This test code would potentially panic on the assert.Contains line because err is nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I know. I use require in the influxdb-client-go tests heavy . I just felt that the server basic test style doesn't use require.

cmd/influxd/upgrade/upgrade_test.go Outdated
Comment on lines 53 to 55
if err != nil {
t.Fatal(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use require package:

Suggested change
if err != nil {
t.Fatal(err)
}
require.NoError(t, err)

cmd/influxd/upgrade/upgrade_test.go Outdated
Comment on lines 57 to 59
if err != nil {
t.Fatal(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use require package

stuartcarnie
stuartcarnie approved these changes Sep 30, 2020
View reviewed changes
Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

Looks good – thanks for breaking this down!

stuartcarnie
stuartcarnie requested changes Sep 30, 2020
View reviewed changes
Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

e2e tests are failing as a required import was removed from influxd/main.go

@stuartcarnie stuartcarnie self-requested a review September 30, 2020 21:07
stuartcarnie
stuartcarnie approved these changes Sep 30, 2020
View reviewed changes
Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

Everything is passing and it looks good 👍

@stuartcarnie
Copy link
Contributor

@vlastahajek you will need to rebase this before I can merge it

@vlastahajek
Copy link
Contributor Author

@stuartcarnie, the goal is to have a clean commit history?

@stuartcarnie
Copy link
Contributor

@vlastahajek yes and you will need to resolve whatever merge conflicts have arisen

@stuartcarnie stuartcarnie merged commit 999660a into influxdata:master Oct 1, 2020
@stuartcarnie stuartcarnie added the area/2.x OSS 2.0 related issues and PRs label Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartcarnie stuartcarnie stuartcarnie approved these changes

Assignees

@vlastahajek vlastahajek

@alespour alespour

Labels
area/2.x OSS 2.0 related issues and PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vlastahajek @stuartcarnie @alespour