Skip to content

roachtest: disable encryption on tests that can't handle it. #27087

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
Merged
merged 1 commit into from
Jul 5, 2018
Merged

roachtest: disable encryption on tests that can't handle it. #27087

merged 1 commit into from
Jul 5, 2018

Conversation

@mberhault
Copy link
Contributor

@mberhault mberhault commented Jun 30, 2018

Two cases make encryption always fail:

  • old versions (<= 2.0)
  • uses of debug commands that open the rocksdb instance (needs
    encryption flags)

Release note: None

@mberhault mberhault requested a review from tbg June 30, 2018 11:49
@cockroach-teamcity
Copy link
Member

cockroach-teamcity commented Jun 30, 2018

This change is Reviewable

@mberhault
Copy link
Contributor Author

mberhault commented Jun 30, 2018

I tried one full run of roachtest run --encrypt before this change. Only failures were:

--- FAIL: cdc/w=10/nodes=3/init=true [unstable] (240.79s)
--- FAIL: cdc/w=100/nodes=3/init=false [unstable] (612.48s)
--- FAIL: clearrange [unstable] (860.62s)
--- FAIL: backup2TB (917.96s)
--- FAIL: jepsen/bank/majority-ring-start-kill-2 [unstable] (71.08s)
--- FAIL: upgrade/oldVersion=v2.0.0/nodes=5 [unstable] (131.90s)
--- FAIL: version/mixedWith=v2.0.0/nodes=3 (172.72s)
--- FAIL: version/mixedWith=v2.0.0/nodes=5 (239.58s)

This change fixes cdc/*, upgrade/* and version/*.

I retried a clearrange, backup2TB and still failing but not due to encryption settings, getting the inspecting engines: store <no-attributes>=/mnt/data1/cockroach, last used with cockroach version v1.1, is too old for running version v2.0-7 (which requires data from v2.0 or later) error message instead.
Jepsen I haven't retried, I'm guessing it's due to a flake.

The end goal is to make sure roachtest run --encrypt passes as it should while only disabling tests that really can't handle encryption.

@mberhault mberhault force-pushed the marc/disable_roachtest_encryption branch from 28b06b6 to 50bf70e Compare July 3, 2018 08:52
@mberhault
Copy link
Contributor Author

mberhault commented Jul 4, 2018

ping?

tbg
tbg previously requested changes Jul 4, 2018
View reviewed changes
Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

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

Can you not skip these tests when the encrypt flag is set? That seems better than running them without encryption, which is kind of the opposite of what the user wanted.

@mberhault
Copy link
Contributor Author

mberhault commented Jul 4, 2018

Not trivially. We debated for a while about how to use the --encrypt flag for roachtest. We ended up settling for passing the flag through to roachprod unless it's overwritten in individual tests.

@tbg
Copy link
Member

tbg commented Jul 4, 2018 via email

@mberhault
Copy link
Contributor Author

mberhault commented Jul 5, 2018

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 5, 2018

👎 Rejected by code reviews

@mberhault mberhault dismissed tbg’s stale review July 5, 2018 05:44

This was LGTMed

roachtest: disable encryption on tests that can't handle it.
51bed48 
Two cases make encryption always fail:
- old versions (<= 2.0)
- uses of debug commands that open the rocksdb instance (needs
  encryption flags)

Release note: None
@mberhault mberhault force-pushed the marc/disable_roachtest_encryption branch from 50bf70e to 51bed48 Compare July 5, 2018 05:46
@mberhault
Copy link
Contributor Author

mberhault commented Jul 5, 2018

bors r+

craig bot pushed a commit that referenced this pull request Jul 5, 2018
Merge #27075 #27087
b18abeb 
27075: storage: use engine for os-level operations. r=mberhault a=mberhault

This is important when encryption is enabled as we need to make sure we
handle file metadata properly.
While `os.Remove` will leave cruft behind, `os.Link` won't copy the file
encryption settings from the original and will therefore make it
unreadable.

This fixes `RESTORE` which writes file to local disk using local
encryption settings then ingests them. The link bypassed the rocksdb Env
leaving the ingested file without any encryption settings attached (aka:
plaintext).

Release note: None

27087: roachtest: disable encryption on tests that can't handle it. r=mberhault a=mberhault

Two cases make encryption always fail:
- old versions (<= 2.0)
- uses of debug commands that open the rocksdb instance (needs
  encryption flags)

Release note: None

Co-authored-by: marc <marc@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Jul 5, 2018

Build succeeded

@craig craig bot merged commit 51bed48 into cockroachdb:master Jul 5, 2018
@mberhault mberhault deleted the marc/disable_roachtest_encryption branch July 5, 2018 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tbg tbg tbg left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mberhault @cockroach-teamcity @tbg