From 260b952e86d385664848e8a160ae4c805087281e Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Mon, 13 Jun 2022 21:43:27 +0200 Subject: [PATCH] Wait for deployment rollout only when binding created / modified (#5785) * Wait for deployment rollout only when binding created / modified * Add integration test --- pkg/kclient/dynamic.go | 27 ++++++--- pkg/kclient/interface.go | 2 +- pkg/kclient/mock_Client.go | 44 +++++---------- pkg/service/link.go | 5 +- pkg/service/service.go | 20 +++---- tests/integration/devfile/cmd_dev_test.go | 68 +++++++++++++++++++++++ 6 files changed, 114 insertions(+), 52 deletions(-) diff --git a/pkg/kclient/dynamic.go b/pkg/kclient/dynamic.go index 72e20426f1b..9ef23248476 100644 --- a/pkg/kclient/dynamic.go +++ b/pkg/kclient/dynamic.go @@ -16,27 +16,40 @@ import ( "k8s.io/klog" ) -// CreateDynamicResource creates a dynamic custom resource -func (c *Client) CreateDynamicResource(resource unstructured.Unstructured) error { +// PatchDynamicResource patches a dynamic custom resource and returns true +// if the generation of the resource increased or the resource is created +func (c *Client) PatchDynamicResource(resource unstructured.Unstructured) (bool, error) { klog.V(5).Infoln("Applying resource via server-side apply:") klog.V(5).Infoln(resourceAsJson(resource.Object)) data, err := json.Marshal(resource.Object) if err != nil { - return fmt.Errorf("unable to marshal resource: %w", err) + return false, fmt.Errorf("unable to marshal resource: %w", err) } gvr, err := c.GetRestMappingFromUnstructured(resource) if err != nil { - return err + return false, err + } + + var previousGeneration int64 = -1 + // Get the generation of the current resource + previous, err := c.DynamicClient.Resource(gvr.Resource).Namespace(c.Namespace).Get(context.TODO(), resource.GetName(), metav1.GetOptions{}) + if err != nil { + if !kerrors.IsNotFound(err) { + return false, err + } + } else { + previousGeneration = previous.GetGeneration() } // Patch the dynamic resource - _, err = c.DynamicClient.Resource(gvr.Resource).Namespace(c.Namespace).Patch(context.TODO(), resource.GetName(), types.ApplyPatchType, data, metav1.PatchOptions{FieldManager: FieldManager, Force: boolPtr(true)}) + current, err := c.DynamicClient.Resource(gvr.Resource).Namespace(c.Namespace).Patch(context.TODO(), resource.GetName(), types.ApplyPatchType, data, metav1.PatchOptions{FieldManager: FieldManager, Force: boolPtr(true)}) if err != nil { - return err + return false, err } + newGeneration := current.GetGeneration() - return nil + return newGeneration > previousGeneration, nil } // ListDynamicResource returns an unstructured list of instances of a Custom diff --git a/pkg/kclient/interface.go b/pkg/kclient/interface.go index 9c932aa29d6..6d10c5ed4af 100644 --- a/pkg/kclient/interface.go +++ b/pkg/kclient/interface.go @@ -51,7 +51,7 @@ type ClientInterface interface { IsDeploymentExtensionsV1Beta1() (bool, error) // dynamic.go - CreateDynamicResource(exampleCustomResource unstructured.Unstructured) error + PatchDynamicResource(exampleCustomResource unstructured.Unstructured) (bool, error) ListDynamicResources(gvr schema.GroupVersionResource) (*unstructured.UnstructuredList, error) GetDynamicResource(gvr schema.GroupVersionResource, name string) (*unstructured.Unstructured, error) UpdateDynamicResource(gvr schema.GroupVersionResource, name string, u *unstructured.Unstructured) error diff --git a/pkg/kclient/mock_Client.go b/pkg/kclient/mock_Client.go index dd678e70de7..a21186b1b9f 100644 --- a/pkg/kclient/mock_Client.go +++ b/pkg/kclient/mock_Client.go @@ -107,20 +107,6 @@ func (mr *MockClientInterfaceMockRecorder) CreateDeployment(deploy interface{}) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateDeployment", reflect.TypeOf((*MockClientInterface)(nil).CreateDeployment), deploy) } -// CreateDynamicResource mocks base method. -func (m *MockClientInterface) CreateDynamicResource(exampleCustomResource unstructured.Unstructured) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateDynamicResource", exampleCustomResource) - ret0, _ := ret[0].(error) - return ret0 -} - -// CreateDynamicResource indicates an expected call of CreateDynamicResource. -func (mr *MockClientInterfaceMockRecorder) CreateDynamicResource(exampleCustomResource interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateDynamicResource", reflect.TypeOf((*MockClientInterface)(nil).CreateDynamicResource), exampleCustomResource) -} - // CreateNamespace mocks base method. func (m *MockClientInterface) CreateNamespace(name string) (*v11.Namespace, error) { m.ctrl.T.Helper() @@ -811,21 +797,6 @@ func (mr *MockClientInterfaceMockRecorder) GetProject(projectName interface{}) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetProject", reflect.TypeOf((*MockClientInterface)(nil).GetProject), projectName) } -// GetReplicaSetByName mocks base method. -func (m *MockClientInterface) GetReplicaSetByName(name string) (*v10.ReplicaSet, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetReplicaSetByName", name) - ret0, _ := ret[0].(*v10.ReplicaSet) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetReplicaSetByName indicates an expected call of GetReplicaSetByName. -func (mr *MockClientInterfaceMockRecorder) GetReplicaSetByName(name interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetReplicaSetByName", reflect.TypeOf((*MockClientInterface)(nil).GetReplicaSetByName), name) -} - // GetResourceSpecDefinition mocks base method. func (m *MockClientInterface) GetResourceSpecDefinition(group, version, kind string) (*spec.Schema, error) { m.ctrl.T.Helper() @@ -1125,6 +1096,21 @@ func (mr *MockClientInterfaceMockRecorder) NewServiceBindingServiceObject(unstru return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewServiceBindingServiceObject", reflect.TypeOf((*MockClientInterface)(nil).NewServiceBindingServiceObject), unstructuredService, bindingName) } +// PatchDynamicResource mocks base method. +func (m *MockClientInterface) PatchDynamicResource(exampleCustomResource unstructured.Unstructured) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "PatchDynamicResource", exampleCustomResource) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// PatchDynamicResource indicates an expected call of PatchDynamicResource. +func (mr *MockClientInterfaceMockRecorder) PatchDynamicResource(exampleCustomResource interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PatchDynamicResource", reflect.TypeOf((*MockClientInterface)(nil).PatchDynamicResource), exampleCustomResource) +} + // RunLogout mocks base method. func (m *MockClientInterface) RunLogout(stdout io.Writer) error { m.ctrl.T.Helper() diff --git a/pkg/service/link.go b/pkg/service/link.go index ab55a841669..f2055263d20 100644 --- a/pkg/service/link.go +++ b/pkg/service/link.go @@ -90,7 +90,8 @@ func pushLinksWithOperator(client kclient.ClientInterface, devfileObj parser.Dev u.SetOwnerReferences([]metav1.OwnerReference{ownerReference}) u.SetLabels(labels) - err = createOperatorService(client, u) + var updated bool + updated, err = updateOperatorService(client, u) delete(deployed, u.GetKind()+"/"+crdName) if err != nil { if strings.Contains(err.Error(), "already exists") { @@ -104,7 +105,7 @@ func pushLinksWithOperator(client kclient.ClientInterface, devfileObj parser.Dev // uncomment/modify when service linking is enabled in v3 // name := u.GetName() // log.Successf("Created link %q using Service Binding Operator on the cluster; component will be restarted", name) - restartNeeded = true + restartNeeded = restartNeeded || updated } for key, val := range deployed { diff --git a/pkg/service/service.go b/pkg/service/service.go index 8b6c864f158..69a0fd4045f 100644 --- a/pkg/service/service.go +++ b/pkg/service/service.go @@ -257,7 +257,7 @@ func PushKubernetesResource(client kclient.ClientInterface, u unstructured.Unstr // Pass in all annotations to the k8s resource u.SetAnnotations(mergeMaps(u.GetAnnotations(), annotations)) - err = createOperatorService(client, u) + _, err = updateOperatorService(client, u) return isOp, err } @@ -374,27 +374,21 @@ func isLinkResource(kind string) bool { return kind == "ServiceBinding" } -// createOperatorService creates the given operator on the cluster -// it returns the CR,Kind and errors -func createOperatorService(client kclient.ClientInterface, u unstructured.Unstructured) error { - - // Check resource - checkSpinner := log.Spinner("Searching resource in cluster") - defer checkSpinner.End(false) - - checkSpinner.End(true) +// updateOperatorService creates the given operator on the cluster +// it returns true if the generation of the resource increased or the resource is created +func updateOperatorService(client kclient.ClientInterface, u unstructured.Unstructured) (bool, error) { // Create the service on cluster createSpinner := log.Spinnerf("Creating kind %s", u.GetKind()) defer createSpinner.End(false) - err := client.CreateDynamicResource(u) + updated, err := client.PatchDynamicResource(u) if err != nil { - return err + return false, err } createSpinner.End(true) - return err + return updated, err } // ValidateResourcesExist validates if the Kubernetes inlined components are installed on the cluster diff --git a/tests/integration/devfile/cmd_dev_test.go b/tests/integration/devfile/cmd_dev_test.go index 0eb8dc127cc..ec961d93f45 100644 --- a/tests/integration/devfile/cmd_dev_test.go +++ b/tests/integration/devfile/cmd_dev_test.go @@ -660,6 +660,74 @@ var _ = Describe("odo dev command tests", func() { }) }) + When("Starting a PostgreSQL service", func() { + BeforeEach(func() { + if helper.IsKubernetesCluster() { + Skip("Operators have not been setup on Kubernetes cluster yet. Remove this once the issue has been fixed.") + } + // Ensure that the operators are installed + commonVar.CliRunner.EnsureOperatorIsInstalled("service-binding-operator") + commonVar.CliRunner.EnsureOperatorIsInstalled("cloud-native-postgresql") + Eventually(func() string { + out, _ := commonVar.CliRunner.GetBindableKinds() + return out + }, 120, 3).Should(ContainSubstring("Cluster")) + addBindableKind := commonVar.CliRunner.Run("apply", "-f", helper.GetExamplePath("manifests", "bindablekind-instance.yaml")) + Expect(addBindableKind.ExitCode()).To(BeEquivalentTo(0)) + }) + + When("creating local files and dir and running odo dev", func() { + var newDirPath, newFilePath, stdOut, podName string + var session helper.DevSession + // from devfile + devfileCmpName := "my-nodejs-app" + BeforeEach(func() { + newFilePath = filepath.Join(commonVar.Context, "foobar.txt") + newDirPath = filepath.Join(commonVar.Context, "testdir") + helper.CopyExampleDevFile(filepath.Join("source", "devfiles", "nodejs", "devfile-with-service-binding-files.yaml"), filepath.Join(commonVar.Context, "devfile.yaml")) + // Create a new file that we plan on deleting later... + if err := helper.CreateFileWithContent(newFilePath, "hello world"); err != nil { + fmt.Printf("the foobar.txt file was not created, reason %v", err.Error()) + } + // Create a new directory + helper.MakeDir(newDirPath) + var err error + session, _, _, _, err = helper.StartDevMode() + Expect(err).ToNot(HaveOccurred()) + }) + + AfterEach(func() { + session.Stop() + session.WaitEnd() + }) + + It("should correctly propagate changes to the container", func() { + + // Check to see if it's been pushed (foobar.txt abd directory testdir) + podName = commonVar.CliRunner.GetRunningPodNameByComponent(devfileCmpName, commonVar.Project) + + stdOut = commonVar.CliRunner.ExecListDir(podName, commonVar.Project, "/projects") + helper.MatchAllInOutput(stdOut, []string{"foobar.txt", "testdir"}) + }) + + When("deleting local files and dir and waiting for sync", func() { + BeforeEach(func() { + // Now we delete the file and dir and push + helper.DeleteDir(newFilePath) + helper.DeleteDir(newDirPath) + _, _, err := session.WaitSync() + Expect(err).ToNot(HaveOccurred()) + }) + It("should not list deleted dir and file in container", func() { + podName = commonVar.CliRunner.GetRunningPodNameByComponent(devfileCmpName, commonVar.Project) + // Then check to see if it's truly been deleted + stdOut = commonVar.CliRunner.ExecListDir(podName, commonVar.Project, "/projects") + helper.DontMatchAllInOutput(stdOut, []string{"foobar.txt", "testdir"}) + }) + }) + }) + }) + When("adding local files to gitignore and running odo dev", func() { var gitignorePath, newDirPath, newFilePath1, newFilePath2, newFilePath3, stdOut, podName string var session helper.DevSession