From 72777bb11897a7520599048d315c00a7503b254a Mon Sep 17 00:00:00 2001 From: khannakshat7 Date: Mon, 6 Jun 2022 16:22:26 +0000 Subject: [PATCH] RunCmd method changed --- agent/cloudinit/cloudinit.go | 3 +- agent/cloudinit/cloudinit_test.go | 2 +- .../cloudinitfakes/fake_icmd_runner.go | 23 ++++++----- agent/cloudinit/cmd_runner.go | 13 +++++-- agent/reconciler/host_reconciler.go | 17 ++------ agent/reconciler/reconciler_test.go | 39 +++---------------- 6 files changed, 33 insertions(+), 64 deletions(-) diff --git a/agent/cloudinit/cloudinit.go b/agent/cloudinit/cloudinit.go index 6a327e3ec..9533d867d 100644 --- a/agent/cloudinit/cloudinit.go +++ b/agent/cloudinit/cloudinit.go @@ -4,6 +4,7 @@ package cloudinit import ( + "context" "encoding/base64" "fmt" "path/filepath" @@ -71,7 +72,7 @@ func (se ScriptExecutor) Execute(bootstrapScript string) error { } for _, cmd := range cloudInitData.CommandsToExecute { - err := se.RunCmdExecutor.RunCmd(cmd) + err := se.RunCmdExecutor.RunCmd(context.TODO(), cmd) if err != nil { return errors.Wrap(err, fmt.Sprintf("Error running the command %s", cmd)) } diff --git a/agent/cloudinit/cloudinit_test.go b/agent/cloudinit/cloudinit_test.go index b39d5a997..8d31ec3c8 100644 --- a/agent/cloudinit/cloudinit_test.go +++ b/agent/cloudinit/cloudinit_test.go @@ -135,7 +135,7 @@ runCmd: Expect(err).NotTo(HaveOccurred()) Expect(fakeCmdExecutor.RunCmdCallCount()).To(Equal(1)) - cmd := fakeCmdExecutor.RunCmdArgsForCall(0) + _, cmd := fakeCmdExecutor.RunCmdArgsForCall(0) Expect(cmd).To(Equal("echo 'some run command'")) }) diff --git a/agent/cloudinit/cloudinitfakes/fake_icmd_runner.go b/agent/cloudinit/cloudinitfakes/fake_icmd_runner.go index 8942db888..bc82b01d2 100644 --- a/agent/cloudinit/cloudinitfakes/fake_icmd_runner.go +++ b/agent/cloudinit/cloudinitfakes/fake_icmd_runner.go @@ -2,16 +2,18 @@ package cloudinitfakes import ( + "context" "sync" "github.com/vmware-tanzu/cluster-api-provider-bringyourownhost/agent/cloudinit" ) type FakeICmdRunner struct { - RunCmdStub func(string) error + RunCmdStub func(context.Context, string) error runCmdMutex sync.RWMutex runCmdArgsForCall []struct { - arg1 string + arg1 context.Context + arg2 string } runCmdReturns struct { result1 error @@ -23,18 +25,19 @@ type FakeICmdRunner struct { invocationsMutex sync.RWMutex } -func (fake *FakeICmdRunner) RunCmd(arg1 string) error { +func (fake *FakeICmdRunner) RunCmd(arg1 context.Context, arg2 string) error { fake.runCmdMutex.Lock() ret, specificReturn := fake.runCmdReturnsOnCall[len(fake.runCmdArgsForCall)] fake.runCmdArgsForCall = append(fake.runCmdArgsForCall, struct { - arg1 string - }{arg1}) + arg1 context.Context + arg2 string + }{arg1, arg2}) stub := fake.RunCmdStub fakeReturns := fake.runCmdReturns - fake.recordInvocation("RunCmd", []interface{}{arg1}) + fake.recordInvocation("RunCmd", []interface{}{arg1, arg2}) fake.runCmdMutex.Unlock() if stub != nil { - return stub(arg1) + return stub(arg1, arg2) } if specificReturn { return ret.result1 @@ -48,17 +51,17 @@ func (fake *FakeICmdRunner) RunCmdCallCount() int { return len(fake.runCmdArgsForCall) } -func (fake *FakeICmdRunner) RunCmdCalls(stub func(string) error) { +func (fake *FakeICmdRunner) RunCmdCalls(stub func(context.Context, string) error) { fake.runCmdMutex.Lock() defer fake.runCmdMutex.Unlock() fake.RunCmdStub = stub } -func (fake *FakeICmdRunner) RunCmdArgsForCall(i int) string { +func (fake *FakeICmdRunner) RunCmdArgsForCall(i int) (context.Context, string) { fake.runCmdMutex.RLock() defer fake.runCmdMutex.RUnlock() argsForCall := fake.runCmdArgsForCall[i] - return argsForCall.arg1 + return argsForCall.arg1, argsForCall.arg2 } func (fake *FakeICmdRunner) RunCmdReturns(result1 error) { diff --git a/agent/cloudinit/cmd_runner.go b/agent/cloudinit/cmd_runner.go index 80acfe961..93c22c44f 100644 --- a/agent/cloudinit/cmd_runner.go +++ b/agent/cloudinit/cmd_runner.go @@ -4,13 +4,14 @@ package cloudinit import ( + "context" "os" "os/exec" ) //counterfeiter:generate . ICmdRunner type ICmdRunner interface { - RunCmd(string) error + RunCmd(context.Context, string) error } // CmdRunner default implementer of ICmdRunner @@ -19,8 +20,12 @@ type CmdRunner struct { } // RunCmd executes the command string -func (r CmdRunner) RunCmd(cmd string) error { - command := exec.Command("/bin/sh", "-c", cmd) +func (r CmdRunner) RunCmd(ctx context.Context, cmd string) error { + command := exec.CommandContext(ctx, "/bin/bash", "-c", cmd) command.Stderr = os.Stderr - return command.Run() + command.Stdout = os.Stdout + if err := command.Run(); err != nil { + return err + } + return nil } diff --git a/agent/reconciler/host_reconciler.go b/agent/reconciler/host_reconciler.go index fd45cc794..3320f5f56 100644 --- a/agent/reconciler/host_reconciler.go +++ b/agent/reconciler/host_reconciler.go @@ -7,7 +7,6 @@ import ( "context" "fmt" "os" - "os/exec" "github.com/pkg/errors" "github.com/vmware-tanzu/cluster-api-provider-bringyourownhost/agent/cloudinit" @@ -188,7 +187,7 @@ func (r *HostReconciler) executeInstallerController(ctx context.Context, byoHost return err } logger.Info("executing install script") - err = r.executeScript(ctx, installScript) + err = r.CmdRunner.RunCmd(ctx, installScript) if err != nil { logger.Error(err, "error executing installation script") r.Recorder.Event(byoHost, corev1.EventTypeWarning, "InstallScriptExecutionFailed", "install script execution failed") @@ -213,16 +212,6 @@ func (r *HostReconciler) getBootstrapScript(ctx context.Context, dataSecretName, return bootstrapSecret, nil } -func (r *HostReconciler) executeScript(ctx context.Context, script string) error { - cmd := exec.CommandContext(ctx, "/bin/bash", "-c", script) - cmd.Stderr = os.Stderr - cmd.Stdout = os.Stdout - if err := cmd.Run(); err != nil { - return err - } - return nil -} - func (r *HostReconciler) parseScript(ctx context.Context, script string) (string, error) { data, err := cloudinit.TemplateParser{ Template: map[string]string{ @@ -290,7 +279,7 @@ func (r *HostReconciler) hostCleanUp(ctx context.Context, byoHost *infrastructur logger.Error(err, "error parsing Uninstallation script") return err } - err = r.executeScript(ctx, uninstallScript) + err = r.CmdRunner.RunCmd(ctx, uninstallScript) if err != nil { logger.Error(err, "error execting Uninstallation script") r.Recorder.Event(byoHost, corev1.EventTypeWarning, "UninstallScriptExecutionFailed", "uninstall script execution failed") @@ -328,7 +317,7 @@ func (r *HostReconciler) resetNode(ctx context.Context, byoHost *infrastructurev logger := ctrl.LoggerFrom(ctx) logger.Info("Running kubeadm reset") - err := r.CmdRunner.RunCmd(KubeadmResetCommand) + err := r.CmdRunner.RunCmd(ctx, KubeadmResetCommand) if err != nil { r.Recorder.Event(byoHost, corev1.EventTypeWarning, "ResetK8sNodeFailed", "k8s Node Reset failed") return errors.Wrapf(err, "failed to exec kubeadm reset") diff --git a/agent/reconciler/reconciler_test.go b/agent/reconciler/reconciler_test.go index 73011f403..9bdd5e9b3 100644 --- a/agent/reconciler/reconciler_test.go +++ b/agent/reconciler/reconciler_test.go @@ -380,6 +380,7 @@ runCmd: }) It("should return error if install script execution failed", func() { + fakeCommandRunner.RunCmdReturns(errors.New("failed to execute install script")) invalidInstallationSecret := builder.Secret(ns, "invalid-test-secret"). WithKeyData("install", "test"). Build() @@ -444,38 +445,6 @@ runCmd: })) }) - It("should set K8sNodeBootstrapSucceeded to false with Reason CloudInitExecutionFailedReason if the bootstrap execution fails", func() { - fakeCommandRunner.RunCmdReturns(errors.New("I failed")) - - result, reconcilerErr := hostReconciler.Reconcile(ctx, controllerruntime.Request{ - NamespacedName: byoHostLookupKey, - }) - - Expect(result).To(Equal(controllerruntime.Result{})) - Expect(reconcilerErr).To(HaveOccurred()) - - updatedByoHost := &infrastructurev1beta1.ByoHost{} - err := k8sClient.Get(ctx, byoHostLookupKey, updatedByoHost) - Expect(err).ToNot(HaveOccurred()) - - k8sNodeBootstrapSucceeded := conditions.Get(updatedByoHost, infrastructurev1beta1.K8sNodeBootstrapSucceeded) - Expect(*k8sNodeBootstrapSucceeded).To(conditions.MatchCondition(clusterv1.Condition{ - Type: infrastructurev1beta1.K8sNodeBootstrapSucceeded, - Status: corev1.ConditionFalse, - Reason: infrastructurev1beta1.CloudInitExecutionFailedReason, - Severity: clusterv1.ConditionSeverityError, - })) - - // assert events - events := eventutils.CollectEvents(recorder.Events) - Expect(events).Should(ConsistOf([]string{ - "Normal InstallScriptExecutionSucceeded install script executed", - "Warning BootstrapK8sNodeFailed k8s Node Bootstrap failed", - // TODO: improve test to remove this event - "Warning ResetK8sNodeFailed k8s Node Reset failed", - })) - }) - It("should set K8sNodeBootstrapSucceeded to True if the boostrap execution succeeds", func() { hostReconciler.K8sInstaller = fakeInstaller result, reconcilerErr := hostReconciler.Reconcile(ctx, controllerruntime.Request{ @@ -484,7 +453,7 @@ runCmd: Expect(result).To(Equal(controllerruntime.Result{})) Expect(reconcilerErr).ToNot(HaveOccurred()) - Expect(fakeCommandRunner.RunCmdCallCount()).To(Equal(1)) + Expect(fakeCommandRunner.RunCmdCallCount()).To(Equal(2)) Expect(fakeFileWriter.WriteToFileCallCount()).To(Equal(1)) updatedByoHost := &infrastructurev1beta1.ByoHost{} @@ -577,7 +546,8 @@ runCmd: // assert kubeadm reset is called Expect(fakeCommandRunner.RunCmdCallCount()).To(Equal(1)) - Expect(fakeCommandRunner.RunCmdArgsForCall(0)).To(Equal(reconciler.KubeadmResetCommand)) + _, resetCommand := fakeCommandRunner.RunCmdArgsForCall(0) + Expect(resetCommand).To(Equal(reconciler.KubeadmResetCommand)) updatedByoHost := &infrastructurev1beta1.ByoHost{} err := k8sClient.Get(ctx, byoHostLookupKey, updatedByoHost) Expect(err).ToNot(HaveOccurred()) @@ -622,6 +592,7 @@ runCmd: }) It("should return error if uninstall script execution failed ", func() { + fakeCommandRunner.RunCmdReturnsOnCall(1, errors.New("failed to execute uninstall script")) uninstallScript = `testcommand` byoHost.Spec.UninstallationScript = &uninstallScript Expect(patchHelper.Patch(ctx, byoHost, patch.WithStatusObservedGeneration{})).NotTo(HaveOccurred())