From e674fb6c233c5d5f4d64f90263b5da3e19f87c4f Mon Sep 17 00:00:00 2001 From: Wanlin Du Date: Thu, 3 Feb 2022 17:03:01 -0800 Subject: [PATCH] Address comments --- api/v1/loadtest_types.go | 2 +- config/constants.go | 7 ++-- .../crd/bases/e2etest.grpc.io_loadtests.yaml | 4 +- config/defaults.go | 37 +++++++------------ config/samples/go_example_loadtest.yaml | 4 +- containers/init/ready/ready_test.go | 4 +- podbuilder/podbuilder.go | 10 ++--- podbuilder/podbuilder_test.go | 10 ++--- 8 files changed, 33 insertions(+), 45 deletions(-) diff --git a/api/v1/loadtest_types.go b/api/v1/loadtest_types.go index 59c0985a..66e85237 100644 --- a/api/v1/loadtest_types.go +++ b/api/v1/loadtest_types.go @@ -178,7 +178,7 @@ type Server struct { // +optional Build *Build `json:"build,omitempty"` - // Run describes a list of run container, , which is the runtime of the server for + // Run describes a list of run containers, which is the runtime of the server for // the actual test. Run []corev1.Container `json:"run"` } diff --git a/config/constants.go b/config/constants.go index cb1dc84c..e4bfade1 100644 --- a/config/constants.go +++ b/config/constants.go @@ -98,9 +98,10 @@ const ( // example, "loadtest-role=server" indicates a server component. RoleLabel = "loadtest-role" - // RunContainerListName holds the name of the list of containers that run - // simultaneously after the init containers. - RunContainerListName = "run" + // RunContainerName holds the name of the main container where the test is + // executed. The runtime for the test may contain multiple run containers. + // The main container is always the first container on the list. + RunContainerName = "run" // ScenariosFileEnv specifies the name of an env variable that specifies the // path to a JSON file with scenarios. diff --git a/config/crd/bases/e2etest.grpc.io_loadtests.yaml b/config/crd/bases/e2etest.grpc.io_loadtests.yaml index 6a3b63e2..61ccea03 100644 --- a/config/crd/bases/e2etest.grpc.io_loadtests.yaml +++ b/config/crd/bases/e2etest.grpc.io_loadtests.yaml @@ -2910,8 +2910,8 @@ spec: will choose a pool based on defaults. type: string run: - description: Run describes a list of run container, , which - is the runtime of the server for the actual test. + description: Run describes a list of run containers, which is + the runtime of the server for the actual test. items: description: A single application container that you want to run within a pod. diff --git a/config/defaults.go b/config/defaults.go index 0792ce87..a747a14e 100644 --- a/config/defaults.go +++ b/config/defaults.go @@ -150,16 +150,20 @@ func (d *Defaults) setBuildOrDefault(im *imageMap, language string, build *grpcv // setRunOrDefault sets the default runtime image if it is unset. It returns an // error if there is no default runtime image for the provided language. -func (d *Defaults) setRunOrDefault(im *imageMap, language string, run *corev1.Container) error { - if run != nil && run.Image == "" { +func (d *Defaults) setRunOrDefault(im *imageMap, language string, run []corev1.Container) error { + + if len(run) == 0 { + run = []corev1.Container{{Name: RunContainerName}} + } + + if run[0].Image == "" { runImage, err := im.runImage(language) if err != nil { return errors.Wrap(err, "could not infer default run image") } - run.Image = runImage - - run.Env = append(run.Env, corev1.EnvVar{ + run[0].Image = runImage + run[0].Env = append(run[0].Env, corev1.EnvVar{ Name: KillAfterEnv, Value: fmt.Sprintf("%f", d.KillAfter), }) @@ -182,12 +186,9 @@ func (d *Defaults) setDriverDefaults(im *imageMap, testSpec *grpcv1.LoadTestSpec } if len(driver.Run) == 0 { - driver.Run = []corev1.Container{{Name: WorkerContainerName}} - } - - if driver.Run[0].Image == "" { - driver.Run[0].Image = d.DriverImage + driver.Run = []corev1.Container{{Name: RunContainerName}} } + driver.Run[0].Image = d.DriverImage driver.Name = unwrapStrOrUUID(driver.Name) d.setCloneOrDefault(driver.Clone) @@ -196,10 +197,6 @@ func (d *Defaults) setDriverDefaults(im *imageMap, testSpec *grpcv1.LoadTestSpec return errors.Wrap(err, "failed to set defaults on instructions to build the driver") } - if err := d.setRunOrDefault(im, driver.Language, &driver.Run[0]); err != nil { - return errors.Wrap(err, "failed to set defaults on instructions to run the driver") - } - return nil } @@ -217,11 +214,7 @@ func (d *Defaults) setClientDefaults(im *imageMap, client *grpcv1.Client) error return errors.Wrap(err, "failed to set defaults on instructions to build the client") } - if len(client.Run) == 0 { - client.Run = []corev1.Container{{Name: WorkerContainerName}} - } - - if err := d.setRunOrDefault(im, client.Language, &client.Run[0]); err != nil { + if err := d.setRunOrDefault(im, client.Language, client.Run); err != nil { return errors.Wrap(err, "failed to set defaults on instructions to run the client") } @@ -242,11 +235,7 @@ func (d *Defaults) setServerDefaults(im *imageMap, server *grpcv1.Server) error return errors.Wrap(err, "failed to set defaults on instructions to build the server") } - if len(server.Run) == 0 { - server.Run = []corev1.Container{{Name: WorkerContainerName}} - } - - if err := d.setRunOrDefault(im, server.Language, &server.Run[0]); err != nil { + if err := d.setRunOrDefault(im, server.Language, server.Run); err != nil { return errors.Wrap(err, "failed to set defaults on instructions to run the server") } diff --git a/config/samples/go_example_loadtest.yaml b/config/samples/go_example_loadtest.yaml index 147be3a9..5556b46d 100644 --- a/config/samples/go_example_loadtest.yaml +++ b/config/samples/go_example_loadtest.yaml @@ -27,7 +27,7 @@ spec: language: go name: '0' run: - - name: "worker" + - name: "run" args: - -c - | @@ -118,7 +118,7 @@ spec: language: go name: '0' run: - - name: "worker" + - name: "run" args: - -c - | diff --git a/containers/init/ready/ready_test.go b/containers/init/ready/ready_test.go index 10d7abbf..06b3c4eb 100644 --- a/containers/init/ready/ready_test.go +++ b/containers/init/ready/ready_test.go @@ -46,7 +46,7 @@ var _ = Describe("WaitForReadyPods", func() { slowDuration = 100 * time.Millisecond * timeMultiplier driverPod = newTestPod("driver") - driverRunContainer := kubehelpers.ContainerForName(config.RunContainerListName, driverPod.Spec.Containers) + driverRunContainer := kubehelpers.ContainerForName(config.RunContainerName, driverPod.Spec.Containers) driverRunContainer.Ports = nil serverPod = newTestPod("server") @@ -203,7 +203,7 @@ var _ = Describe("WaitForReadyPods", func() { var customPort int32 = 9542 client2Pod := newTestPod("client") client2Pod.Name = "client-2" - client2PodContainer := kubehelpers.ContainerForName(config.RunContainerListName, client2Pod.Spec.Containers) + client2PodContainer := kubehelpers.ContainerForName(config.RunContainerName, client2Pod.Spec.Containers) client2PodContainer.Ports[0].ContainerPort = customPort podListerMock := &PodListerMock{ diff --git a/podbuilder/podbuilder.go b/podbuilder/podbuilder.go index c5f597a0..1d6fd52a 100644 --- a/podbuilder/podbuilder.go +++ b/podbuilder/podbuilder.go @@ -134,8 +134,6 @@ func (pb *PodBuilder) PodForClient(client *grpcv1.Client) (*corev1.Pod, error) { pb.pool = safeStrUnwrap(client.Pool) pb.clone = client.Clone pb.build = client.Build - // Pods are build after setting the default, so at least one container - // in the client.Run has been set. pb.run[0] = client.Run[0] pod := pb.newPod() @@ -150,7 +148,7 @@ func (pb *PodBuilder) PodForClient(client *grpcv1.Client) (*corev1.Pod, error) { } pod.Spec.NodeSelector = nodeSelector - runContainer := kubehelpers.ContainerForName(config.RunContainerListName, pod.Spec.Containers) + runContainer := kubehelpers.ContainerForName(config.RunContainerName, pod.Spec.Containers) pod.Spec.Containers[0].Env = append(pod.Spec.Containers[0].Env, corev1.EnvVar{ Name: config.DriverPortEnv, @@ -188,7 +186,7 @@ func (pb *PodBuilder) PodForDriver(driver *grpcv1.Driver) (*corev1.Pod, error) { } pod.Spec.NodeSelector = nodeSelector - runContainer := kubehelpers.ContainerForName(config.RunContainerListName, pod.Spec.Containers) + runContainer := kubehelpers.ContainerForName(config.RunContainerName, pod.Spec.Containers) addReadyInitContainer(pb.defaults, pb.test, &pod.Spec, runContainer) pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{ @@ -254,7 +252,7 @@ func (pb *PodBuilder) PodForServer(server *grpcv1.Server) (*corev1.Pod, error) { } pod.Spec.NodeSelector = nodeSelector - runContainer := kubehelpers.ContainerForName(config.RunContainerListName, pod.Spec.Containers) + runContainer := kubehelpers.ContainerForName(config.RunContainerName, pod.Spec.Containers) pod.Spec.Containers[0].Env = append(pod.Spec.Containers[0].Env, corev1.EnvVar{ Name: config.DriverPortEnv, @@ -341,7 +339,7 @@ func (pb *PodBuilder) newPod() *corev1.Pod { InitContainers: initContainers, Containers: []corev1.Container{ { - Name: config.RunContainerListName, + Name: config.RunContainerName, Image: safeStrUnwrap(&pb.run[0].Image), Command: pb.run[0].Command, Args: pb.run[0].Args, diff --git a/podbuilder/podbuilder_test.go b/podbuilder/podbuilder_test.go index 5cad22e3..f3ea20aa 100644 --- a/podbuilder/podbuilder_test.go +++ b/podbuilder/podbuilder_test.go @@ -295,7 +295,7 @@ var _ = Describe("PodBuilder", func() { Expect(err).ToNot(HaveOccurred()) Expect(pod.Spec.Containers).ToNot(BeEmpty()) - runContainer := kubehelpers.ContainerForName(config.RunContainerListName, pod.Spec.Containers) + runContainer := kubehelpers.ContainerForName(config.RunContainerName, pod.Spec.Containers) Expect(runContainer.VolumeMounts).ToNot(BeEmpty()) Expect(runContainer.VolumeMounts).To(ContainElement(corev1.VolumeMount{ Name: config.WorkspaceVolumeName, @@ -312,7 +312,7 @@ var _ = Describe("PodBuilder", func() { Expect(err).ToNot(HaveOccurred()) Expect(pod.Spec.Containers).ToNot(BeEmpty()) - runContainer := kubehelpers.ContainerForName(config.RunContainerListName, pod.Spec.Containers) + runContainer := kubehelpers.ContainerForName(config.RunContainerName, pod.Spec.Containers) Expect(getNames(runContainer.Ports)).To(ContainElement("driver")) Expect(getValue("driver", "ContainerPort", runContainer.Ports)).To(BeEquivalentTo(config.DriverPort)) }) @@ -541,7 +541,7 @@ var _ = Describe("PodBuilder", func() { Expect(err).ToNot(HaveOccurred()) Expect(pod.Spec.Containers).ToNot(BeEmpty()) - runContainer := kubehelpers.ContainerForName(config.RunContainerListName, pod.Spec.Containers) + runContainer := kubehelpers.ContainerForName(config.RunContainerName, pod.Spec.Containers) Expect(runContainer.VolumeMounts).ToNot(BeEmpty()) Expect(runContainer.VolumeMounts).To(ContainElement(corev1.VolumeMount{ Name: config.WorkspaceVolumeName, @@ -558,7 +558,7 @@ var _ = Describe("PodBuilder", func() { Expect(err).ToNot(HaveOccurred()) Expect(pod.Spec.Containers).ToNot(BeEmpty()) - runContainer := kubehelpers.ContainerForName(config.RunContainerListName, pod.Spec.Containers) + runContainer := kubehelpers.ContainerForName(config.RunContainerName, pod.Spec.Containers) Expect(getNames(runContainer.Ports)).To(ContainElement("driver")) Expect(getValue("driver", "ContainerPort", runContainer.Ports)).To(BeEquivalentTo(config.DriverPort)) }) @@ -787,7 +787,7 @@ var _ = Describe("PodBuilder", func() { Expect(err).ToNot(HaveOccurred()) Expect(pod.Spec.Containers).ToNot(BeEmpty()) - runContainer := kubehelpers.ContainerForName(config.RunContainerListName, pod.Spec.Containers) + runContainer := kubehelpers.ContainerForName(config.RunContainerName, pod.Spec.Containers) Expect(runContainer.VolumeMounts).ToNot(BeEmpty()) Expect(runContainer.VolumeMounts).To(ContainElement(corev1.VolumeMount{ Name: config.WorkspaceVolumeName,