don't exec create keyspace if it exists #2032

Wing924 · 2020-01-24T08:19:31Z

What this PR does:

Don't execute CREATE KEYSPACE IF NOT EXISTS if the given keyspace has already existed.

The CREATE KEYSPACE IF NOT EXISTS statement requires CREATE permission on <all keyspaces> even if it exists. It's not common that normal users have that privilege.

ref. how to check keyspace exist: https://github.com/gocql/gocql/blob/master/cassandra_test.go#L85-L97

Which issue(s) this PR fixes:

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Wing924 <weihe924stephen@gmail.com>

pstibrany

Approving, based on the https://github.com/gocql/gocql/blob/95d072f1b5bbc604ce10f41ce1c914578592ed00/cassandra_test.go#L85 which shows that ErrNoConnectionsStarted is expected if keyspace doesn't exist. It's weirdly named error though.

pstibrany · 2020-01-24T13:55:31Z

Please update CHANGELOG.md as well.

pstibrany · 2020-01-24T13:56:07Z

And thank you for your contribution!

pracucci

Thanks for your contribution! I left a couple of small comments. Please remember to add an entry to the CHANGELOG.md and mention the PR number too (see the format of other entries there).

@VineethReddy02 May you test this PR against your test cluster based on Cassandra, please?

pracucci · 2020-01-24T14:23:44Z

pkg/chunk/cassandra/storage_client.go

@@ -70,7 +66,19 @@ func (cfg *Config) session() (*gocql.Session, error) {
 	cluster.ConnectTimeout = cfg.ConnectTimeout
 	cfg.setClusterConfig(cluster)

-	return cluster.CreateSession()
+	session, err := cluster.CreateSession()
+	if err != nil {


I would simplify this into a guard:

if err == nil { return session, nil }

pracucci · 2020-01-24T14:23:58Z

pkg/chunk/cassandra/storage_client.go

+		if err := cfg.createKeyspace(); err != nil {
+			return nil, errors.WithStack(err)
+		}
+		session, err = cluster.CreateSession()


Then you can simplify this into return cluster.CreateSession()

Signed-off-by: Wing924 <weihe924stephen@gmail.com>

pkg/chunk/cassandra/storage_client.go

VineethReddy02 · 2020-01-26T05:29:40Z

@pracucci I will test this PR and will update here.

VineethReddy02 · 2020-01-26T16:08:57Z

@pracucci @Wing924

Somehow I missed this issue while running cassandra locally.
https://github.com/cortexproject/cortex/blob/master/docs/guides/cortex-with-cassandra.md
In this doc, I am creating keyspace explicitly by cqlsh prompt. But my config.file has the same keyspace. which should ideally try to again create the keyspace. But I don't see any error log that you mentioned.
But when I don't have keyspace already created. It is throwing some irrelevant error. The changes in this PR is automatically creating the keyspace provided in the config.file

Wing924 · 2020-01-26T22:59:20Z

@VineethReddy02
The doc don't enable cassandra auth. Please test it with auth enabled.
ref. http://cassandra.apache.org/doc/latest/operating/security.html#enabling-password-authentication

pracucci

Please take a look at my comments. I think the current changes introduce a regression. Moreover, please do some manual tests and report the result (ie. Cortex logs); minimum tests to do:

Keyspace already created and no CREATE privilege
Keyspace NOT created and no CREATE privilege
Keyspace NOT created and CREATE privilege

pracucci · 2020-02-05T10:11:09Z

pkg/chunk/cassandra/storage_client.go

@@ -83,6 +79,14 @@ func (cfg *Config) session() (*gocql.Session, error) {
 	}
 	cfg.setClusterConfig(cluster)

+	session, err := cluster.CreateSession()
+	if err == nil || err != gocql.ErrNoConnectionsStarted {


Please add a comment to explain what ErrNoConnectionsStarted is.

pkg/chunk/cassandra/storage_client.go

Signed-off-by: Wing924 <weihe924stephen@gmail.com>

Wing924 · 2020-02-07T09:44:32Z

Manual Test Result

Summary

It works as expected.

Logs

Keyspace NOT created and no CREATE privilege

level=info ts=2020-02-07T09:15:30.567056987Z caller=cortex.go:240 msg=initialising module=server
level=info ts=2020-02-07T09:15:30.570438508Z caller=server.go:117 http=0.0.0.0:9009 grpc=0.0.0.0:9095 msg="server listening on addresses"
level=info ts=2020-02-07T09:15:30.57064428Z caller=cortex.go:240 msg=initialising module=runtime-config
level=info ts=2020-02-07T09:15:30.570667901Z caller=manager.go:63 msg="runtime config disabled: file not specified"
level=info ts=2020-02-07T09:15:30.57068235Z caller=cortex.go:240 msg=initialising module=overrides
level=info ts=2020-02-07T09:15:30.570690426Z caller=cortex.go:240 msg=initialising module=table-manager
level=error ts=2020-02-07T09:15:53.450998721Z caller=log.go:141 msg="error initializing cortex" err="User test_cortex has no CREATE permission on <all keyspaces> or any of its parents ..."
(exit with error)

Keyspace already created and no CREATE privilege

level=info ts=2020-02-07T09:24:47.68719324Z caller=cortex.go:247 msg=initialising module=runtime-config
level=info ts=2020-02-07T09:24:47.687254652Z caller=manager.go:63 msg="runtime config disabled: file not specified"
level=info ts=2020-02-07T09:24:47.687275176Z caller=cortex.go:247 msg=initialising module=server
level=info ts=2020-02-07T09:24:47.690203425Z caller=server.go:147 http=0.0.0.0:9009 grpc=0.0.0.0:9095 msg="server listening on addresses"
level=info ts=2020-02-07T09:24:47.690455183Z caller=cortex.go:247 msg=initialising module=ring
level=info ts=2020-02-07T09:24:47.690612906Z caller=cortex.go:247 msg=initialising module=overrides
level=info ts=2020-02-07T09:24:47.69064727Z caller=cortex.go:247 msg=initialising module=store
level=info ts=2020-02-07T09:24:47.691175592Z caller=client.go:212 msg="value is nil" key=collectors/ring index=1
level=info ts=2020-02-07T09:25:25.175172864Z caller=cortex.go:247 msg=initialising module=ingester
level=info ts=2020-02-07T09:25:25.179362475Z caller=cortex.go:247 msg=initialising module=distributor
level=info ts=2020-02-07T09:25:25.179461895Z caller=cortex.go:247 msg=initialising module=table-manager
level=info ts=2020-02-07T09:25:25.179556392Z caller=lifecycler.go:476 msg="not loading tokens from file, tokens file path is empty"
level=info ts=2020-02-07T09:25:25.180010436Z caller=lifecycler.go:500 msg="instance not found in ring, adding with no tokens" ring=ingester
level=info ts=2020-02-07T09:25:25.180313711Z caller=lifecycler.go:368 msg="auto-joining cluster after timeout" ring=ingester
level=info ts=2020-02-07T09:25:35.553827695Z caller=cortex.go:247 msg=initialising module=querier
level=info ts=2020-02-07T09:25:35.553988049Z caller=table_manager.go:220 msg="synching tables" expected_tables=2
level=info ts=2020-02-07T09:25:35.554530448Z caller=worker.go:72 msg="no address specified, not starting worker"
level=info ts=2020-02-07T09:25:35.554568037Z caller=cortex.go:247 msg=initialising module=all
level=info ts=2020-02-07T09:25:35.554582372Z caller=main.go:100 msg="Starting Cortex" version="(version=, branch=, revision=)"
level=info ts=2020-02-07T09:25:35.590321567Z caller=table_manager.go:364 msg="creating table" table=chunk_2614
...
(running)

Keyspace NOT created and CREATE privilege

level=info ts=2020-02-07T09:34:35.233093746Z caller=cortex.go:247 msg=initialising module=server
level=info ts=2020-02-07T09:34:35.235921256Z caller=server.go:147 http=0.0.0.0:9009 grpc=0.0.0.0:9095 msg="server listening on addresses"
level=info ts=2020-02-07T09:34:35.236107137Z caller=cortex.go:247 msg=initialising module=runtime-config
level=info ts=2020-02-07T09:34:35.236140945Z caller=manager.go:63 msg="runtime config disabled: file not specified"
level=info ts=2020-02-07T09:34:35.236160646Z caller=cortex.go:247 msg=initialising module=ring
level=info ts=2020-02-07T09:34:35.236227622Z caller=cortex.go:247 msg=initialising module=overrides
level=info ts=2020-02-07T09:34:35.236241035Z caller=cortex.go:247 msg=initialising module=table-manager
level=info ts=2020-02-07T09:34:35.236868018Z caller=client.go:212 msg="value is nil" key=collectors/ring index=1
2020-02-07 09:34:35.521554 I | error: failed to connect to 100.89.2.196:9042 due to error: Keyspace 'cortex_test3' does not exist
2020-02-07 09:34:35.658739 I | error: failed to connect to 100.78.39.31:9042 due to error: Keyspace 'cortex_test3' does not exist
2020-02-07 09:34:35.805070 I | error: failed to connect to 100.89.4.223:9042 due to error: Keyspace 'cortex_test3' does not exist
2020-02-07 09:34:35.993499 I | error: failed to connect to 100.78.39.84:9042 due to error: Keyspace 'cortex_test3' does not exist
2020-02-07 09:34:36.190253 I | error: failed to connect to 100.89.4.39:9042 due to error: Keyspace 'cortex_test3' does not exist
2020-02-07 09:34:36.308985 I | error: failed to connect to 100.89.2.196:9042 due to error: Keyspace 'cortex_test3' does not exist
2020-02-07 09:34:36.490566 I | error: failed to connect to 100.89.0.75:9042 due to error: Keyspace 'cortex_test3' does not exist
2020-02-07 09:36:09.814857 I | gocql: cluster schema versions not consistent: [2d2957e4-4456-3522-a945-0fa47f99ad4d 44bd4095-034c-342e-b99a-788ed1675b1b]
level=info ts=2020-02-07T09:36:28.548977067Z caller=cortex.go:247 msg=initialising module=distributor
level=info ts=2020-02-07T09:36:28.549158543Z caller=cortex.go:247 msg=initialising module=store
level=info ts=2020-02-07T09:36:28.549711683Z caller=table_manager.go:220 msg="synching tables" expected_tables=2
level=info ts=2020-02-07T09:36:28.58161796Z caller=table_manager.go:364 msg="creating table" table=chunk_2614
level=error ts=2020-02-07T09:36:43.549718646Z caller=pool.go:170 msg="error removing stale clients" err="empty ring"
level=info ts=2020-02-07T09:36:52.302659404Z caller=cortex.go:247 msg=initialising module=ingester
level=info ts=2020-02-07T09:36:52.303278785Z caller=cortex.go:247 msg=initialising module=querier
level=info ts=2020-02-07T09:36:52.303644135Z caller=worker.go:72 msg="no address specified, not starting worker"
level=info ts=2020-02-07T09:36:52.303688187Z caller=cortex.go:247 msg=initialising module=all
level=info ts=2020-02-07T09:36:52.303701327Z caller=main.go:100 msg="Starting Cortex" version="(version=, branch=, revision=)"
level=info ts=2020-02-07T09:36:52.307579708Z caller=lifecycler.go:476 msg="not loading tokens from file, tokens file path is empty"
level=info ts=2020-02-07T09:36:52.3076691Z caller=lifecycler.go:500 msg="instance not found in ring, adding with no tokens" ring=ingester
level=info ts=2020-02-07T09:36:52.308692624Z caller=lifecycler.go:368 msg="auto-joining cluster after timeout" ring=ingester
2020-02-07 09:37:29.148629 I | gocql: cluster schema versions not consistent: [e0684dff-d7ff-36b2-9cbc-deb8aae5583b 44bd4095-034c-342e-b99a-788ed1675b1b]
level=info ts=2020-02-07T09:37:29.148715011Z caller=table_manager.go:364 msg="creating table" table=index_2614
2020-02-07 09:38:29.466508 I | gocql: cluster schema versions not consistent: [d669c677-3fb1-3e72-8aaa-90de990af1ef 44bd4095-034c-342e-b99a-788ed1675b1b]
level=info ts=2020-02-07T09:39:36.821926199Z caller=table_manager.go:220 msg="synching tables" expected_tables=2
(running)

VineethReddy02 · 2020-02-07T17:03:52Z

Hey @Wing924 @pracucci
I just verified all the scenarios mentioned by @Wing924 from my end, All the mentioned scenarios work the same as @Wing924 mentioned.
Thanks @Wing924 for finding this issue and on fixing this :)

pracucci

LGTM 🎉 Thanks for the tests results!

Wing924 · 2020-02-11T09:25:13Z

test failed but not from part of my changes.

don't exec create keyspace if it exists

02579b4

Signed-off-by: Wing924 <weihe924stephen@gmail.com>

pstibrany approved these changes Jan 24, 2020

View reviewed changes

pracucci reviewed Jan 24, 2020

View reviewed changes

Wing924 added 2 commits January 25, 2020 09:10

fix

007236d

Signed-off-by: Wing924 <weihe924stephen@gmail.com>

Merge branch 'master' into dont_exec_create_keyspace_if_exist

00efd23

pstibrany reviewed Jan 25, 2020

View reviewed changes

pkg/chunk/cassandra/storage_client.go Show resolved Hide resolved

pstibrany approved these changes Jan 25, 2020

View reviewed changes

Merge branch 'master' into dont_exec_create_keyspace_if_exist

ef1f789

pracucci reviewed Feb 5, 2020

View reviewed changes

Wing924 added 2 commits February 7, 2020 17:24

Merge branch 'master' into dont_exec_create_keyspace_if_exist

10dea17

fix

1e6d24d

Signed-off-by: Wing924 <weihe924stephen@gmail.com>

pracucci approved these changes Feb 7, 2020

View reviewed changes

Merge branch 'master' into dont_exec_create_keyspace_if_exist

8835de3

gouthamve approved these changes Feb 11, 2020

View reviewed changes

gouthamve merged commit 220743a into cortexproject:master Feb 11, 2020

Wing924 deleted the dont_exec_create_keyspace_if_exist branch February 11, 2020 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

don't exec create keyspace if it exists #2032

don't exec create keyspace if it exists #2032

Uh oh!

Wing924 commented Jan 24, 2020 •

edited

Loading

pstibrany left a comment

pstibrany commented Jan 24, 2020

pstibrany commented Jan 24, 2020

pracucci left a comment

pracucci Jan 24, 2020

pracucci Jan 24, 2020

Uh oh!

VineethReddy02 commented Jan 26, 2020 •

edited

Loading

VineethReddy02 commented Jan 26, 2020 •

edited

Loading

Wing924 commented Jan 26, 2020

pracucci left a comment

pracucci Feb 5, 2020

Uh oh!

Wing924 commented Feb 7, 2020

VineethReddy02 commented Feb 7, 2020

pracucci left a comment

Wing924 commented Feb 11, 2020

don't exec create keyspace if it exists #2032

don't exec create keyspace if it exists #2032

Uh oh!

Conversation

Wing924 commented Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

pstibrany commented Jan 24, 2020

pstibrany commented Jan 24, 2020

pracucci left a comment

Choose a reason for hiding this comment

pracucci Jan 24, 2020

Choose a reason for hiding this comment

pracucci Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

VineethReddy02 commented Jan 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

VineethReddy02 commented Jan 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Wing924 commented Jan 26, 2020

pracucci left a comment

Choose a reason for hiding this comment

pracucci Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

Wing924 commented Feb 7, 2020

Manual Test Result

Summary

Logs

VineethReddy02 commented Feb 7, 2020

pracucci left a comment

Choose a reason for hiding this comment

Wing924 commented Feb 11, 2020

Wing924 commented Jan 24, 2020 •

edited

Loading

VineethReddy02 commented Jan 26, 2020 •

edited

Loading

VineethReddy02 commented Jan 26, 2020 •

edited

Loading