[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r233266609 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/HadoopConfExecutorFeatureStepSuite.scala --- @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + *http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features + +import org.mockito.{Mock, MockitoAnnotations} +import org.mockito.Matchers.{eq => Eq} +import org.mockito.Mockito.when +import org.mockito.invocation.InvocationOnMock +import org.mockito.stubbing.Answer +import org.scalatest.BeforeAndAfter + +import org.apache.spark.{SparkConf, SparkFunSuite} +import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesExecutorSpecificConf, SparkPod} +import org.apache.spark.deploy.k8s.Constants.HADOOP_CONFIG_MAP_NAME +import org.apache.spark.deploy.k8s.features.hadooputils.HadoopBootstrapUtil + +class HadoopConfExecutorFeatureStepSuite extends SparkFunSuite with BeforeAndAfter { + private val hadoopConfMapName = "HADOOP_CONF_NAME" + private val sparkPod = SparkPod.initialPod() + + @Mock + private var kubernetesConf: KubernetesConf[KubernetesExecutorSpecificConf] = _ + + @Mock + private var hadoopBootstrapUtil: HadoopBootstrapUtil = _ + + before { +MockitoAnnotations.initMocks(this) +val sparkConf = new SparkConf(false) + .set(HADOOP_CONFIG_MAP_NAME, hadoopConfMapName) +when(kubernetesConf.sparkConf).thenReturn(sparkConf) + } + + test("bootstrapHadoopConf being applied to a base spark pod") { +when(hadoopBootstrapUtil.bootstrapHadoopConfDir( --- End diff -- > I think it's fine though that we want to test "We're calling this submodule with these parameters" I think that's a very restrictive view of testing. I prefer my previous interpretation: the goal of this step is to mount a volume if a certain config is set. Whether it does that by calling into another module or not, it doesn't matter. That's what should be tested. You're taking "white box testing" to an extreme here - you're testing specific lines of code in the implementation instead of testing the contract ("for these inputs generate these outputs"). If it's done via another module, and you want to have unit tests for that module too, that's fine. But then you get into your other question... Looking from the other side: if you change the implementation in a way that breaks this test, but keeps the actual functionality, what good was this test doing? > did we need a submodule to begin with? My answer to that question is no - and as part of my updates to SPARK-25815 I'll be removing that class. But that's kinda orthogonal to my point. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user mccheah commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r233257218 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/HadoopConfExecutorFeatureStepSuite.scala --- @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + *http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features + +import org.mockito.{Mock, MockitoAnnotations} +import org.mockito.Matchers.{eq => Eq} +import org.mockito.Mockito.when +import org.mockito.invocation.InvocationOnMock +import org.mockito.stubbing.Answer +import org.scalatest.BeforeAndAfter + +import org.apache.spark.{SparkConf, SparkFunSuite} +import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesExecutorSpecificConf, SparkPod} +import org.apache.spark.deploy.k8s.Constants.HADOOP_CONFIG_MAP_NAME +import org.apache.spark.deploy.k8s.features.hadooputils.HadoopBootstrapUtil + +class HadoopConfExecutorFeatureStepSuite extends SparkFunSuite with BeforeAndAfter { + private val hadoopConfMapName = "HADOOP_CONF_NAME" + private val sparkPod = SparkPod.initialPod() + + @Mock + private var kubernetesConf: KubernetesConf[KubernetesExecutorSpecificConf] = _ + + @Mock + private var hadoopBootstrapUtil: HadoopBootstrapUtil = _ + + before { +MockitoAnnotations.initMocks(this) +val sparkConf = new SparkConf(false) + .set(HADOOP_CONFIG_MAP_NAME, hadoopConfMapName) +when(kubernetesConf.sparkConf).thenReturn(sparkConf) + } + + test("bootstrapHadoopConf being applied to a base spark pod") { +when(hadoopBootstrapUtil.bootstrapHadoopConfDir( --- End diff -- There is a point of over-mocking, I agree. I think `KubernetesConf` shouldn't be mocked in most cases because it's just a structure. This is similar to not mocking something like a case class or a hash map. I also don't think we need to use Mockito Answers here - that could be done with stub implementations of the submodule. I didn't have a strong enough conviction of not using Mockito answers, but in general I think we should favor stub implementations over `Answer`; it's more readable. I think it's fine though that we want to test "We're calling this submodule with these parameters", because the rest of the module's correctness is unit-test covered in the unit tests of that submodule. The general premise we'd like to follow is that a unit test should only execute code that is in the class under test. In other words, in this concrete case, since `HadoopBootstrapUtil` is a separate class, no code in `HadoopBootstrapUtil` should be run as part of the test. The class under test is responsible for calling the utility submodule with certain arguments but is not concerned about what that submodule actually does. Thus the test also doesn't have to be concerned here about what that submodule actually does, but should check that the submodule was actually called. If we want to check the submodule's correctness, we unit test the submodule. Now - did we need a submodule to begin with? We could have, for example, kept HadoopBootstrapUtil not really be a submodule at all, but just a `static` call on a utility method. I think that's debatable - it makes each test need to cover a larger amount of code; that is, we no longer test the utility method in isolation, which is what we get here. Therefore it's unclear if HadoopBootstrapUtil being its own object that can be stubbed, is the right approach. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r233210145 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/HadoopConfExecutorFeatureStepSuite.scala --- @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + *http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features + +import org.mockito.{Mock, MockitoAnnotations} +import org.mockito.Matchers.{eq => Eq} +import org.mockito.Mockito.when +import org.mockito.invocation.InvocationOnMock +import org.mockito.stubbing.Answer +import org.scalatest.BeforeAndAfter + +import org.apache.spark.{SparkConf, SparkFunSuite} +import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesExecutorSpecificConf, SparkPod} +import org.apache.spark.deploy.k8s.Constants.HADOOP_CONFIG_MAP_NAME +import org.apache.spark.deploy.k8s.features.hadooputils.HadoopBootstrapUtil + +class HadoopConfExecutorFeatureStepSuite extends SparkFunSuite with BeforeAndAfter { + private val hadoopConfMapName = "HADOOP_CONF_NAME" + private val sparkPod = SparkPod.initialPod() + + @Mock + private var kubernetesConf: KubernetesConf[KubernetesExecutorSpecificConf] = _ + + @Mock + private var hadoopBootstrapUtil: HadoopBootstrapUtil = _ + + before { +MockitoAnnotations.initMocks(this) +val sparkConf = new SparkConf(false) + .set(HADOOP_CONFIG_MAP_NAME, hadoopConfMapName) +when(kubernetesConf.sparkConf).thenReturn(sparkConf) + } + + test("bootstrapHadoopConf being applied to a base spark pod") { +when(hadoopBootstrapUtil.bootstrapHadoopConfDir( --- End diff -- > The purpose of the mocks is to make every unit test run only code that is in the class under test First, let me clarify that I'm against what I think is the excessive use of mocks in these tests; and by that I mean mostly mocking a whole bunch of things using mockito. You can mock things without mockito. e.g. if what you want is to provide the test with a specific mock configuration, it's pretty trivial to do that with existing classes. Tests do this all over the code base without having to mock `SparkConf` or any other classes. (This is currently more painful than it should be in the k8s backend, which is one of the things I'm trying to fix with my cleanups.) So let's use this particular test here as an example. What is it testing? I think it should be testing that if a user has set the appropriate configuration in their `SparkConf`, that this step will mount a volume with the user's Hadoop configuration in the final pod. Is that what the code here is testing? I don't think so. It's testing that a specific method call is being made with certain parameters. A much better test would be to actually let the code in the step run and do its thing, and test the final result for expected things (in this case, the hadoop conf being mounted). Or thinking another way: how can you actually break this test, without removing that one method call from the step implementation? Since both the inputs and the output of that call are mocked here. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user mccheah commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r233198612 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/HadoopConfExecutorFeatureStepSuite.scala --- @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + *http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features + +import org.mockito.{Mock, MockitoAnnotations} +import org.mockito.Matchers.{eq => Eq} +import org.mockito.Mockito.when +import org.mockito.invocation.InvocationOnMock +import org.mockito.stubbing.Answer +import org.scalatest.BeforeAndAfter + +import org.apache.spark.{SparkConf, SparkFunSuite} +import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesExecutorSpecificConf, SparkPod} +import org.apache.spark.deploy.k8s.Constants.HADOOP_CONFIG_MAP_NAME +import org.apache.spark.deploy.k8s.features.hadooputils.HadoopBootstrapUtil + +class HadoopConfExecutorFeatureStepSuite extends SparkFunSuite with BeforeAndAfter { + private val hadoopConfMapName = "HADOOP_CONF_NAME" + private val sparkPod = SparkPod.initialPod() + + @Mock + private var kubernetesConf: KubernetesConf[KubernetesExecutorSpecificConf] = _ + + @Mock + private var hadoopBootstrapUtil: HadoopBootstrapUtil = _ + + before { +MockitoAnnotations.initMocks(this) +val sparkConf = new SparkConf(false) + .set(HADOOP_CONFIG_MAP_NAME, hadoopConfMapName) +when(kubernetesConf.sparkConf).thenReturn(sparkConf) + } + + test("bootstrapHadoopConf being applied to a base spark pod") { +when(hadoopBootstrapUtil.bootstrapHadoopConfDir( --- End diff -- Instead of mocks would we implement stub interfaces then? The purpose of the mocks is to make every unit test run only code that is in the class under test, plus utility methods. I think the only alternative to mocks is writing stub interfaces, and it's not clear how much more / less boilerplate that would be also. If the stub interfaces lead to less boilerplate than mocks then we should go with that. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r233168980 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/HadoopConfExecutorFeatureStepSuite.scala --- @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + *http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features + +import org.mockito.{Mock, MockitoAnnotations} +import org.mockito.Matchers.{eq => Eq} +import org.mockito.Mockito.when +import org.mockito.invocation.InvocationOnMock +import org.mockito.stubbing.Answer +import org.scalatest.BeforeAndAfter + +import org.apache.spark.{SparkConf, SparkFunSuite} +import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesExecutorSpecificConf, SparkPod} +import org.apache.spark.deploy.k8s.Constants.HADOOP_CONFIG_MAP_NAME +import org.apache.spark.deploy.k8s.features.hadooputils.HadoopBootstrapUtil + +class HadoopConfExecutorFeatureStepSuite extends SparkFunSuite with BeforeAndAfter { + private val hadoopConfMapName = "HADOOP_CONF_NAME" + private val sparkPod = SparkPod.initialPod() + + @Mock + private var kubernetesConf: KubernetesConf[KubernetesExecutorSpecificConf] = _ + + @Mock + private var hadoopBootstrapUtil: HadoopBootstrapUtil = _ + + before { +MockitoAnnotations.initMocks(this) +val sparkConf = new SparkConf(false) + .set(HADOOP_CONFIG_MAP_NAME, hadoopConfMapName) +when(kubernetesConf.sparkConf).thenReturn(sparkConf) + } + + test("bootstrapHadoopConf being applied to a base spark pod") { +when(hadoopBootstrapUtil.bootstrapHadoopConfDir( + Eq(None), + Eq(None), + Eq(Some(hadoopConfMapName)), + Eq(sparkPod) +)).thenAnswer(new Answer[SparkPod] { + override def answer(invocation: InvocationOnMock) : SparkPod = { +KubernetesFeaturesTestUtils.hadoopConfBootPod( + invocation.getArgumentAt(3, classOf[SparkPod])) + } +}) +val hConfStep = new HadoopConfExecutorFeatureStep(kubernetesConf, hadoopBootstrapUtil) +val pod = hConfStep.configurePod(sparkPod) +KubernetesFeaturesTestUtils.podHasLabels(pod.pod, Map("bootstrap-hconf" -> "true")) +assert(hConfStep.getAdditionalPodSystemProperties() == Map.empty, --- End diff -- use `assert(foo.isEmpty)`, no need for a message. Also in a bunch of places below. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r233174157 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/HadoopConfExecutorFeatureStepSuite.scala --- @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + *http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features + +import org.mockito.{Mock, MockitoAnnotations} +import org.mockito.Matchers.{eq => Eq} +import org.mockito.Mockito.when +import org.mockito.invocation.InvocationOnMock +import org.mockito.stubbing.Answer +import org.scalatest.BeforeAndAfter + +import org.apache.spark.{SparkConf, SparkFunSuite} +import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesExecutorSpecificConf, SparkPod} +import org.apache.spark.deploy.k8s.Constants.HADOOP_CONFIG_MAP_NAME +import org.apache.spark.deploy.k8s.features.hadooputils.HadoopBootstrapUtil + +class HadoopConfExecutorFeatureStepSuite extends SparkFunSuite with BeforeAndAfter { + private val hadoopConfMapName = "HADOOP_CONF_NAME" + private val sparkPod = SparkPod.initialPod() + + @Mock + private var kubernetesConf: KubernetesConf[KubernetesExecutorSpecificConf] = _ + + @Mock + private var hadoopBootstrapUtil: HadoopBootstrapUtil = _ + + before { +MockitoAnnotations.initMocks(this) +val sparkConf = new SparkConf(false) + .set(HADOOP_CONFIG_MAP_NAME, hadoopConfMapName) +when(kubernetesConf.sparkConf).thenReturn(sparkConf) + } + + test("bootstrapHadoopConf being applied to a base spark pod") { +when(hadoopBootstrapUtil.bootstrapHadoopConfDir( --- End diff -- So after I spent some time cleaning other code in the k8s backend, I started to really dislike this kind of test that over-mocks things. This test is actually just a result of a lot of other things that I think are wrong in the current code (not just kerberos-related, but in the k8s backend in general). Basically you have a lot of code and mocks here to test a single thing: that the feature step is calling this one method with the parameters you expect. If instead the feature step was written to be more easily testable, this wouldn't be needed. You wouldn't need to mock anything, and it would just run the normal code and you'd assert that the resulting pod has what you expect. I filed SPARK-25874 to fix all this stuff (with a few sub-tasks), and I'm now thinking it's better to do that first. It would simplify the tests here a lot. It would avoid all this noisy and IMO unnecessary mocking. And we'd have better tests. You can look at my p.o.c. code linked from that bug to see what I mean. As another data point, to address the comments on SPARK-25815 I'm having to rewrite a bunch of code in this area. So all these tests would have to be largely rewritten (on which PR, it depends on which one goes in first). So, long story short, checking this in would mean I'd be rewriting all this code very, very soon. So I'm not seeing a lot of gains here... --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user mccheah commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r230965800 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/KubernetesFeaturesTestUtils.scala --- @@ -63,4 +63,66 @@ object KubernetesFeaturesTestUtils { def containerHasEnvVar(container: Container, envVarName: String): Boolean = { container.getEnv.asScala.exists(envVar => envVar.getName == envVarName) } + + def containerHasEnvVars(container: Container, envs: Map[String, String]): Unit = { +assertHelper[Set[(String, String)]](envs.toSet, + container.getEnv.asScala +.map { e => (e.getName, e.getValue) }.toSet, + subsetOfTup[Set[(String, String)], String], "a subset of") + } + + def containerHasVolumeMounts(container: Container, vms: Map[String, String]): Unit = { +assertHelper[Set[(String, String)]](vms.toSet, + container.getVolumeMounts.asScala +.map { vm => (vm.getName, vm.getMountPath) }.toSet, + subsetOfTup[Set[(String, String)], String], "a subset of") + } + + def podHasLabels(pod: Pod, labels: Map[String, String]): Unit = { +assertHelper[Set[(String, String)]](labels.toSet, pod.getMetadata.getLabels.asScala.toSet, + subsetOfTup[Set[(String, String)], String], "a subset of") + } + + def podHasVolumes(pod: Pod, volumes: Seq[Volume]): Unit = { +assertHelper[Set[Volume]](volumes.toSet, pod.getSpec.getVolumes.asScala.toSet, + subsetOfElem[Set[Volume], Volume], "a subset of") + } + + // Mocking bootstrapHadoopConfDir + def hadoopConfBootPod(inputPod: SparkPod): SparkPod = +SparkPod( + new PodBuilder(inputPod.pod) +.editOrNewMetadata() + .addToLabels("bootstrap-hconf", "true") + .endMetadata() +.build(), + inputPod.container) + + // Mocking bootstrapKerberosPod + def krbBootPod(inputPod: SparkPod): SparkPod = +SparkPod( + new PodBuilder(inputPod.pod) +.editOrNewMetadata() + .addToLabels("bootstrap-kerberos", "true") + .endMetadata() +.build(), + inputPod.container) + + // Mocking bootstrapSparkUserPod + def userBootPod(inputPod: SparkPod): SparkPod = +SparkPod( + new PodBuilder(inputPod.pod) +.editOrNewMetadata() + .addToLabels("bootstrap-user", "true") + .endMetadata() +.build(), + inputPod.container) + + def subsetOfElem[T <: Set[B], B <: Any]: (T, T) => Boolean = (a, b) => a.subsetOf(b) + def subsetOfTup[T <: Set[(B, B)], B <: Any]: (T, T) => Boolean = (a, b) => a.subsetOf(b) + + def assertHelper[T](con1: T, con2: T, + expr: (T, T) => Boolean = (a: T, b: T) => a == b, exprMsg: String = "equal to"): Unit = { +assert(expr(con1, con2), s"$con1 is not $exprMsg $con2 as expected") --- End diff -- I think here the simpler approach is preferred - on each assertion line I want to see right then and there what error message I'll get back, not having to go through a helper method. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user ifilonenko commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r230951222 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/KubernetesFeaturesTestUtils.scala --- @@ -63,4 +63,66 @@ object KubernetesFeaturesTestUtils { def containerHasEnvVar(container: Container, envVarName: String): Boolean = { container.getEnv.asScala.exists(envVar => envVar.getName == envVarName) } + + def containerHasEnvVars(container: Container, envs: Map[String, String]): Unit = { +assertHelper[Set[(String, String)]](envs.toSet, + container.getEnv.asScala +.map { e => (e.getName, e.getValue) }.toSet, + subsetOfTup[Set[(String, String)], String], "a subset of") + } + + def containerHasVolumeMounts(container: Container, vms: Map[String, String]): Unit = { +assertHelper[Set[(String, String)]](vms.toSet, + container.getVolumeMounts.asScala +.map { vm => (vm.getName, vm.getMountPath) }.toSet, + subsetOfTup[Set[(String, String)], String], "a subset of") + } + + def podHasLabels(pod: Pod, labels: Map[String, String]): Unit = { +assertHelper[Set[(String, String)]](labels.toSet, pod.getMetadata.getLabels.asScala.toSet, + subsetOfTup[Set[(String, String)], String], "a subset of") + } + + def podHasVolumes(pod: Pod, volumes: Seq[Volume]): Unit = { +assertHelper[Set[Volume]](volumes.toSet, pod.getSpec.getVolumes.asScala.toSet, + subsetOfElem[Set[Volume], Volume], "a subset of") + } + + // Mocking bootstrapHadoopConfDir + def hadoopConfBootPod(inputPod: SparkPod): SparkPod = +SparkPod( + new PodBuilder(inputPod.pod) +.editOrNewMetadata() + .addToLabels("bootstrap-hconf", "true") + .endMetadata() +.build(), + inputPod.container) + + // Mocking bootstrapKerberosPod + def krbBootPod(inputPod: SparkPod): SparkPod = +SparkPod( + new PodBuilder(inputPod.pod) +.editOrNewMetadata() + .addToLabels("bootstrap-kerberos", "true") + .endMetadata() +.build(), + inputPod.container) + + // Mocking bootstrapSparkUserPod + def userBootPod(inputPod: SparkPod): SparkPod = +SparkPod( + new PodBuilder(inputPod.pod) +.editOrNewMetadata() + .addToLabels("bootstrap-user", "true") + .endMetadata() +.build(), + inputPod.container) + + def subsetOfElem[T <: Set[B], B <: Any]: (T, T) => Boolean = (a, b) => a.subsetOf(b) + def subsetOfTup[T <: Set[(B, B)], B <: Any]: (T, T) => Boolean = (a, b) => a.subsetOf(b) + + def assertHelper[T](con1: T, con2: T, + expr: (T, T) => Boolean = (a: T, b: T) => a == b, exprMsg: String = "equal to"): Unit = { +assert(expr(con1, con2), s"$con1 is not $exprMsg $con2 as expected") --- End diff -- I thought it would be better than doing a custom string at every assert statement. I don't find it to be too awkward, but *shrug* :) I actually kind of like it since we could do a check with any expression --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user mccheah commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r230941006 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/hadooputils/HadoopBootstrapUtilSuite.scala --- @@ -0,0 +1,221 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + *http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features.hadooputils + +import java.io.File +import java.util.UUID + +import scala.collection.JavaConverters._ + +import com.google.common.io.Files +import io.fabric8.kubernetes.api.model._ +import org.scalatest.BeforeAndAfter + +import org.apache.spark.SparkFunSuite +import org.apache.spark.deploy.k8s.Constants._ +import org.apache.spark.deploy.k8s.SparkPod +import org.apache.spark.deploy.k8s.features.KubernetesFeaturesTestUtils._ +import org.apache.spark.util.Utils + +class HadoopBootstrapUtilSuite extends SparkFunSuite with BeforeAndAfter{ + private val sparkPod = SparkPod.initialPod() + private val hadoopBootstrapUtil = new HadoopBootstrapUtil + private var tmpDir: File = _ + private var tmpFile: File = _ + + before { +tmpDir = Utils.createTempDir() +tmpFile = File.createTempFile(s"${UUID.randomUUID().toString}", ".txt", tmpDir) +Files.write("contents".getBytes, tmpFile) + } + + after { +tmpFile.delete() +tmpDir.delete() + } + + test("bootstrapKerberosPod with file location specified for krb5.conf file") { +val dtSecretName = "EXAMPLE_SECRET_NAME" +val dtSecretItemKey = "EXAMPLE_ITEM_KEY" +val userName = "SPARK_USER_NAME" +val fileLocation = Some(tmpFile.getAbsolutePath) +val stringPath = tmpFile.getName +val newKrb5ConfName = Some("/etc/krb5.conf") +val resultingPod = hadoopBootstrapUtil.bootstrapKerberosPod( + dtSecretName, + dtSecretItemKey, + userName, + fileLocation, + newKrb5ConfName, + None, + sparkPod) +val expectedVolumes = Seq( + new VolumeBuilder() +.withName(KRB_FILE_VOLUME) +.withNewConfigMap() + .withName(newKrb5ConfName.get) + .withItems(new KeyToPathBuilder() +.withKey(stringPath) +.withPath(stringPath) +.build()) +.endConfigMap() +.build(), + new VolumeBuilder() +.withName(SPARK_APP_HADOOP_SECRET_VOLUME_NAME) +.withNewSecret() + .withSecretName(dtSecretName) + .endSecret() +.build() +) +podHasVolumes(resultingPod.pod, expectedVolumes) +containerHasEnvVars(resultingPod.container, Map( + ENV_HADOOP_TOKEN_FILE_LOCATION -> s"$SPARK_APP_HADOOP_CREDENTIALS_BASE_DIR/$dtSecretItemKey", + ENV_SPARK_USER -> userName) +) +containerHasVolumeMounts(resultingPod.container, Map( + SPARK_APP_HADOOP_SECRET_VOLUME_NAME -> SPARK_APP_HADOOP_CREDENTIALS_BASE_DIR, + KRB_FILE_VOLUME -> (KRB_FILE_DIR_PATH + "/krb5.conf")) +) + } + + test("bootstrapKerberosPod with pre-existing configMap specified for krb5.conf file") { +val dtSecretName = "EXAMPLE_SECRET_NAME" --- End diff -- These can be constants at the top of the file or in a companion object. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user mccheah commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r230942826 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/KubernetesFeaturesTestUtils.scala --- @@ -63,4 +63,66 @@ object KubernetesFeaturesTestUtils { def containerHasEnvVar(container: Container, envVarName: String): Boolean = { container.getEnv.asScala.exists(envVar => envVar.getName == envVarName) } + + def containerHasEnvVars(container: Container, envs: Map[String, String]): Unit = { +assertHelper[Set[(String, String)]](envs.toSet, + container.getEnv.asScala +.map { e => (e.getName, e.getValue) }.toSet, + subsetOfTup[Set[(String, String)], String], "a subset of") + } + + def containerHasVolumeMounts(container: Container, vms: Map[String, String]): Unit = { +assertHelper[Set[(String, String)]](vms.toSet, + container.getVolumeMounts.asScala +.map { vm => (vm.getName, vm.getMountPath) }.toSet, + subsetOfTup[Set[(String, String)], String], "a subset of") + } + + def podHasLabels(pod: Pod, labels: Map[String, String]): Unit = { +assertHelper[Set[(String, String)]](labels.toSet, pod.getMetadata.getLabels.asScala.toSet, + subsetOfTup[Set[(String, String)], String], "a subset of") + } + + def podHasVolumes(pod: Pod, volumes: Seq[Volume]): Unit = { +assertHelper[Set[Volume]](volumes.toSet, pod.getSpec.getVolumes.asScala.toSet, + subsetOfElem[Set[Volume], Volume], "a subset of") + } + + // Mocking bootstrapHadoopConfDir + def hadoopConfBootPod(inputPod: SparkPod): SparkPod = +SparkPod( + new PodBuilder(inputPod.pod) +.editOrNewMetadata() + .addToLabels("bootstrap-hconf", "true") + .endMetadata() +.build(), + inputPod.container) + + // Mocking bootstrapKerberosPod + def krbBootPod(inputPod: SparkPod): SparkPod = +SparkPod( + new PodBuilder(inputPod.pod) +.editOrNewMetadata() + .addToLabels("bootstrap-kerberos", "true") + .endMetadata() +.build(), + inputPod.container) + + // Mocking bootstrapSparkUserPod + def userBootPod(inputPod: SparkPod): SparkPod = +SparkPod( + new PodBuilder(inputPod.pod) +.editOrNewMetadata() + .addToLabels("bootstrap-user", "true") + .endMetadata() +.build(), + inputPod.container) + + def subsetOfElem[T <: Set[B], B <: Any]: (T, T) => Boolean = (a, b) => a.subsetOf(b) + def subsetOfTup[T <: Set[(B, B)], B <: Any]: (T, T) => Boolean = (a, b) => a.subsetOf(b) + + def assertHelper[T](con1: T, con2: T, + expr: (T, T) => Boolean = (a: T, b: T) => a == b, exprMsg: String = "equal to"): Unit = { +assert(expr(con1, con2), s"$con1 is not $exprMsg $con2 as expected") --- End diff -- This interaction seems really awkward to me. Why can't we just embed the error message directly into the assertion on each assertion statement? I'm also not familiar enough with the ScalaTest framework but perhaps we can explore other options the library has to offer. (In Java I'm used to using [AssertJ](https://github.com/joel-costigliola/assertj-core) for fluent assertions but I'm not sure if that will be of any help for Scala objects, particularly Scala collections without converting everything.) --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user mccheah commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r230941669 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/hadooputils/HadoopBootstrapUtilSuite.scala --- @@ -0,0 +1,221 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + *http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features.hadooputils + +import java.io.File +import java.util.UUID + +import scala.collection.JavaConverters._ + +import com.google.common.io.Files +import io.fabric8.kubernetes.api.model._ +import org.scalatest.BeforeAndAfter + +import org.apache.spark.SparkFunSuite +import org.apache.spark.deploy.k8s.Constants._ +import org.apache.spark.deploy.k8s.SparkPod +import org.apache.spark.deploy.k8s.features.KubernetesFeaturesTestUtils._ +import org.apache.spark.util.Utils + +class HadoopBootstrapUtilSuite extends SparkFunSuite with BeforeAndAfter{ + private val sparkPod = SparkPod.initialPod() + private val hadoopBootstrapUtil = new HadoopBootstrapUtil + private var tmpDir: File = _ + private var tmpFile: File = _ + + before { +tmpDir = Utils.createTempDir() +tmpFile = File.createTempFile(s"${UUID.randomUUID().toString}", ".txt", tmpDir) +Files.write("contents".getBytes, tmpFile) + } + + after { +tmpFile.delete() +tmpDir.delete() + } + + test("bootstrapKerberosPod with file location specified for krb5.conf file") { +val dtSecretName = "EXAMPLE_SECRET_NAME" --- End diff -- These can be constants in a companion object or at the top of the class. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r230223616 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/KubernetesFeaturesTestUtils.scala --- @@ -63,4 +63,54 @@ object KubernetesFeaturesTestUtils { def containerHasEnvVar(container: Container, envVarName: String): Boolean = { container.getEnv.asScala.exists(envVar => envVar.getName == envVarName) } + + def containerHasEnvVars(container: Container, envs: Map[String, String]): Boolean = { +envs.toSet.subsetOf(container.getEnv.asScala + .map { e => (e.getName, e.getValue)}.toSet) --- End diff -- space before `}` Also in other places. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r230221109 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/hadooputils/HadoopBootstrapUtilSuite.scala --- @@ -0,0 +1,227 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + *http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features.hadooputils + +import java.io.File +import java.util.UUID + +import scala.collection.JavaConverters._ + +import com.google.common.io.Files +import io.fabric8.kubernetes.api.model._ +import org.scalatest.BeforeAndAfter + +import org.apache.spark.SparkFunSuite +import org.apache.spark.deploy.k8s.Constants._ +import org.apache.spark.deploy.k8s.SparkPod +import org.apache.spark.deploy.k8s.features.KubernetesFeaturesTestUtils._ +import org.apache.spark.util.Utils + +class HadoopBootstrapUtilSuite extends SparkFunSuite with BeforeAndAfter{ + private val sparkPod = SparkPod.initialPod() + private val hadoopBootstrapUtil = new HadoopBootstrapUtil + private var tmpDir: File = _ + private var tmpFile: File = _ + + before { +tmpDir = Utils.createTempDir() +tmpFile = createTempFile(tmpDir, "contents") + } + + after { +tmpFile.delete() +tmpDir.delete() + } + + test("Testing bootstrapKerberosPod with file location of krb5") { +val dtSecretName = "EXAMPLE_SECRET_NAME" +val dtSecretItemKey = "EXAMPLE_ITEM_KEY" +val userName = "SPARK_USER_NAME" +val fileLocation = Some(tmpFile.getAbsolutePath) +val stringPath = tmpFile.toPath.getFileName.toString +val newKrb5ConfName = Some("/etc/krb5.conf") +val resultingPod = hadoopBootstrapUtil.bootstrapKerberosPod( + dtSecretName, + dtSecretItemKey, + userName, + fileLocation, + newKrb5ConfName, + None, + sparkPod) +val expectedVolumes = Seq( + new VolumeBuilder() +.withName(KRB_FILE_VOLUME) +.withNewConfigMap() + .withName(newKrb5ConfName.get) + .withItems(new KeyToPathBuilder() +.withKey(stringPath) +.withPath(stringPath) +.build()) +.endConfigMap() +.build(), + new VolumeBuilder() +.withName(SPARK_APP_HADOOP_SECRET_VOLUME_NAME) +.withNewSecret() + .withSecretName(dtSecretName) + .endSecret() +.build() +) +assert(podHasVolumes(resultingPod.pod, expectedVolumes)) +assert(containerHasEnvVars(resultingPod.container, Map( + ENV_HADOOP_TOKEN_FILE_LOCATION -> s"$SPARK_APP_HADOOP_CREDENTIALS_BASE_DIR/$dtSecretItemKey", + ENV_SPARK_USER -> userName) +)) +assert(containerHasVolumeMounts(resultingPod.container, Map( + SPARK_APP_HADOOP_SECRET_VOLUME_NAME -> SPARK_APP_HADOOP_CREDENTIALS_BASE_DIR, + KRB_FILE_VOLUME -> (KRB_FILE_DIR_PATH + "/krb5.conf")) +)) + } + + test("Testing bootstrapKerberosPod with configMap of krb5") { +val dtSecretName = "EXAMPLE_SECRET_NAME" +val dtSecretItemKey = "EXAMPLE_ITEM_KEY" +val userName = "SPARK_USER_NAME" +val existingKrb5ConfName = Some("krb5CMap") +val resultingPod = hadoopBootstrapUtil.bootstrapKerberosPod( + dtSecretName, + dtSecretItemKey, + userName, + None, + None, + existingKrb5ConfName, + sparkPod) +val expectedVolumes = Seq( + new VolumeBuilder() +.withName(KRB_FILE_VOLUME) +.withNewConfigMap() + .withName(existingKrb5ConfName.get) + .endConfigMap() +.build(), + new VolumeBuilder() +.withName(SPARK_APP_HADOOP_SECRET_VOLUME_NAME) +.withNewSecret() + .withSecretName(dtSecretName) + .endSecret() +.build() +)
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r230223300 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/hadooputils/HadoopBootstrapUtilSuite.scala --- @@ -0,0 +1,227 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + *http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features.hadooputils + +import java.io.File +import java.util.UUID + +import scala.collection.JavaConverters._ + +import com.google.common.io.Files +import io.fabric8.kubernetes.api.model._ +import org.scalatest.BeforeAndAfter + +import org.apache.spark.SparkFunSuite +import org.apache.spark.deploy.k8s.Constants._ +import org.apache.spark.deploy.k8s.SparkPod +import org.apache.spark.deploy.k8s.features.KubernetesFeaturesTestUtils._ +import org.apache.spark.util.Utils + +class HadoopBootstrapUtilSuite extends SparkFunSuite with BeforeAndAfter{ + private val sparkPod = SparkPod.initialPod() + private val hadoopBootstrapUtil = new HadoopBootstrapUtil + private var tmpDir: File = _ + private var tmpFile: File = _ + + before { +tmpDir = Utils.createTempDir() +tmpFile = createTempFile(tmpDir, "contents") + } + + after { +tmpFile.delete() +tmpDir.delete() + } + + test("Testing bootstrapKerberosPod with file location of krb5") { +val dtSecretName = "EXAMPLE_SECRET_NAME" +val dtSecretItemKey = "EXAMPLE_ITEM_KEY" +val userName = "SPARK_USER_NAME" +val fileLocation = Some(tmpFile.getAbsolutePath) +val stringPath = tmpFile.toPath.getFileName.toString +val newKrb5ConfName = Some("/etc/krb5.conf") +val resultingPod = hadoopBootstrapUtil.bootstrapKerberosPod( + dtSecretName, + dtSecretItemKey, + userName, + fileLocation, + newKrb5ConfName, + None, + sparkPod) +val expectedVolumes = Seq( + new VolumeBuilder() +.withName(KRB_FILE_VOLUME) +.withNewConfigMap() + .withName(newKrb5ConfName.get) + .withItems(new KeyToPathBuilder() +.withKey(stringPath) +.withPath(stringPath) +.build()) +.endConfigMap() +.build(), + new VolumeBuilder() +.withName(SPARK_APP_HADOOP_SECRET_VOLUME_NAME) +.withNewSecret() + .withSecretName(dtSecretName) + .endSecret() +.build() +) +assert(podHasVolumes(resultingPod.pod, expectedVolumes)) +assert(containerHasEnvVars(resultingPod.container, Map( + ENV_HADOOP_TOKEN_FILE_LOCATION -> s"$SPARK_APP_HADOOP_CREDENTIALS_BASE_DIR/$dtSecretItemKey", + ENV_SPARK_USER -> userName) +)) +assert(containerHasVolumeMounts(resultingPod.container, Map( + SPARK_APP_HADOOP_SECRET_VOLUME_NAME -> SPARK_APP_HADOOP_CREDENTIALS_BASE_DIR, + KRB_FILE_VOLUME -> (KRB_FILE_DIR_PATH + "/krb5.conf")) +)) + } + + test("Testing bootstrapKerberosPod with configMap of krb5") { +val dtSecretName = "EXAMPLE_SECRET_NAME" +val dtSecretItemKey = "EXAMPLE_ITEM_KEY" +val userName = "SPARK_USER_NAME" +val existingKrb5ConfName = Some("krb5CMap") +val resultingPod = hadoopBootstrapUtil.bootstrapKerberosPod( + dtSecretName, + dtSecretItemKey, + userName, + None, + None, + existingKrb5ConfName, + sparkPod) +val expectedVolumes = Seq( + new VolumeBuilder() +.withName(KRB_FILE_VOLUME) +.withNewConfigMap() + .withName(existingKrb5ConfName.get) + .endConfigMap() +.build(), + new VolumeBuilder() +.withName(SPARK_APP_HADOOP_SECRET_VOLUME_NAME) +.withNewSecret() + .withSecretName(dtSecretName) + .endSecret() +.build() +)
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r230223467 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/hadooputils/HadoopBootstrapUtilSuite.scala --- @@ -0,0 +1,227 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + *http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features.hadooputils + +import java.io.File +import java.util.UUID + +import scala.collection.JavaConverters._ + +import com.google.common.io.Files +import io.fabric8.kubernetes.api.model._ +import org.scalatest.BeforeAndAfter + +import org.apache.spark.SparkFunSuite +import org.apache.spark.deploy.k8s.Constants._ +import org.apache.spark.deploy.k8s.SparkPod +import org.apache.spark.deploy.k8s.features.KubernetesFeaturesTestUtils._ +import org.apache.spark.util.Utils + +class HadoopBootstrapUtilSuite extends SparkFunSuite with BeforeAndAfter{ + private val sparkPod = SparkPod.initialPod() + private val hadoopBootstrapUtil = new HadoopBootstrapUtil + private var tmpDir: File = _ + private var tmpFile: File = _ + + before { +tmpDir = Utils.createTempDir() +tmpFile = createTempFile(tmpDir, "contents") + } + + after { +tmpFile.delete() +tmpDir.delete() + } + + test("Testing bootstrapKerberosPod with file location of krb5") { +val dtSecretName = "EXAMPLE_SECRET_NAME" +val dtSecretItemKey = "EXAMPLE_ITEM_KEY" +val userName = "SPARK_USER_NAME" +val fileLocation = Some(tmpFile.getAbsolutePath) +val stringPath = tmpFile.toPath.getFileName.toString +val newKrb5ConfName = Some("/etc/krb5.conf") +val resultingPod = hadoopBootstrapUtil.bootstrapKerberosPod( + dtSecretName, + dtSecretItemKey, + userName, + fileLocation, + newKrb5ConfName, + None, + sparkPod) +val expectedVolumes = Seq( + new VolumeBuilder() +.withName(KRB_FILE_VOLUME) +.withNewConfigMap() + .withName(newKrb5ConfName.get) + .withItems(new KeyToPathBuilder() +.withKey(stringPath) +.withPath(stringPath) +.build()) +.endConfigMap() +.build(), + new VolumeBuilder() +.withName(SPARK_APP_HADOOP_SECRET_VOLUME_NAME) +.withNewSecret() + .withSecretName(dtSecretName) + .endSecret() +.build() +) +assert(podHasVolumes(resultingPod.pod, expectedVolumes)) +assert(containerHasEnvVars(resultingPod.container, Map( + ENV_HADOOP_TOKEN_FILE_LOCATION -> s"$SPARK_APP_HADOOP_CREDENTIALS_BASE_DIR/$dtSecretItemKey", + ENV_SPARK_USER -> userName) +)) +assert(containerHasVolumeMounts(resultingPod.container, Map( + SPARK_APP_HADOOP_SECRET_VOLUME_NAME -> SPARK_APP_HADOOP_CREDENTIALS_BASE_DIR, + KRB_FILE_VOLUME -> (KRB_FILE_DIR_PATH + "/krb5.conf")) +)) + } + + test("Testing bootstrapKerberosPod with configMap of krb5") { +val dtSecretName = "EXAMPLE_SECRET_NAME" +val dtSecretItemKey = "EXAMPLE_ITEM_KEY" +val userName = "SPARK_USER_NAME" +val existingKrb5ConfName = Some("krb5CMap") +val resultingPod = hadoopBootstrapUtil.bootstrapKerberosPod( + dtSecretName, + dtSecretItemKey, + userName, + None, + None, + existingKrb5ConfName, + sparkPod) +val expectedVolumes = Seq( + new VolumeBuilder() +.withName(KRB_FILE_VOLUME) +.withNewConfigMap() + .withName(existingKrb5ConfName.get) + .endConfigMap() +.build(), + new VolumeBuilder() +.withName(SPARK_APP_HADOOP_SECRET_VOLUME_NAME) +.withNewSecret() + .withSecretName(dtSecretName) + .endSecret() +.build() +)
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r230223525 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/hadooputils/HadoopBootstrapUtilSuite.scala --- @@ -0,0 +1,227 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + *http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features.hadooputils + +import java.io.File +import java.util.UUID + +import scala.collection.JavaConverters._ + +import com.google.common.io.Files +import io.fabric8.kubernetes.api.model._ +import org.scalatest.BeforeAndAfter + +import org.apache.spark.SparkFunSuite +import org.apache.spark.deploy.k8s.Constants._ +import org.apache.spark.deploy.k8s.SparkPod +import org.apache.spark.deploy.k8s.features.KubernetesFeaturesTestUtils._ +import org.apache.spark.util.Utils + +class HadoopBootstrapUtilSuite extends SparkFunSuite with BeforeAndAfter{ + private val sparkPod = SparkPod.initialPod() + private val hadoopBootstrapUtil = new HadoopBootstrapUtil + private var tmpDir: File = _ + private var tmpFile: File = _ + + before { +tmpDir = Utils.createTempDir() +tmpFile = createTempFile(tmpDir, "contents") + } + + after { +tmpFile.delete() +tmpDir.delete() + } + + test("Testing bootstrapKerberosPod with file location of krb5") { +val dtSecretName = "EXAMPLE_SECRET_NAME" +val dtSecretItemKey = "EXAMPLE_ITEM_KEY" +val userName = "SPARK_USER_NAME" +val fileLocation = Some(tmpFile.getAbsolutePath) +val stringPath = tmpFile.toPath.getFileName.toString +val newKrb5ConfName = Some("/etc/krb5.conf") +val resultingPod = hadoopBootstrapUtil.bootstrapKerberosPod( + dtSecretName, + dtSecretItemKey, + userName, + fileLocation, + newKrb5ConfName, + None, + sparkPod) +val expectedVolumes = Seq( + new VolumeBuilder() +.withName(KRB_FILE_VOLUME) +.withNewConfigMap() + .withName(newKrb5ConfName.get) + .withItems(new KeyToPathBuilder() +.withKey(stringPath) +.withPath(stringPath) +.build()) +.endConfigMap() +.build(), + new VolumeBuilder() +.withName(SPARK_APP_HADOOP_SECRET_VOLUME_NAME) +.withNewSecret() + .withSecretName(dtSecretName) + .endSecret() +.build() +) +assert(podHasVolumes(resultingPod.pod, expectedVolumes)) +assert(containerHasEnvVars(resultingPod.container, Map( + ENV_HADOOP_TOKEN_FILE_LOCATION -> s"$SPARK_APP_HADOOP_CREDENTIALS_BASE_DIR/$dtSecretItemKey", + ENV_SPARK_USER -> userName) +)) +assert(containerHasVolumeMounts(resultingPod.container, Map( + SPARK_APP_HADOOP_SECRET_VOLUME_NAME -> SPARK_APP_HADOOP_CREDENTIALS_BASE_DIR, + KRB_FILE_VOLUME -> (KRB_FILE_DIR_PATH + "/krb5.conf")) +)) + } + + test("Testing bootstrapKerberosPod with configMap of krb5") { +val dtSecretName = "EXAMPLE_SECRET_NAME" +val dtSecretItemKey = "EXAMPLE_ITEM_KEY" +val userName = "SPARK_USER_NAME" +val existingKrb5ConfName = Some("krb5CMap") +val resultingPod = hadoopBootstrapUtil.bootstrapKerberosPod( + dtSecretName, + dtSecretItemKey, + userName, + None, + None, + existingKrb5ConfName, + sparkPod) +val expectedVolumes = Seq( + new VolumeBuilder() +.withName(KRB_FILE_VOLUME) +.withNewConfigMap() + .withName(existingKrb5ConfName.get) + .endConfigMap() +.build(), + new VolumeBuilder() +.withName(SPARK_APP_HADOOP_SECRET_VOLUME_NAME) +.withNewSecret() + .withSecretName(dtSecretName) + .endSecret() +.build() +)
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r230223741 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/KubernetesFeaturesTestUtils.scala --- @@ -63,4 +63,54 @@ object KubernetesFeaturesTestUtils { def containerHasEnvVar(container: Container, envVarName: String): Boolean = { container.getEnv.asScala.exists(envVar => envVar.getName == envVarName) } + + def containerHasEnvVars(container: Container, envs: Map[String, String]): Boolean = { +envs.toSet.subsetOf(container.getEnv.asScala + .map { e => (e.getName, e.getValue)}.toSet) + } + + def containerHasVolumeMounts(container: Container, vms: Map[String, String]): Boolean = { +vms.toSet.subsetOf(container.getVolumeMounts.asScala + .map { vm => (vm.getName, vm.getMountPath)}.toSet) + } + + def podHasLabels(pod: Pod, labels: Map[String, String]): Boolean = { +labels.toSet.subsetOf(pod.getMetadata.getLabels.asScala.toSet) + } + + def podHasVolumes(pod: Pod, volumes: Seq[Volume]): Boolean = { +volumes.toSet.subsetOf(pod.getSpec.getVolumes.asScala.toSet) + } + + // Kerberos Specific Test utils --- End diff -- Unnecessary comment. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r230223780 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/KubernetesFeaturesTestUtils.scala --- @@ -63,4 +63,54 @@ object KubernetesFeaturesTestUtils { def containerHasEnvVar(container: Container, envVarName: String): Boolean = { container.getEnv.asScala.exists(envVar => envVar.getName == envVarName) } + + def containerHasEnvVars(container: Container, envs: Map[String, String]): Boolean = { +envs.toSet.subsetOf(container.getEnv.asScala + .map { e => (e.getName, e.getValue)}.toSet) + } + + def containerHasVolumeMounts(container: Container, vms: Map[String, String]): Boolean = { +vms.toSet.subsetOf(container.getVolumeMounts.asScala + .map { vm => (vm.getName, vm.getMountPath)}.toSet) + } + + def podHasLabels(pod: Pod, labels: Map[String, String]): Boolean = { +labels.toSet.subsetOf(pod.getMetadata.getLabels.asScala.toSet) + } + + def podHasVolumes(pod: Pod, volumes: Seq[Volume]): Boolean = { +volumes.toSet.subsetOf(pod.getSpec.getVolumes.asScala.toSet) + } + + // Kerberos Specific Test utils + + // Upon use of bootstrapHadoopConfDir --- End diff -- Not sure I understand what this comment means. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r230222736 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/hadooputils/HadoopBootstrapUtilSuite.scala --- @@ -0,0 +1,227 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + *http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features.hadooputils + +import java.io.File +import java.util.UUID + +import scala.collection.JavaConverters._ + +import com.google.common.io.Files +import io.fabric8.kubernetes.api.model._ +import org.scalatest.BeforeAndAfter + +import org.apache.spark.SparkFunSuite +import org.apache.spark.deploy.k8s.Constants._ +import org.apache.spark.deploy.k8s.SparkPod +import org.apache.spark.deploy.k8s.features.KubernetesFeaturesTestUtils._ +import org.apache.spark.util.Utils + +class HadoopBootstrapUtilSuite extends SparkFunSuite with BeforeAndAfter{ + private val sparkPod = SparkPod.initialPod() + private val hadoopBootstrapUtil = new HadoopBootstrapUtil + private var tmpDir: File = _ + private var tmpFile: File = _ + + before { +tmpDir = Utils.createTempDir() +tmpFile = createTempFile(tmpDir, "contents") + } + + after { +tmpFile.delete() +tmpDir.delete() + } + + test("Testing bootstrapKerberosPod with file location of krb5") { --- End diff -- Could you create better names for these tests? All of them start with "Testing", which is redundant and also makes it harder to differentiate them. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r230222388 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/hadooputils/HadoopBootstrapUtilSuite.scala --- @@ -0,0 +1,227 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + *http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.spark.deploy.k8s.features.hadooputils + +import java.io.File +import java.util.UUID + +import scala.collection.JavaConverters._ + +import com.google.common.io.Files +import io.fabric8.kubernetes.api.model._ +import org.scalatest.BeforeAndAfter + +import org.apache.spark.SparkFunSuite +import org.apache.spark.deploy.k8s.Constants._ +import org.apache.spark.deploy.k8s.SparkPod +import org.apache.spark.deploy.k8s.features.KubernetesFeaturesTestUtils._ +import org.apache.spark.util.Utils + +class HadoopBootstrapUtilSuite extends SparkFunSuite with BeforeAndAfter{ + private val sparkPod = SparkPod.initialPod() + private val hadoopBootstrapUtil = new HadoopBootstrapUtil + private var tmpDir: File = _ + private var tmpFile: File = _ + + before { +tmpDir = Utils.createTempDir() +tmpFile = createTempFile(tmpDir, "contents") + } + + after { +tmpFile.delete() +tmpDir.delete() + } + + test("Testing bootstrapKerberosPod with file location of krb5") { +val dtSecretName = "EXAMPLE_SECRET_NAME" +val dtSecretItemKey = "EXAMPLE_ITEM_KEY" +val userName = "SPARK_USER_NAME" +val fileLocation = Some(tmpFile.getAbsolutePath) +val stringPath = tmpFile.toPath.getFileName.toString +val newKrb5ConfName = Some("/etc/krb5.conf") +val resultingPod = hadoopBootstrapUtil.bootstrapKerberosPod( + dtSecretName, + dtSecretItemKey, + userName, + fileLocation, + newKrb5ConfName, + None, + sparkPod) +val expectedVolumes = Seq( + new VolumeBuilder() +.withName(KRB_FILE_VOLUME) +.withNewConfigMap() + .withName(newKrb5ConfName.get) + .withItems(new KeyToPathBuilder() +.withKey(stringPath) +.withPath(stringPath) +.build()) +.endConfigMap() +.build(), + new VolumeBuilder() +.withName(SPARK_APP_HADOOP_SECRET_VOLUME_NAME) +.withNewSecret() + .withSecretName(dtSecretName) + .endSecret() +.build() +) +assert(podHasVolumes(resultingPod.pod, expectedVolumes)) --- End diff -- If this assert (and others like it) fails, you'll get a pretty generic message ("false was not true" or some such). Might be better to make `podHasVolumes` (and friends) assert with a more helpful message that shows at least the computed set. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user liyinan926 commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r227064253 --- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala --- @@ -78,6 +79,12 @@ private[spark] case class KubernetesConf[T <: KubernetesRoleSpecificConf]( def krbConfigMapName: String = s"$appResourceNamePrefix-krb5-file" + // For unit-testing purposes + def hadoopBootstrapUtil: HadoopBootstrapUtil = new HadoopBootstrapUtil() --- End diff -- +1. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user liyinan926 commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r227065151 --- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/KubernetesFeaturesTestUtils.scala --- @@ -63,4 +63,40 @@ object KubernetesFeaturesTestUtils { def containerHasEnvVar(container: Container, envVarName: String): Boolean = { container.getEnv.asScala.exists(envVar => envVar.getName == envVarName) } + + def podHasLabels(pod: Pod, labels: Map[String, String]): Boolean = { +labels.toSet.subsetOf(pod.getMetadata.getLabels.asScala.toSet) + } + + // Kerberos Specific Test utils + + // Upon use of bootstrapHadoopConfDir + def hConfBootPod(inputPod: SparkPod): SparkPod = --- End diff -- Can we just name it `hadoopConfBootPod`? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
Github user mccheah commented on a diff in the pull request: https://github.com/apache/spark/pull/22760#discussion_r227062895 --- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala --- @@ -78,6 +79,12 @@ private[spark] case class KubernetesConf[T <: KubernetesRoleSpecificConf]( def krbConfigMapName: String = s"$appResourceNamePrefix-krb5-file" + // For unit-testing purposes + def hadoopBootstrapUtil: HadoopBootstrapUtil = new HadoopBootstrapUtil() --- End diff -- KubernetesConf might not be the right place for hadoopBootstrapUtil, hadoopKerberosLogin, and tokenManager - I'd expect KubernetesConf to just be a struct-like object. Can we rewrite these things such that these modules are injected into the constructors? We can use default implementations and override in tests. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22760: [SPARK-25751][K8S][TEST] Unit Testing for Kerbero...
GitHub user ifilonenko opened a pull request: https://github.com/apache/spark/pull/22760 [SPARK-25751][K8S][TEST] Unit Testing for Kerberos Support ## What changes were proposed in this pull request? Unit tests for Kerberos support addition ## How was this patch tested? Unit and Integration tests You can merge this pull request into a Git repository by running: $ git pull https://github.com/ifilonenko/spark SPARK-25751 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/22760.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #22760 commit 47bd9071f88199e4c73b43227626108481d72595 Author: Ilan Filonenko Date: 2018-10-17T22:12:51Z unit tests for all features --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org