MINOR: MiniKdc JVM shutdown hook fix

[rondagostino]

When started as a separate process (which happens only in system tests), the shutdown hook for MiniKdc was running immediately, which causes tests that depend on it for Kerberos to fail. The system tests in zookeeper_security_upgrade_test.py, for example, fail because ZooKeeper is unable to contact the KDC.

I ran the above system test locally and confirmed it failed prior to the fix and succeeded after the fix:

WITHOUT FIX:

$ ducktape tests/kafkatest/tests/core/zookeeper_security_upgrade_test.py
[INFO  - 2020-01-13 09:19:04,249 - runner_client - log - lineno:240]: RunnerClient: kafkatest.tests.core.zookeeper_security_upgrade_test.ZooKeeperSecurityUpgradeTest.test_zk_security_upgrade.security_protocol=PLAINTEXT: Summary: Zookeeper node failed to start
Traceback (most recent call last):
...
test_id:    kafkatest.tests.core.zookeeper_security_upgrade_test.ZooKeeperSecurityUpgradeTest.test_zk_security_upgrade.security_protocol=PLAINTEXT
status:     FAIL

WITH FIX:

$ ducktape tests/kafkatest/tests/core/zookeeper_security_upgrade_test.py
[INFO:2020-01-13 09:24:08,072]: RunnerClient: kafkatest.tests.core.zookeeper_security_upgrade_test.ZooKeeperSecurityUpgradeTest.test_zk_security_upgrade.security_protocol=PLAINTEXT: PASS
...
test_id:    kafkatest.tests.core.zookeeper_security_upgrade_test.ZooKeeperSecurityUpgradeTest.test_zk_security_upgrade.security_protocol=PLAINTEXT
status:     PASS

[ijuma]

Thanks for finding and fixing the issue. Could we add a unit or integration test that verifies the fix? That will help us not regress in the same way in the future.

[ijuma]

ok to test

[rondagostino]

Thanks for finding and fixing the issue. Could we add a unit or integration test that verifies the fix? That will help us not regress in the same way in the future.

@ijuma I can think of two ways but not sure which one (if either) is appropriate.

1. The brute force approach. Actually build and start a separate MiniKdc process, and test to make sure it doesn't exit right away. This would entail setting up the CLASSPATH environment variable with all the required jars, creating appropriate Kerberos config files, etc. It seems like a lot of work, and it isn't going to test anything else that creates a JVM shutdown hook -- it would just apply to the MiniKdc class. We add JVM shutdown hooks in 7 different places by my count, and all but the MiniKdc follow this pattern (MiniKdc leverages KafkaThread instead of Thread):

      Runtime.getRuntime().addShutdownHook(new Thread("<threadName>") {
        override def run(): Unit = {...}
      })

I think the possibility of regressing exists in the MiniKdc case because KafkaThread has so many overloaded constructors that it is possible to mistakenly not pass in the Runnable.

A similar potential for mistake exists with the other 6 cases (that create a Thread rather than a KafkaThread) as well because Thread isn't abstract; it is possible to mistakenly not override the run() method with this syntactically valid code:

      Runtime.getRuntime().addShutdownHook(new Thread("<threadName>") {
        miniKdc.stop() // no override def run() = good syntax but bad semantics
      })

Which leads us to the second possibility:

2. Define a separate utility method that creates a JVM shutdown hook based on a provided instance of Runnable and a thread name:

  def addShutdownHook(threadName: String, runnable: Runnable): Unit = {
    Runtime.getRuntime.addShutdownHook(new KafkaThread(threadName, runnable, false))
  }

Such a utility method eliminates the possibility of mistakenly not overriding the run() method because this code is syntactically invalid:

addShutdownHook("minikdc-shutdown-hook", {miniKdc.stop()}) // does not compile

Static type checking that forces us to write it correctly:

SomeUtilityObject.addShutdownHook("minikdc-shutdown-hook", new Runnable() {override def run = miniKdc.stop()})

A test would make sure that whatever is passed in doesn't actually run right away.

A process that leverages the method wouldn't need so much configuration (just the test jar on the CLASSPATH and no Kerberos configs), so if we wanted to we could actually test that the Runnable in fact really does run when the JVM shuts down by creating a process and making the shutdown hook create a file or something like that when we kill the process.

A disadvantage of this second approach is that we won't test that MiniKdc (or anything else) actually applies a shutdown hook -- we would only be testing that any hook -- if applied -- will be called at the right time.

(2) seems like the better choice, but since shutdown hooks are generally difficult to test I wanted to confirm that the approach of writing the utility method seems reasonable and worth it before moving ahead.

[cmccabe]

I'm not sure that having a separate utility method is really that much more error-proof than having a constructor for KafkaThread like we do now. After all, you could always forget to call the utility method.

Since this is tested through ducktape, maybe that's enough for now? Unless we really want to add a junit test of spawning this thing in a separate JVM...

[ijuma]

Can you not call start and check if MiniKdc is not stopped at the end of the method call?

[rondagostino]

@ijuma Do you mean pass in something at the command line (e.g. --testing-do-not-start) that will cause private def start(...) to only register the shutdown hook? That seems kind of contrived to me and doesn't address any of the other 6 places where shutdown hooks are used.

I was tending to agree more with @cmccabe, that maybe this isn't worth trying to test, but it did occur to me that creating a utility method and doing in-process testing (i.e. confirm the shutdown hook is not being called immediately) would be quick to implement, and doing so would provide the static type checking benefits relatively cheaply. Out-of-process testing to confirm that the utility method is actually registering the hook seems like overkill, but we could creating something akin to kafka.util.Exit(e.g. kafka.util.Runtime) to confirm it -- this would also be relatively inexpensive to do.

It is expensive to confirm that anything spawning as a process is adding the hook. Maybe we just skip this part and optimize for cost/benefit as opposed to perfection?

[ijuma]

@rondagostino I mean simply calling start in a test. No forking needed. Since stop will be called immediately, MiniKdc will be closed soon after the method returns. What am I missing?

[rondagostino]

@ijuma That method is defined as private def start(...) on object MiniKdc. Would we have to change it to be public instead? We would also have to make a stop() method so we could tear down at the end of the test.

[ijuma]

You can make it private[minikdc]. You don't really need to add a stop method since the returned MiniKdc has a stop method.

[rondagostino]

@ijuma I'm think a two-pronged approach would be quick and effective:

1. Address the root cause of this particular occurrence of the problem -- ineffective static type checking -- via a separate utility method on the Exit class (Exit.addShutdownHook(name: String, runnable: Runnable)` that we can quickly test. Make all 7 cases leverage the new method.

2. Add a test for the MinKdc class itself to make sure the JVM doesn't exit.

Unless I hear otherwise I'll go ahead with both.

[ijuma]

Sounds good. I agree that a utility method that takes a Runnable instead of a Thread is useful.

[rondagostino]

@ijuma I changed the 7 Scala classes that were calling Runtime.getRuntime().addShutdownHook() and made them invoke kafka.utils.Exit.addShutdownHook() instead. I also changed the 20 instances of Java classes that were calling Runtime.getRuntime().addShutdownHook() and made them invoke org.apache.kafka.common.utils.Exit.addShutdownHook() instead.

There were 5 Java instances under streams/example/... and 3 Java instances under streams/quickstart/... that I did not modify; I thought perhaps these are supposed to represent code snippets that people can copy and it might be best for that code to not invoke non-public APIs.

[rondagostino]

retest this please

[ijuma]

Should we use a by-name parameter instead?

[rondagostino]

I don't think so because the runnable is dereferenced once, immediately, and the code passes it to the underlying Java implementation org.apache.kafka.common.utils.Exit

[ijuma]

I don't understand why any of that disqualifies a by name parameter. The benefit of using a by-name is that the caller code becomes cleaner (no need for () => ...).

[rondagostino]

I got it working with a by-name parameter, and the below test passes. Is this what you were thinking? If so, I can add a new commit with the changes for a better look.

  @Test
  def shouldNotInvokeShutdownHookImmediately(): Unit = {
    val value = "value"
    val array:Array[Any] = Array(value)

    def sideEffect(): Unit = {
      // mutate the first element
      array(0) = array(0).toString + array(0).toString
    }
    Exit.addShutdownHook(sideEffect) // by-name parameter, not invoked
    // make sure the first element wasn't mutated
    assertEquals(value, array(0))
    Exit.addShutdownHook(sideEffect()) // by-name parameter, not invoked
    // again make sure the first element wasn't mutated
    assertEquals(value, array(0))
    Exit.addShutdownHook(sideEffect, Some("message")) // by-name parameter, not invoked
    // make sure the first element still isn't mutated
    assertEquals(value, array(0))
  }

[ijuma]

@rondagostino yeah

[ijuma]

Scala is an expression oriented language, there are no statements. This could be simply f or shutdownHook. Also, I think name should not be optional and should be the first parameter.

[rondagostino]

there are no statements
This could be simply f or shutdownHook

I was under the impression that statements were expressions that return no value (i.e. :Unit), but I could see how this may be technically incorrect. I'll rename it shutdownHook.

I think name should not be optional and should be the first parameter.

There are places where currently a name is not given, and the Thread class auto-generates a name when it is not supplied, but I don't see a problem with making it a mandatory first parameter and supplying a thread name in the places where one is not yet supplied to the shutdown hook thread; I will do that. I will switch the order in the Java Exit class as well (name first, Runnable second).

[ijuma]

Yeah, Unit in Scala is actually a type and () a value. So, no statements. :)

With regards to names, I personally think it's very useful to give names to threads and we should require it. I think you've done that for the Scala code. Can we do it for the Java code too?

[rajinisivaram]

@ijuma Looks like @rondagostino has addressed your comments. Does this PR need another full review or have you already reviewed the changes?

[ijuma]

@rajinisivaram I didn't do a proper review, just looked at the method definitions. So, it would probably be good for you to review.

[rondagostino]

@rajinisivaram The last commit addresses the last review comment from @ijuma; we now accept a thread name with a Runnable from Java clients. We don't check that the name is non-null and throw an IllegalArgumentException in that case, but I think it's fine -- the Thread class will auto-generate a name in such cases.

[rajinisivaram]

@rondagostino Thanks for the PR. Left a few minor comments, apart from that LGTM.

[rondagostino]

Thanks, @rajinisivaram. All set. I reverted the other format changes -- I'm not sure how that happened. Probably the IDE did it without me realizing it.

[rajinisivaram]

@rondagostino Thanks for the updates, merging to trunk.