-
Notifications
You must be signed in to change notification settings - Fork 14k
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
KAFKA-3985: Transient system test failure ZooKeeperSecurityUpgradeTest.test_zk_security_upgrade.security_protocol #1973
Conversation
Thanks for the PR, this seems fine to me. @rajinisivaram and/or @granders, what do you think? |
I've also triggered this build for the PR branch: https://jenkins.confluent.io/job/system-test-kafka-branch-builder-2/130/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good. Left a minor comment.
self.ca_passwd = "test-ca-passwd" | ||
|
||
self.truststore_path = "/tmp/test.truststore.jks" | ||
self.truststore_dir = mkdtemp(dir="/tmp") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think truststore and ca could be stored in a single directory. The files are related and are created together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rajinisivaram done.
LGTM |
if os.path.exists(file): | ||
os.remove(file) | ||
# Register rmtree to run on exit | ||
atexit.register(rmtree, self.ca_and_truststore_dir) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isatexit
is the right approach here? The registered function will only run when the entire ducktape process finishes (i.e. after all ~215 tests finish). Use of the tempfile.mkdtemp
will still avoid path collisions from concurrent processes, so maybe it's good enough.
Another possibility that at least results in immediate cleanup:
put directory removal into clean_node
(but check the containing directory is present before removing to avoid errors)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@granders It is correct to clean up those files when the duck tape process finishes. I'd say it is good practice to not leave files behind even in the absence of collision.
I don't think we can clean up on clean_node
, we really want those files to live across test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sounds good
The check has failed because of a unit test, it is unrelated to the changes here. The system test branch builder run I posted above has succeeded, though. Thanks for the reviews @granders @rajinisivaram @ijuma . |
Thanks for the PR, LGTM. |
Merged to trunk and 0.10.1 branches. |
…t.test_zk_security_upgrade.security_protocol Author: Flavio Junqueira <fpj@apache.org> Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>, Geoff Anderson <geoff@confluent.io>, Ismael Juma <ismael@juma.me.uk> Closes #1973 from fpj/KAFKA-3985
No description provided.