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.

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

[DRAFT] ci: run all integration tests #139

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

muzarski
Copy link
Collaborator

@muzarski muzarski commented Jul 8, 2024

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
  • I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

@roydahan
Copy link
Collaborator

roydahan commented Jul 9, 2024

@muzarski seems like the change of scylla regressed the CI significantly (Previous one had 253 integration tests passing, this one has 457 tests but only 50 tests pass).
Can you please send another PR that changes scylla version to 5.4.8 so we can compare tests results?

@wprzytula
Copy link
Collaborator

@muzarski seems like the change of scylla regressed the CI significantly (Previous one had 253 integration tests passing, this one has 457 tests but only 50 tests pass). Can you please send another PR that changes scylla version to 5.4.8 so we can compare tests results?

This is most probably due to tablets being the default now. As tests assume token ring, we need to alter them to explicitly disable tablets for created keyspaces.

@roydahan
Copy link
Collaborator

roydahan commented Jul 9, 2024

@muzarski seems like the change of scylla regressed the CI significantly (Previous one had 253 integration tests passing, this one has 457 tests but only 50 tests pass). Can you please send another PR that changes scylla version to 5.4.8 so we can compare tests results?

This is most probably due to tablets being the default now. As tests assume token ring, we need to alter them to explicitly disable tablets for created keyspaces.

This is my assumption as well, but let's verify it first with a 5.4.8 run.

@muzarski
Copy link
Collaborator Author

muzarski commented Jul 9, 2024

Done - changed scylla version to 5.4.8

@muzarski
Copy link
Collaborator Author

muzarski commented Jul 9, 2024

Again, only 50 tests passed. Something weird happens with ccm when we try to use authentication:

ccm create --scylla -n 1:0 -i 127.0.0. -v release:5.4.8 -b --pwd-auth cpp-driver_3-0-8_1-0-password_authenticator
/home/runner/work/cpp-rust-driver/cpp-rust-driver/tests/src/integration/integration.cpp:191: Failure
Failed
ccm> ccm updateconf authenticator:PasswordAuthenticator
Usage: ccm create [options] cluster_name

ccm: error: no such option: --pwd-auth

ccm> ccm start --wait-other-notice --wait-for-binary-proto --jvm_arg=-Dcassandra.superuser_setup_delay_ms=0 --jvm_arg=--skip-wait-for-gossip-to-settle=0
unknown file: Failure
C++ exception with description "Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.12.4/x64/bin/ccm", line 74, in <module>
    cmd.run()
  File "/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/site-packages/ccmlib/cmds/cluster_cmds.py", line 676, in run
    if self.cluster.start(no_wait=self.options.no_wait,
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/site-packages/ccmlib/scylla_cluster.py", line 174, in start
    started = self.start_nodes(**kwargs)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/site-packages/ccmlib/scylla_cluster.py", line 139, in start_nodes
    p = node.start(update_pid=False, jvm_args=jvm_args,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/site-packages/ccmlib/scylla_node.py", line 700, in start
    scylla_process = self._start_scylla(args=args, marks=marks, update_pid=update_pid,
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/site-packages/ccmlib/scylla_node.py", line 343, in _start_scylla
    self.wait_for_binary_interface(from_mark=from_mark, process=self._process_scylla, timeout=t)
  File "/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/site-packages/ccmlib/node.py", line 540, in wait_for_binary_interface
    self.watch_log_for("Starting listening for CQL clients", **kwargs)
  File "/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/site-packages/ccmlib/node.py", line 506, in watch_log_for
    raise RuntimeError(f"The process is dead, returncode={process.returncode}")
RuntimeError: The process is dead, returncode=2
" thrown in SetUp().

@roydahan
Copy link
Collaborator

roydahan commented Jul 9, 2024

what version of ccm are we using? is it master?

@muzarski
Copy link
Collaborator Author

muzarski commented Jul 9, 2024

what version of ccm are we using? is it master?

pip3 install https://github.com/scylladb/scylla-ccm/archive/master.zip <- this is the installation instruction in CI

@muzarski
Copy link
Collaborator Author

muzarski commented Jul 9, 2024

I'll try to disable auth tests and see what happens.

@roydahan
Copy link
Collaborator

roydahan commented Jul 9, 2024

I don't understand where this "pwd-auth" is coming from.
I don't see it in the args of the command being used for scylla nor in ccm.
@fruch any idea?

@Lorak-mmk
Copy link
Collaborator

I can see this option present in original ccm: https://github.com/riptano/ccm/blob/3d84863d766a95fe1e37d779d12de2358a35c923/ccmlib/cmds/cluster_cmds.py#L95
but not in our fork.

@Lorak-mmk
Copy link
Collaborator

It was added in this commit, in 2016: riptano/ccm@8dc02c4
When was our ccm forked? Did we merge with upstream since then?

@wprzytula
Copy link
Collaborator

It was added in this commit, in 2016: riptano/ccm@8dc02c4 When was our ccm forked? Did we merge with upstream since then?

@fruch Do you know this by any chance?

@Lorak-mmk
Copy link
Collaborator

Lorak-mmk commented Jul 9, 2024

The first commit in git history that is not present in riptano/ccm but present in scylla-ccm is scylladb/scylla-ccm@eb60291
That would mean we forked in June 2015 - and we didn't merge upstream improvements.

Btw was Scylla originally called Urchin???

@Lorak-mmk
Copy link
Collaborator

It looks like this. First commit that mentions Scylla: scylladb/scylla-ccm@0966214
Later "urchin" was slowly replaced by "scylla", across multiple commits:
scylladb/scylla-ccm@4dcd81c
scylladb/scylla-ccm@e804e29
scylladb/scylla-ccm@493c4f0
scylladb/scylla-ccm@85c992a
and many others

@muzarski
Copy link
Collaborator Author

muzarski commented Jul 9, 2024

The stats from the run excluding auth tests for scylla 5.4.8:

[==========] 451 tests from 74 test cases ran. (1158673 ms total)
[  PASSED  ] 312 tests.
[  FAILED  ] 139 tests, listed below:

This means that 145 (139 + 6 x auth) tests still fail.

@fruch
Copy link

fruch commented Jul 9, 2024

It was added in this commit, in 2016: riptano/ccm@8dc02c4 When was our ccm forked? Did we merge with upstream since then?

@fruch Do you know this by any chance?

I know it has nothing todo with scylla what so ever,
here the commit introduced it in the cpp-driver:
scylladb/cpp-driver@11e61c5

any test that depend on whatever it would be, isn't relevant to scylla, and should be disabled (or removed)

@fruch
Copy link

fruch commented Jul 9, 2024

The first commit in git history that is not present in riptano/ccm but present in scylla-ccm is scylladb/scylla-ccm@eb60291 That would mean we forked in June 2015 - and we didn't merge upstream improvements.

we are not syncing ccm, since it was forked.
we do port relevant things if they are needed

Btw was Scylla originally called Urchin???

it was a long long time ago

@roydahan
Copy link
Collaborator

roydahan commented Jul 9, 2024

was Scylla originally called Urchin???

Yes, before it had an official name :)

@muzarski
Copy link
Collaborator Author

muzarski commented Sep 12, 2024

Rebased on master, to see current status of ITs with new version of CI.

cc: @dkropachev

@dkropachev dkropachev force-pushed the run_all_integration_tests branch 2 times, most recently from 79bdd83 to c2973e6 Compare September 13, 2024 00:49
@muzarski muzarski marked this pull request as draft October 2, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants