Fix setting up connection to non-IP sockets #178

sylwiaszunejko · 2024-05-27T10:43:55Z

Before the fix using a custom Dialer which connects to unix socket would fail: scylladb/scylla-manager#3831 (comment).

This PR makes the code default to the defaultPort in case unix socket is used and we cannot get tcp address by conn.conn.RemoteAddr().(*net.TCPAddr). I also added test for that.

Fixes: #175

sylwiaszunejko · 2024-05-27T10:45:15Z

I am not sure how to access maintenance socket (cql.m). Mounting directories seem to not work properly (permission denied). I could use some advice here how to solve it.

sylwiaszunejko · 2024-05-31T13:25:36Z

v2.

I changed CI to use latest version of scylla for all tests
I granted permissions to socket file to resolve error messages

@avelanarius @Michal-Leszczynski

dkropachev · 2024-06-02T15:15:19Z

docker-compose.yml

@@ -2,7 +2,7 @@ version: "3.7"

 services:
  node_1:
-    image: ${SCYLLA_IMAGE}
+    image: scylladb/scylla-nightly


This is bad idea, this CICD needs to be stable.

I think that scylla-enterprise-nightly:latest-enterprise is more stable and still allows for testing the newest release without changing docker tags in every PR.

before this PR you could test it by SCYLLA_VERSION=scylla-enterprise-nightly:latest-enterpris ./integration.sh

This is perfect example of why you should not run cicd on nightly builds:
https://github.com/scylladb/gocql/actions/runs/9346882983/job/25722658483?pr=186

It is failing because image is broken, now you either ignore pipeline failure, or wait till working image is published.

I will create separate PR to use newer version of SCYLLA_IMAGE for all of the nodes and I will bump scylla version in github workflows

You can use "latest" tag which is the latest release we have (Not RC) in (scylladb/scylla not nightly).

dkropachev · 2024-06-02T15:19:32Z

testdata/recreate/table_golden.cql

@@ -42,7 +42,7 @@ CREATE TABLE gocqlx_table.monkeyspecies (
    AND max_index_interval = 2048
    AND memtable_flush_period_in_ms = 0
    AND min_index_interval = 128
-    AND read_repair_chance = 1
+    AND read_repair_chance = 0


Could you please clarify how this is related to the PR?

sylwiaszunejko · 2024-06-07T13:28:07Z

v3:
I changed the scylla version to 6.0.0 and now CI works.

sylwiaszunejko · 2024-06-07T13:28:22Z

@avelanarius Could you take a look?

dkropachev · 2024-06-07T17:34:36Z

control_integration_test.go

+	if err != nil {
+		panic(fmt.Sprintf("unable to create session: %v", err))
+	}
+


Shouldn't you ran a query to make sure session is operational ?

I could add it, but I thought it was not necessary cause the issue was discovered without it

True, but you know, lot's of things happening between session creation and query being executed, aren't we need to make sure that whole thing is working ?

I have added the queries

sylwiaszunejko · 2024-06-11T07:15:02Z

@avelanarius ping

dkropachev

Beside force_schema_commit_log issue, look greate.

dkropachev · 2024-06-12T16:35:43Z

testdata/config/scylla.yaml

@@ -8,5 +8,4 @@ client_encryption_options:
  keyfile: /etc/scylla/db.key
  truststore: /etc/scylla/ca.crt
  require_client_auth: true
-# when using 5.4.x we have to specify force_schema_commit_log option
-force_schema_commit_log: true


Have you deleted it intentionaly, since it is used by dirver-matrix I do not recommend deleting it

I think it was only necessary to specify it in 5.4.x. In the 6.0 it should be enabled be default

sylwiaszunejko · 2024-06-18T08:21:19Z

@avelanarius @roydahan Could you look at it?

wprzytula · 2024-06-19T07:23:31Z

testdata/config/scylla.yaml

  keyfile: /etc/scylla/db.key
  truststore: /etc/scylla/ca.crt
  require_client_auth: true
-# when using 5.4.x we have to specify force_schema_commit_log option
-force_schema_commit_log: true
+maintenance_socket: workdir


The addition of maintenance_socket here is not related to the commit description. Why is it here?

You are right it should be in the second commit

wprzytula · 2024-06-19T07:27:35Z

control_integration_test.go

+	err = createTable(sess, `DROP KEYSPACE IF EXISTS `+keyspace)
+	if err != nil {
+		t.Fatal("unable to drop keyspace:", err)
+	}


I'd say: "unable to drop keyspace if exists", because otherwise the message would imply unconditional dropping of a keyspace, and such could fail due to e.g. lack of such keyspace defined.

control_integration_test.go

Remove {dclocal_,}read_repair_chance options from tests as they are deprecated in scylladb/scylladb#18087. Remove `tablets` and `consistent-topology-feature` experimental flags as they are not longer experimental.

sylwiaszunejko requested a review from avelanarius May 27, 2024 10:48

sylwiaszunejko self-assigned this May 27, 2024

roydahan requested a review from Michal-Leszczynski May 29, 2024 15:01

sylwiaszunejko force-pushed the unix_socket branch 3 times, most recently from 2a3722a to a87184a Compare May 31, 2024 13:16

sylwiaszunejko marked this pull request as ready for review May 31, 2024 13:25

dkropachev reviewed Jun 2, 2024

View reviewed changes

Michal-Leszczynski approved these changes Jun 3, 2024

View reviewed changes

sylwiaszunejko force-pushed the unix_socket branch 2 times, most recently from 0f066e8 to b9620cb Compare June 7, 2024 13:16

sylwiaszunejko requested a review from roydahan June 7, 2024 13:28

dkropachev reviewed Jun 7, 2024

View reviewed changes

sylwiaszunejko force-pushed the unix_socket branch from b9620cb to cdcabe3 Compare June 11, 2024 07:07

dkropachev approved these changes Jun 12, 2024

View reviewed changes

sylwiaszunejko requested a review from dkropachev June 13, 2024 08:25

wprzytula reviewed Jun 19, 2024

View reviewed changes

sylwiaszunejko added 2 commits June 19, 2024 09:37

Bump scylla version to 6.0.0 and use it for all of the nodes

a209a0d

Remove {dclocal_,}read_repair_chance options from tests as they are deprecated in scylladb/scylladb#18087. Remove `tablets` and `consistent-topology-feature` experimental flags as they are not longer experimental.

Fix setting up connection to non-IP sockets

76878db

sylwiaszunejko force-pushed the unix_socket branch from cdcabe3 to 76878db Compare June 19, 2024 08:02

sylwiaszunejko requested a review from wprzytula June 19, 2024 08:03

wprzytula approved these changes Jun 19, 2024

View reviewed changes

sylwiaszunejko merged commit 790b1a8 into scylladb:master Jun 19, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix setting up connection to non-IP sockets #178

Fix setting up connection to non-IP sockets #178

sylwiaszunejko commented May 27, 2024 •

edited

Loading

sylwiaszunejko commented May 27, 2024

sylwiaszunejko commented May 31, 2024 •

edited

Loading

dkropachev Jun 2, 2024

Michal-Leszczynski Jun 3, 2024

dkropachev Jun 3, 2024

dkropachev Jun 3, 2024

sylwiaszunejko Jun 3, 2024

roydahan Jun 3, 2024 •

edited

Loading

dkropachev Jun 2, 2024

sylwiaszunejko commented Jun 7, 2024

sylwiaszunejko commented Jun 7, 2024

dkropachev Jun 7, 2024

sylwiaszunejko Jun 10, 2024

dkropachev Jun 10, 2024

sylwiaszunejko Jun 11, 2024

sylwiaszunejko commented Jun 11, 2024

dkropachev left a comment

dkropachev Jun 12, 2024

sylwiaszunejko Jun 13, 2024

sylwiaszunejko commented Jun 18, 2024

wprzytula Jun 19, 2024

sylwiaszunejko Jun 19, 2024

wprzytula Jun 19, 2024

Fix setting up connection to non-IP sockets #178

Fix setting up connection to non-IP sockets #178

Conversation

sylwiaszunejko commented May 27, 2024 • edited Loading

sylwiaszunejko commented May 27, 2024

sylwiaszunejko commented May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roydahan Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylwiaszunejko commented Jun 7, 2024

sylwiaszunejko commented Jun 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylwiaszunejko commented Jun 11, 2024

dkropachev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylwiaszunejko commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylwiaszunejko commented May 27, 2024 •

edited

Loading

sylwiaszunejko commented May 31, 2024 •

edited

Loading

roydahan Jun 3, 2024 •

edited

Loading