From 5f61602b8e61a638478d3dd8f88ee2aedf93c8b1 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Wed, 16 Jun 2021 07:27:34 +0200 Subject: [PATCH 01/15] Do not deploy sbr at `odo link` time, save it in devfile --- .../adapters/kubernetes/component/adapter.go | 51 ++++-- pkg/envinfo/envinfo.go | 65 +------ pkg/envinfo/envinfo_test.go | 69 -------- pkg/kclient/deployments.go | 69 +++++++- pkg/kclient/deployments_test.go | 8 +- pkg/odo/cli/component/common_link.go | 99 ++++------- pkg/odo/cli/component/delete.go | 6 - pkg/odo/cli/component/link.go | 2 +- pkg/odo/cli/component/unlink.go | 2 +- pkg/service/service.go | 162 ++++++++++++++---- pkg/storage/kubernetes.go | 7 + .../devfiles/nodejs/devfile-with-link.yaml | 95 ++++++++++ .../integration/operatorhub/cmd_link_test.go | 133 ++++++++++++++ .../operatorhub/cmd_service_test.go | 11 +- 14 files changed, 517 insertions(+), 262 deletions(-) create mode 100644 tests/examples/source/devfiles/nodejs/devfile-with-link.yaml create mode 100644 tests/integration/operatorhub/cmd_link_test.go diff --git a/pkg/devfile/adapters/kubernetes/component/adapter.go b/pkg/devfile/adapters/kubernetes/component/adapter.go index 69c589967f9..ff59bafa0ed 100644 --- a/pkg/devfile/adapters/kubernetes/component/adapter.go +++ b/pkg/devfile/adapters/kubernetes/component/adapter.go @@ -9,11 +9,10 @@ import ( "strings" "time" - "github.com/openshift/odo/pkg/service" - "github.com/devfile/library/pkg/devfile/generator" componentlabels "github.com/openshift/odo/pkg/component/labels" "github.com/openshift/odo/pkg/envinfo" + "github.com/openshift/odo/pkg/service" "github.com/openshift/odo/pkg/util" appsv1 "k8s.io/api/apps/v1" @@ -111,6 +110,7 @@ type Adapter struct { // Push updates the component if a matching component exists or creates one if it doesn't exist // Once the component has started, it will sync the source code to it. func (a Adapter) Push(parameters common.PushParameters) (err error) { + a.deployment, err = a.Client.GetKubeClient().GetOneDeployment(a.ComponentName, a.AppName) if err != nil { if _, ok := err.(*kclient.DeploymentNotFoundError); !ok { @@ -160,6 +160,30 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) { } s.End(true) + log.Info("\nUpdating services") + // fetch the "kubernetes inlined components" to create them on cluster + // from odo standpoint, these components contain yaml manifest of an odo service or an odo link + k8sComponents, err := a.Devfile.Data.GetComponents(parsercommon.DevfileOptions{ + ComponentOptions: parsercommon.ComponentOptions{ComponentType: devfilev1.KubernetesComponentType}, + }) + if err != nil { + return errors.Wrap(err, "error while trying to fetch service(s) from devfile") + } + labels := componentlabels.GetLabels(a.ComponentName, a.AppName, true) + // create the Kubernetes objects from the manifest and delete the ones not in the devfile + needRestart, err := service.PushServiceFromKubernetesInlineComponents(a.Client.GetKubeClient(), k8sComponents, labels) + if err != nil { + return errors.Wrap(err, "failed to create service(s) associated with the component") + } + + if componentExists && needRestart { + time.Sleep(time.Second) + a.deployment, err = a.Client.GetKubeClient().WaitForDeploymentRollout(a.deployment.Name) + if err != nil { + return errors.Wrap(err, "error while waiting for deployment rollout") + } + } + log.Infof("\nCreating Kubernetes resources for component %s", a.ComponentName) previousMode := parameters.EnvSpecificInfo.GetRunMode() @@ -183,21 +207,6 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) { return errors.Wrap(err, "unable to create or update component") } - // fetch the "kubernetes inlined components" to create them on cluster - // from odo standpoint, these components contain yaml manifest of an odo service or an odo link - k8sComponents, err := a.Devfile.Data.GetComponents(parsercommon.DevfileOptions{ - ComponentOptions: parsercommon.ComponentOptions{ComponentType: devfilev1.KubernetesComponentType}, - }) - if err != nil { - return errors.Wrap(err, "error while trying to fetch service(s) from devfile") - } - labels := componentlabels.GetLabels(a.ComponentName, a.AppName, true) - // create the Kubernetes objects from the manifest and delete the ones not in the devfile - err = service.PushServiceFromKubernetesInlineComponents(a.Client.GetKubeClient(), k8sComponents, labels) - if err != nil { - return errors.Wrap(err, "failed to create service(s) associated with the component") - } - a.deployment, err = a.Client.GetKubeClient().WaitForDeploymentRollout(a.deployment.Name) if err != nil { return errors.Wrap(err, "error while waiting for deployment rollout") @@ -215,17 +224,23 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) { return err } + ownerReference := generator.GetOwnerReference(a.deployment) // update the owner reference of the PVCs with the deployment for i := range pvcs { if pvcs[i].OwnerReferences != nil || pvcs[i].DeletionTimestamp != nil { continue } - err = a.Client.GetKubeClient().UpdateStorageOwnerReference(&pvcs[i], generator.GetOwnerReference(a.deployment)) + err = a.Client.GetKubeClient().UpdateStorageOwnerReference(&pvcs[i], ownerReference) if err != nil { return err } } + err = service.UpdateKubernetesInlineComponentsOwnerReferences(a.Client.GetKubeClient(), k8sComponents, ownerReference) + if err != nil { + return err + } + parameters.EnvSpecificInfo.SetDevfileObj(a.Devfile) err = component.ApplyConfig(&a.Client, config.LocalConfigInfo{}, parameters.EnvSpecificInfo, color.Output, componentExists, false) if err != nil { diff --git a/pkg/envinfo/envinfo.go b/pkg/envinfo/envinfo.go index ca8f52d9960..d765e091605 100644 --- a/pkg/envinfo/envinfo.go +++ b/pkg/envinfo/envinfo.go @@ -36,8 +36,7 @@ type ComponentSettings struct { URL *[]localConfigProvider.LocalURL `yaml:"Url,omitempty" json:"url,omitempty"` // AppName is the application name. Application is a virtual concept present in odo used // for grouping of components. A namespace can contain multiple applications - AppName string `yaml:"AppName,omitempty" json:"appName,omitempty"` - Link *[]EnvInfoLink `yaml:"Link,omitempty" json:"link,omitempty"` + AppName string `yaml:"AppName,omitempty" json:"appName,omitempty"` // DebugPort controls the port used by the pod to run the debugging agent on DebugPort *int `yaml:"DebugPort,omitempty" json:"debugPort,omitempty"` @@ -88,15 +87,6 @@ type EnvSpecificInfo struct { envinfoFileExists bool } -type EnvInfoLink struct { - // Name of link (same as name of k8s secret) - Name string `yaml:"Name,omitempty" json:"name,omitempty"` - // Kind of service with which the component is linked - ServiceKind string `yaml:"ServiceKind,omitempty" json:"serviceKind,omitempty"` - // Name of the instance of the ServiceKind that component is linked with - ServiceName string `yaml:"ServiceName,omitempty" json:"serviceName,omitempty"` -} - func WrapForJSONOutput(compSettings ComponentSettings) JSONEnvInfoRepr { return JSONEnvInfoRepr{ TypeMeta: metav1.TypeMeta{ @@ -216,14 +206,6 @@ func (esi *EnvSpecificInfo) SetConfiguration(parameter string, value interface{} } else { esi.componentSettings.URL = &[]localConfigProvider.LocalURL{urlValue} } - - case "link": - linkValue := value.(EnvInfoLink) - if esi.componentSettings.Link != nil { - *esi.componentSettings.Link = append(*esi.componentSettings.Link, linkValue) - } else { - esi.componentSettings.Link = &[]EnvInfoLink{linkValue} - } } return esi.writeToFile() @@ -305,26 +287,6 @@ func (esi *EnvSpecificInfo) DeleteConfiguration(parameter string) error { } -func (esi *EnvSpecificInfo) DeleteLink(parameter string) error { - index := -1 - - for i, link := range *esi.componentSettings.Link { - if link.Name == parameter { - index = i - break - } - } - - if index != -1 { - s := *esi.componentSettings.Link - s = append(s[:index], s[index+1:]...) - esi.componentSettings.Link = &s - return esi.writeToFile() - } else { - return nil - } -} - // GetComponentSettings returns the componentSettings from envinfo func (esi *EnvSpecificInfo) GetComponentSettings() ComponentSettings { return esi.componentSettings @@ -431,26 +393,6 @@ func (ei *EnvInfo) SetIsRouteSupported(isRouteSupported bool) { ei.isRouteSupported = isRouteSupported } -// GetLink returns the EnvInfoLink, returns default if nil -func (ei *EnvInfo) GetLink() []EnvInfoLink { - if ei.componentSettings.Link == nil { - return []EnvInfoLink{} - } - return *ei.componentSettings.Link -} - -// SearchLinkName searches for a Link with given service kind and service name -// and returns its name if found -func (ei *EnvInfo) SearchLinkName(serviceKind, serviceName string) (string, bool) { - links := ei.GetLink() - for _, link := range links { - if link.ServiceKind == serviceKind && link.ServiceName == serviceName { - return link.Name, true - } - } - return "", false -} - const ( // Name is the name of the setting controlling the component name Name = "Name" @@ -472,10 +414,6 @@ const ( Push = "PUSH" // PushDescription is the description of push parameter PushDescription = "Push parameter is the action to write devfile commands to env.yaml" - // Link parameter - Link = "LINK" - // LinkDescription is the description of Link - LinkDescription = "Link to an Operator backed service" ) var ( @@ -485,7 +423,6 @@ var ( DebugPort: DebugPortDescription, URL: URLDescription, Push: PushDescription, - Link: LinkDescription, } lowerCaseLocalParameters = util.GetLowerCaseParameters(GetLocallySupportedParameters()) diff --git a/pkg/envinfo/envinfo_test.go b/pkg/envinfo/envinfo_test.go index 71be91db01d..6fc6f2ceeaf 100644 --- a/pkg/envinfo/envinfo_test.go +++ b/pkg/envinfo/envinfo_test.go @@ -893,75 +893,6 @@ func TestRemoveEndpointInDevfile(t *testing.T) { } } -func TestSearchLinkName(t *testing.T) { - tests := []struct { - name string - ei *EnvInfo - serviceKind string - serviceName string - want string - wantFound bool - }{ - { - name: "Case 1: Existing link", - ei: &EnvInfo{ - componentSettings: ComponentSettings{ - Link: &[]EnvInfoLink{ - { - Name: "a first name", - ServiceKind: "a first kind", - ServiceName: "a first service name", - }, - { - Name: "a name", - ServiceKind: "a kind", - ServiceName: "a service name", - }, - }, - }, - }, - serviceKind: "a kind", - serviceName: "a service name", - want: "a name", - wantFound: true, - }, - { - name: "Case 2: Non existing link", - ei: &EnvInfo{ - componentSettings: ComponentSettings{ - Link: &[]EnvInfoLink{ - { - Name: "a first name", - ServiceKind: "a first kind", - ServiceName: "a first service name", - }, - { - Name: "a name", - ServiceKind: "a kind", - ServiceName: "a service name", - }, - }, - }, - }, - serviceKind: "an unknown kind", - serviceName: "a service name", - wantFound: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result, found := tt.ei.SearchLinkName(tt.serviceKind, tt.serviceName) - if found != tt.wantFound { - t.Errorf("Expected found %v, got %v", tt.wantFound, found) - } - if found && result != tt.want { - t.Errorf("Expected %q, got %q", tt.want, result) - } - }) - } -} - func createDirectoryAndFile(create bool, fs filesystem.Filesystem, odoDir string) error { if !create { return nil diff --git a/pkg/kclient/deployments.go b/pkg/kclient/deployments.go index cc964d80df4..8486830b62b 100644 --- a/pkg/kclient/deployments.go +++ b/pkg/kclient/deployments.go @@ -12,6 +12,7 @@ import ( "github.com/pkg/errors" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/fields" @@ -225,13 +226,17 @@ func (c *Client) UpdateDeployment(deploy appsv1.Deployment) (*appsv1.Deployment, // odo overrides it with the value it expects instead of failing due to conflict. func (c *Client) ApplyDeployment(deploy appsv1.Deployment) (*appsv1.Deployment, error) { data, err := json.Marshal(deploy) - + if err != nil { + return nil, errors.Wrapf(err, "unable to marshal deployment") + } klog.V(5).Infoln("Applying Deployment via server-side apply:") klog.V(5).Infoln(resourceAsJson(deploy)) + err = c.removeDuplicateEnv(deploy) if err != nil { - return nil, errors.Wrapf(err, "unable to marshal deployment") + return nil, err } + deployment, err := c.KubeClient.AppsV1().Deployments(c.Namespace).Patch(context.TODO(), deploy.Name, types.ApplyPatchType, data, metav1.PatchOptions{FieldManager: FieldManager, Force: boolPtr(true)}) if err != nil { return nil, errors.Wrapf(err, "unable to update Deployment %s", deploy.Name) @@ -239,6 +244,55 @@ func (c *Client) ApplyDeployment(deploy appsv1.Deployment) (*appsv1.Deployment, return deployment, nil } +// removeDuplicateEnv removes duplicate environment variables from containers, due to a bug in Service Binding Operator: +// https://github.com/redhat-developer/service-binding-operator/issues/983 +func (c *Client) removeDuplicateEnv(deployment appsv1.Deployment) error { + _, err := c.KubeClient.AppsV1().Deployments(c.Namespace).Get(context.Background(), deployment.Name, metav1.GetOptions{}) + if kerrors.IsNotFound(err) { + return nil + } + if err != nil { + return err + } + containers := deployment.Spec.Template.Spec.Containers + for i, container := range containers { + duplicates := []corev1.EnvVar{} + found := map[string]bool{} + envs := container.Env + for _, env := range envs { + if _, ok := found[env.Name]; ok { + duplicates = append(duplicates, env) + } else { + found[env.Name] = true + } + } + if len(duplicates) > 0 { + newEnv := []corev1.EnvVar{} + // first remove duplicates + for _, env := range envs { + found := false + for _, duplicate := range duplicates { + if env.Name == duplicate.Name { + found = true + break + } + } + if !found { + newEnv = append(newEnv, env) + } + } + // next add them as single values + newEnv = append(newEnv, duplicates...) + containers[i].Env = newEnv + } + } + _, err = c.KubeClient.AppsV1().Deployments(c.Namespace).Update(context.Background(), &deployment, metav1.UpdateOptions{}) + if kerrors.IsNotFound(err) { + return nil + } + return err +} + // DeleteDeployment deletes the deployments with the given selector func (c *Client) DeleteDeployment(labels map[string]string) error { if labels == nil { @@ -299,6 +353,17 @@ func (c *Client) GetDynamicResource(group, version, resource, name string) (*uns return res, nil } +// UpdateDynamicResource updates a dynamic resource +func (c *Client) UpdateDynamicResource(group, version, resource, name string, u *unstructured.Unstructured) error { + deploymentRes := schema.GroupVersionResource{Group: group, Version: version, Resource: resource} + + _, err := c.DynamicClient.Resource(deploymentRes).Namespace(c.Namespace).Update(context.TODO(), u, metav1.UpdateOptions{}) + if err != nil { + return err + } + return nil +} + // DeleteDynamicResource deletes an instance, specified by name, of a Custom Resource func (c *Client) DeleteDynamicResource(name, group, version, resource string) error { deploymentRes := schema.GroupVersionResource{Group: group, Version: version, Resource: resource} diff --git a/pkg/kclient/deployments_test.go b/pkg/kclient/deployments_test.go index 13d8160023a..1a20408f6dd 100644 --- a/pkg/kclient/deployments_test.go +++ b/pkg/kclient/deployments_test.go @@ -114,8 +114,8 @@ func TestCreateDeployment(t *testing.T) { if err == nil { - if len(fkclientset.Kubernetes.Actions()) != 1 { - t.Errorf("expected 1 action in StartDeployment got: %v", fkclientset.Kubernetes.Actions()) + if len(fkclientset.Kubernetes.Actions()) != 2 { + t.Errorf("expected 2 action in StartDeployment got %d: %v", len(fkclientset.Kubernetes.Actions()), fkclientset.Kubernetes.Actions()) } else { if createdDeployment.Name != tt.deploymentName { t.Errorf("deployment name does not match the expected name, expected: %s, got %s", tt.deploymentName, createdDeployment.Name) @@ -261,8 +261,8 @@ func TestUpdateDeployment(t *testing.T) { if err == nil { - if len(fkclientset.Kubernetes.Actions()) != 1 { - t.Errorf("expected 1 action in UpdateDeployment got: %v", fkclientset.Kubernetes.Actions()) + if len(fkclientset.Kubernetes.Actions()) != 2 { + t.Errorf("expected 2 action in UpdateDeployment got %d: %v", len(fkclientset.Kubernetes.Actions()), fkclientset.Kubernetes.Actions()) } else { if updatedDeployment.Name != tt.deploymentName { t.Errorf("deployment name does not match the expected name, expected: %s, got %s", tt.deploymentName, updatedDeployment.Name) diff --git a/pkg/odo/cli/component/common_link.go b/pkg/odo/cli/component/common_link.go index 6db585bdcac..217c6d04a87 100644 --- a/pkg/odo/cli/component/common_link.go +++ b/pkg/odo/cli/component/common_link.go @@ -5,10 +5,8 @@ import ( "fmt" "strings" - "github.com/devfile/library/pkg/devfile/generator" "github.com/openshift/odo/pkg/component" componentlabels "github.com/openshift/odo/pkg/component/labels" - "github.com/openshift/odo/pkg/envinfo" "github.com/openshift/odo/pkg/kclient" "github.com/openshift/odo/pkg/log" "github.com/openshift/odo/pkg/occlient" @@ -18,6 +16,7 @@ import ( "github.com/openshift/odo/pkg/util" servicebinding "github.com/redhat-developer/service-binding-operator/api/v1alpha1" "github.com/spf13/cobra" + "gopkg.in/yaml.v2" corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -54,7 +53,7 @@ func newCommonLinkOptions() *commonLinkOptions { } // Complete completes LinkOptions after they've been created -func (o *commonLinkOptions) complete(name string, cmd *cobra.Command, args []string) (err error) { +func (o *commonLinkOptions) complete(name string, cmd *cobra.Command, args []string, context string) (err error) { o.csvSupport, _ = svc.IsCSVSupported() o.operationName = name @@ -71,7 +70,11 @@ func (o *commonLinkOptions) complete(name string, cmd *cobra.Command, args []str // and must create s2i context instead o.Context, err = genericclioptions.NewContextCreatingAppIfNeeded(cmd) } else { - o.Context, err = genericclioptions.NewDevfileContext(cmd) + o.Context, err = genericclioptions.New(genericclioptions.CreateParameters{ + Cmd: cmd, + DevfilePath: component.DevfilePath, + ComponentContext: context, + }) } if err != nil { @@ -273,11 +276,6 @@ func (o *commonLinkOptions) getServiceBindingName(componentName string) string { return strings.Join([]string{componentName, strings.ToLower(o.serviceType), o.serviceName}, "-") } -// getSvcFullName returns service name in the format / -func getSvcFullName(serviceType, serviceName string) string { - return strings.Join([]string{serviceType, serviceName}, "/") -} - // completeForOperator completes the options when svc is supported func (o *commonLinkOptions) completeForOperator() (err error) { serviceBindingSupport, err := o.Client.GetKubeClient().IsServiceBindingSupported() @@ -309,9 +307,6 @@ func (o *commonLinkOptions) completeForOperator() (err error) { return err } - // make this deployment the owner of the link we're creating so that link gets deleted upon doing "odo delete" - ownerReference := generator.GetOwnerReference(deployment) - deploymentGVR, err := o.KClient.GetDeploymentAPIVersion() if err != nil { return err @@ -323,8 +318,7 @@ func (o *commonLinkOptions) completeForOperator() (err error) { Kind: kclient.ServiceBindingKind, }, ObjectMeta: metav1.ObjectMeta{ - Name: o.getServiceBindingName(componentName), - Namespace: o.EnvSpecificInfo.GetNamespace(), + Name: o.getServiceBindingName(componentName), }, Spec: servicebinding.ServiceBindingSpec{ DetectBindingResources: true, @@ -339,15 +333,17 @@ func (o *commonLinkOptions) completeForOperator() (err error) { }, }, } - o.serviceBinding.SetOwnerReferences(append(o.serviceBinding.GetOwnerReferences(), ownerReference)) return nil } // validateForOperator validates the options when svc is supported func (o *commonLinkOptions) validateForOperator() (err error) { var svcFullName string + var linkType string + if o.isTargetAService { // let's validate if the service exists + linkType = "service" svcFullName = strings.Join([]string{o.serviceType, o.serviceName}, "/") svcExists, err := svc.OperatorSvcExists(o.KClient, svcFullName) if err != nil { @@ -357,6 +353,9 @@ func (o *commonLinkOptions) validateForOperator() (err error) { return fmt.Errorf("couldn't find service named %q. Refer %q to see list of running services", svcFullName, "odo service list") } } else { + o.serviceType = "Service" + linkType = "component" + svcFullName = o.serviceName if o.suppliedName == o.EnvSpecificInfo.GetName() { return fmt.Errorf("the component %q cannot be linked with itself", o.suppliedName) } @@ -371,29 +370,12 @@ func (o *commonLinkOptions) validateForOperator() (err error) { } if o.operationName == unlink { - serviceBindingName, found := o.EnvSpecificInfo.SearchLinkName(o.serviceType, o.serviceName) - if !found { - if o.isTargetAService { - return fmt.Errorf("failed to unlink the service %q since no link was found in the env file", svcFullName) - } else { - return fmt.Errorf("failed to unlink the component %q since no link was found in the env file", o.suppliedName) - } - } - - // Verify if the underlying service binding request actually exists - serviceBindingSvcFullName := strings.Join([]string{kclient.ServiceBindingKind, serviceBindingName}, "/") - serviceBindingExists, err := svc.OperatorSvcExists(o.KClient, serviceBindingSvcFullName) + _, found, err := svc.FindDevfileServiceBinding(o.EnvSpecificInfo.GetDevfileObj(), o.serviceType, o.serviceName) if err != nil { return err } - if !serviceBindingExists { - // This could have happened if the service binding was deleted outside odo workflow (eg: oc delete sb/) - // we must remove entry of the link from env.yaml in this case - err = o.Context.EnvSpecificInfo.DeleteLink(serviceBindingName) - if err != nil { - return fmt.Errorf("component's link with %q has been deleted outside odo; unable to delete odo's state of the link", svcFullName) - } - return fmt.Errorf("component's link with %q has been deleted outside odo", svcFullName) + if !found { + return fmt.Errorf("failed to unlink the %s %q since no link was found in the configuration referring this %s", linkType, svcFullName, linkType) } return nil } @@ -421,7 +403,6 @@ func (o *commonLinkOptions) validateForOperator() (err error) { Kind: kind, Name: o.serviceName, }, - Namespace: &o.KClient.Namespace, }, } } else { @@ -432,7 +413,6 @@ func (o *commonLinkOptions) validateForOperator() (err error) { Kind: "Service", Name: o.serviceName, }, - Namespace: &o.KClient.Namespace, }, } } @@ -446,32 +426,33 @@ func (o *commonLinkOptions) validateForOperator() (err error) { // the current component with the given component's service // and stores the link info in the env func (o *commonLinkOptions) linkOperator() (err error) { - // convert service binding request into a map[string]interface{} type so - // as to use it with dynamic client - serviceBindingMap := make(map[string]interface{}) + // Convert ServiceBinding -> JSON -> Map -> YAML + // JSON conversion step is necessary to inline TypeMeta + intermediate, err := json.Marshal(o.serviceBinding) if err != nil { return err } + + serviceBindingMap := make(map[string]interface{}) err = json.Unmarshal(intermediate, &serviceBindingMap) if err != nil { return err } - // this creates a link by creating a service of type - // "ServiceBindingRequest" from the Operator "ServiceBindingOperator". - err = o.KClient.CreateDynamicResource(serviceBindingMap, kclient.ServiceBindingGroup, kclient.ServiceBindingVersion, kclient.ServiceBindingResource) + yamlDesc, err := yaml.Marshal(serviceBindingMap) if err != nil { - if strings.Contains(err.Error(), "already exists") { - return fmt.Errorf("component %q is already linked with the service %q", o.Context.EnvSpecificInfo.GetName(), o.suppliedName) - } return err } - // once the link is created, we need to store the information in - // env.yaml so that subsequent odo push can create a new deployment - // based on it - err = o.Context.EnvSpecificInfo.SetConfiguration("link", envinfo.EnvInfoLink{Name: o.serviceBinding.GetName(), ServiceKind: o.serviceType, ServiceName: o.serviceName}) + _, found, err := svc.FindDevfileServiceBinding(o.EnvSpecificInfo.GetDevfileObj(), o.serviceType, o.serviceName) + if err != nil { + return err + } + if found { + return fmt.Errorf("component %q is already linked with the service %q", o.Context.EnvSpecificInfo.GetName(), o.suppliedName) + } + err = svc.AddKubernetesComponentToDevfile(string(yamlDesc), o.serviceBinding.Name, o.EnvSpecificInfo.GetDevfileObj()) if err != nil { return err } @@ -485,20 +466,16 @@ func (o *commonLinkOptions) linkOperator() (err error) { return err } -// unlinkOperator deletes the service binding resource from the cluster -// and deletes the link info from the env +// unlinkOperator deletes the service binding resource from the devfile func (o *commonLinkOptions) unlinkOperator() (err error) { - serviceBindingName, found := o.EnvSpecificInfo.SearchLinkName(o.serviceType, o.serviceName) - if !found { - return fmt.Errorf("failed to unlink the service %q of type %q since no link found in env file", o.serviceName, o.serviceType) - } - svcFullName := getSvcFullName(kclient.ServiceBindingKind, serviceBindingName) - err = svc.DeleteServiceBindingRequest(o.KClient, svcFullName) + + // We already tested `found` in `validateForOperator` + name, _, err := svc.FindDevfileServiceBinding(o.EnvSpecificInfo.GetDevfileObj(), o.serviceType, o.serviceName) if err != nil { return err } - err = o.Context.EnvSpecificInfo.DeleteLink(serviceBindingName) + err = svc.DeleteKubernetesComponentFromDevfile(name, o.EnvSpecificInfo.GetDevfileObj()) if err != nil { return err } @@ -507,9 +484,7 @@ func (o *commonLinkOptions) unlinkOperator() (err error) { if o.isTargetAService { targetType = "service" } - log.Successf("Successfully unlinked component %q from %s %q\n", o.Context.EnvSpecificInfo.GetName(), targetType, o.suppliedName) log.Italic("To apply the changes, please use `odo push`") - - return + return nil } diff --git a/pkg/odo/cli/component/delete.go b/pkg/odo/cli/component/delete.go index 59506da1a0d..7eddb1b839f 100644 --- a/pkg/odo/cli/component/delete.go +++ b/pkg/odo/cli/component/delete.go @@ -234,12 +234,6 @@ func (do *DeleteOptions) DevFileRun() (err error) { if err != nil { log.Errorf("error occurred while deleting component, cause: %v", err) } - - // delete the information about link of the components because deleting a component also deletes its links (Service Binding Requests) - err = do.EnvSpecificInfo.DeleteConfiguration("link") - if err != nil { - log.Errorf("error occurred while deleting environment specific information of the component, cause: %v", err) - } } else { log.Error("Aborting deletion of component") } diff --git a/pkg/odo/cli/component/link.go b/pkg/odo/cli/component/link.go index 724ebe43dee..00c6fc3134e 100644 --- a/pkg/odo/cli/component/link.go +++ b/pkg/odo/cli/component/link.go @@ -103,7 +103,7 @@ func (o *LinkOptions) Complete(name string, cmd *cobra.Command, args []string) ( o.commonLinkOptions.devfilePath = filepath.Join(o.componentContext, DevfilePath) o.commonLinkOptions.csvSupport, _ = svc.IsCSVSupported() - err = o.complete(name, cmd, args) + err = o.complete(name, cmd, args, o.componentContext) if err != nil { return err } diff --git a/pkg/odo/cli/component/unlink.go b/pkg/odo/cli/component/unlink.go index 35f6fe4c171..25ddbc8b602 100644 --- a/pkg/odo/cli/component/unlink.go +++ b/pkg/odo/cli/component/unlink.go @@ -55,7 +55,7 @@ func NewUnlinkOptions() *UnlinkOptions { // Complete completes UnlinkOptions after they've been created func (o *UnlinkOptions) Complete(name string, cmd *cobra.Command, args []string) (err error) { o.commonLinkOptions.csvSupport, _ = svc.IsCSVSupported() - err = o.complete(name, cmd, args) + err = o.complete(name, cmd, args, o.componentContext) if err != nil { return err } diff --git a/pkg/service/service.go b/pkg/service/service.go index 72f5e36e7d9..06fe7621b75 100644 --- a/pkg/service/service.go +++ b/pkg/service/service.go @@ -28,6 +28,7 @@ import ( "github.com/pkg/errors" "github.com/devfile/library/pkg/devfile/parser" + servicebinding "github.com/redhat-developer/service-binding-operator/api/v1alpha1" ) const provisionedAndBoundStatus = "ProvisionedAndBound" @@ -148,12 +149,6 @@ func DeleteServiceAndUnlinkComponents(client *occlient.Client, serviceName strin return nil } -// DeleteServiceBindingRequest deletes a service binding request (when user -// does odo unlink). It's just a wrapper on DeleteOperatorService -func DeleteServiceBindingRequest(client *kclient.Client, serviceName string) error { - return DeleteOperatorService(client, serviceName) -} - // DeleteOperatorService deletes an Operator backed service // TODO: make it unlink the service from component as a part of // https://github.com/openshift/odo/issues/3563 @@ -735,6 +730,44 @@ func ListDevfileServices(devfileObj parser.DevfileObj) ([]string, error) { return services, nil } +// FindDevfileServiceBinding returns the name of the ServiceBinding defined in a Devfile matching kind and name +func FindDevfileServiceBinding(devfileObj parser.DevfileObj, kind string, name string) (string, bool, error) { + if devfileObj.Data == nil { + return "", false, nil + } + components, err := devfileObj.Data.GetComponents(common.DevfileOptions{ + ComponentOptions: parsercommon.ComponentOptions{ComponentType: devfile.KubernetesComponentType}, + }) + if err != nil { + return "", false, err + } + + for _, c := range components { + var u unstructured.Unstructured + err = yaml.Unmarshal([]byte(c.Kubernetes.Inlined), &u) + if err != nil { + return "", false, err + } + + if isLinkResource(u.GetKind()) { + var sbr servicebinding.ServiceBinding + err = yaml.Unmarshal([]byte(c.Kubernetes.Inlined), &sbr) + if err != nil { + return "", false, err + } + services := sbr.Spec.Services + if len(services) != 1 { + continue + } + service := services[0] + if service.Kind == kind && service.Name == name { + return u.GetName(), true, nil + } + } + } + return "", false, nil +} + // AddKubernetesComponentToDevfile adds service definition to devfile as an inlined Kubernetes component func AddKubernetesComponentToDevfile(crd, name string, devfileObj parser.DevfileObj) error { err := devfileObj.Data.AddComponents([]devfile.Component{{ @@ -857,32 +890,41 @@ func (d *DynamicCRD) AddComponentLabelsToCRD(labels map[string]string) { } // PushServiceFromKubernetesInlineComponents updates service(s) from Kubernetes Inlined component in a devfile by creating new ones or removing old ones -func PushServiceFromKubernetesInlineComponents(client *kclient.Client, k8sComponents []devfile.Component, labels map[string]string) error { +// returns true if the component needs to be restarted (when a service binding has been created or deleted) +func PushServiceFromKubernetesInlineComponents(client *kclient.Client, k8sComponents []devfile.Component, labels map[string]string) (bool, error) { // check csv support before proceeding csvSupported, err := IsCSVSupported() if err != nil || !csvSupported { - return err + return false, err } - created := []string{} - deleted := []string{} + type DeployedInfo struct { + DoesDeleteRestartsComponent bool + Kind string + Name string + } - deployed := map[string]struct{}{} + deployed := map[string]DeployedInfo{} deployedServices, _, err := ListOperatorServices(client) if err != nil && err != kclient.ErrNoSuchOperator { // We ignore ErrNoSuchOperator error as we can deduce Operator Services are not installed - return err + return false, err } for _, svc := range deployedServices { name := svc.GetName() kind := svc.GetKind() deployedLabels := svc.GetLabels() if deployedLabels[applabels.ManagedBy] == "odo" && deployedLabels[componentlabels.ComponentLabel] == labels[componentlabels.ComponentLabel] { - deployed[kind+"/"+name] = struct{}{} + deployed[kind+"/"+name] = DeployedInfo{ + DoesDeleteRestartsComponent: isLinkResource(kind), + Kind: kind, + Name: name, + } } } + needRestart := false // create an object on the kubernetes cluster for all the Kubernetes Inlined components for _, c := range k8sComponents { @@ -893,12 +935,12 @@ func PushServiceFromKubernetesInlineComponents(client *kclient.Client, k8sCompon d := NewDynamicCRD() err := yaml.Unmarshal([]byte(strCRD), &d.OriginalCRD) if err != nil { - return err + return false, err } cr, csv, err := GetCSV(client, d.OriginalCRD) if err != nil { - return err + return false, err } var group, version, kind, resource string @@ -906,7 +948,7 @@ func PushServiceFromKubernetesInlineComponents(client *kclient.Client, k8sCompon if crd.Kind == cr { group, version, kind, resource, err = getGVKRFromCR(crd) if err != nil { - return err + return false, err } break } @@ -920,6 +962,11 @@ func PushServiceFromKubernetesInlineComponents(client *kclient.Client, k8sCompon continue } + if _, found := deployed[cr+"/"+crdName]; !found && isLinkResource(cr) { + // If creating the ServiceBinding, first delete the component + needRestart = true + } + delete(deployed, cr+"/"+crdName) // create the service on cluster @@ -930,35 +977,84 @@ func PushServiceFromKubernetesInlineComponents(client *kclient.Client, k8sCompon // TODO: better way to handle this might be introduced by https://github.com/openshift/odo/issues/4553 continue // this ensures that services slice is not updated } else { - return err + return false, err } } name, _ := d.GetServiceNameFromCRD() // ignoring error because invalid yaml won't be inserted into devfile through odo - created = append(created, strings.Join([]string{kind, name}, "/")) + if isLinkResource(cr) { + log.Successf("Created link %q on the cluster; component will be restarted", name) + } else { + log.Successf("Created service %q on the cluster; refer %q to know how to link it to the component", strings.Join([]string{kind, name}, "/"), "odo link -h") + } } - for key := range deployed { + for key, val := range deployed { err = DeleteOperatorService(client, key) if err != nil { - return err + return false, err } - deleted = append(deleted, key) - } - if len(created) == 1 { - log.Successf("Created service %q on the cluster; refer %q to know how to link it to the component", created[0], "odo link -h") - } else if len(created) > 1 { - log.Successf("Created services %q on the cluster; refer %q to know how to link them to the component", strings.Join(created, ", "), "odo link -h") - } + if isLinkResource(val.Kind) { + log.Successf("Deleted link %q on the cluster; component will be restarted", val.Name) + } else { + log.Successf("Deleted service %q from the cluster", key) + } - if len(deleted) == 1 { - log.Successf("Deleted service %q from the cluster", deleted[0]) - } else if len(deleted) > 1 { - log.Successf("Deleted services %q from the cluster", strings.Join(deleted, ", ")) + if val.DoesDeleteRestartsComponent { + needRestart = true + } } + return needRestart, nil +} + +func UpdateKubernetesInlineComponentsOwnerReferences(client *kclient.Client, k8sComponents []devfile.Component, ownerReference metav1.OwnerReference) error { + for _, c := range k8sComponents { + // get the string representation of the YAML definition of a CRD + strCRD := c.Kubernetes.Inlined + + // convert the YAML definition into map[string]interface{} since it's needed to create dynamic resource + d := NewDynamicCRD() + err := yaml.Unmarshal([]byte(strCRD), &d.OriginalCRD) + if err != nil { + return err + } + + cr, csv, err := GetCSV(client, d.OriginalCRD) + if err != nil { + return err + } + + var group, version, resource string + for _, crd := range csv.Spec.CustomResourceDefinitions.Owned { + if crd.Kind == cr { + group, version, _, resource, err = getGVKRFromCR(crd) + if err != nil { + return err + } + break + } + } + + crdName, ok := getCRDName(d.OriginalCRD) + if !ok { + continue + } + + u, err := client.GetDynamicResource(group, version, resource, crdName) + if err != nil { + return err + } + + u.SetOwnerReferences(append(u.GetOwnerReferences(), ownerReference)) + + err = client.UpdateDynamicResource(group, version, resource, crdName, u) + if err != nil { + return err + } + } return nil } @@ -973,3 +1069,7 @@ func getCRDName(crd map[string]interface{}) (string, bool) { } return name, true } + +func isLinkResource(kind string) bool { + return kind == "ServiceBinding" +} diff --git a/pkg/storage/kubernetes.go b/pkg/storage/kubernetes.go index d1d5aae87ba..3128bb8fd3c 100644 --- a/pkg/storage/kubernetes.go +++ b/pkg/storage/kubernetes.go @@ -149,6 +149,13 @@ func (k kubernetesClient) ListFromCluster() (StorageList, error) { } } + // to track volumes created by Service Binding Operator + for _, volume := range pod.Spec.Volumes { + if volume.Secret != nil { + validVolumeMounts[volume.Name] = true + } + } + for _, volumeMount := range volumeMounts { if _, ok := validVolumeMounts[volumeMount.Name]; !ok { return StorageList{}, fmt.Errorf("pvc not found for mount path %s", volumeMount.Name) diff --git a/tests/examples/source/devfiles/nodejs/devfile-with-link.yaml b/tests/examples/source/devfiles/nodejs/devfile-with-link.yaml new file mode 100644 index 00000000000..e365f55f3a3 --- /dev/null +++ b/tests/examples/source/devfiles/nodejs/devfile-with-link.yaml @@ -0,0 +1,95 @@ +commands: +- exec: + commandLine: npm install + component: runtime + group: + isDefault: true + kind: build + workingDir: /project + id: install +- exec: + commandLine: npm start + component: runtime + group: + isDefault: true + kind: run + workingDir: /project + id: run +- exec: + commandLine: npm run debug + component: runtime + group: + isDefault: true + kind: debug + workingDir: /project + id: debug +- exec: + commandLine: npm test + component: runtime + group: + isDefault: true + kind: test + workingDir: /project + id: test +components: +- container: + endpoints: + - name: http-3000 + targetPort: 3000 + image: registry.access.redhat.com/ubi8/nodejs-14:latest + memoryLimit: 1024Mi + mountSources: true + sourceMapping: /project + name: runtime +- kubernetes: + inlined: | + apiVersion: etcd.database.coreos.com/v1beta2 + kind: EtcdCluster + metadata: + annotations: + etcd.database.coreos.com/scope: clusterwide + name: myetcd + spec: + size: 1 + version: 3.2.13 + name: myetcd +- kubernetes: + inlined: | + apiVersion: binding.operators.coreos.com/v1alpha1 + kind: ServiceBinding + metadata: + creationTimestamp: null + name: etcd-link + spec: + application: + group: apps + name: api-app + resource: deployments + version: v1 + bindAsFiles: true + detectBindingResources: true + services: + - group: etcd.database.coreos.com + kind: EtcdCluster + name: myetcd + version: v1beta2 + status: + secret: "" + name: etcd-link +metadata: + description: Stack with Node.js 14 + displayName: Node.js Runtime + language: nodejs + name: nodejs + projectType: nodejs + tags: + - NodeJS + - Express + - ubi8 + version: 1.0.1 +schemaVersion: 2.0.0 +starterProjects: +- git: + remotes: + origin: https://github.com/odo-devfiles/nodejs-ex.git + name: nodejs-starter diff --git a/tests/integration/operatorhub/cmd_link_test.go b/tests/integration/operatorhub/cmd_link_test.go new file mode 100644 index 00000000000..7f1fa925540 --- /dev/null +++ b/tests/integration/operatorhub/cmd_link_test.go @@ -0,0 +1,133 @@ +package integration + +import ( + "fmt" + "path/filepath" + "regexp" + "strings" + + . "github.com/onsi/ginkgo" + "github.com/openshift/odo/tests/helper" +) + +var _ = Describe("odo link command tests for OperatorHub", func() { + + var commonVar helper.CommonVar + var oc helper.OcRunner + + BeforeEach(func() { + commonVar = helper.CommonBeforeEach() + helper.Chdir(commonVar.Context) + oc = helper.NewOcRunner("oc") + }) + + AfterEach(func() { + helper.CommonAfterEach(commonVar) + }) + + Context("Operators are installed in the cluster", func() { + + var etcdOperator string + var etcdCluster string + + BeforeEach(func() { + // wait till odo can see that all operators installed by setup script in the namespace + odoArgs := []string{"catalog", "list", "services"} + operators := []string{"etcdoperator", "service-binding-operator"} + for _, operator := range operators { + helper.WaitForCmdOut("odo", odoArgs, 5, true, func(output string) bool { + return strings.Contains(output, operator) + }) + } + + list := helper.Cmd("odo", "catalog", "list", "services").ShouldPass().Out() + etcdOperator = regexp.MustCompile(`etcdoperator\.*[a-z][0-9]\.[0-9]\.[0-9]-clusterwide`).FindString(list) + etcdCluster = fmt.Sprintf("%s/EtcdCluster", etcdOperator) + }) + + AfterEach(func() { + helper.DeleteProject(commonVar.Project) + }) + + When("a component and a service are deployed", func() { + + var componentName string + var svcFullName string + + BeforeEach(func() { + helper.CopyExample(filepath.Join("source", "nodejs"), commonVar.Context) + componentName = "cmp-" + helper.RandString(6) + helper.Cmd("odo", "create", "nodejs", componentName).ShouldPass() + + serviceName := "service-" + helper.RandString(6) + svcFullName = strings.Join([]string{"EtcdCluster", serviceName}, "/") + helper.Cmd("odo", "service", "create", etcdCluster, serviceName, "--project", commonVar.Project).ShouldPass() + + helper.Cmd("odo", "push").ShouldPass() + oc.PodsShouldBeRunning(commonVar.Project, componentName+`-app-.[a-z0-9-]*`) + }) + + It("should find files in component container", func() { + helper.Cmd("odo", "exec", "--", "ls", "/project/server.js").ShouldPass() + }) + + When("a link between the component and the service is created and deployed", func() { + + BeforeEach(func() { + helper.Cmd("odo", "link", svcFullName).ShouldPass() + helper.Cmd("odo", "push").ShouldPass() + oc.PodsShouldBeRunning(commonVar.Project, componentName+`-app-.[a-z0-9-]*`) + }) + + It("should find files in component container", func() { + helper.Cmd("odo", "exec", "--", "ls", "/project/server.js").ShouldPass() + }) + }) + + When("a link with between the component and the service is created with --bind-as-files and deployed", func() { + + BeforeEach(func() { + helper.Cmd("odo", "link", svcFullName, "--bind-as-files").ShouldPass() + helper.Cmd("odo", "push").ShouldPass() + oc.PodsShouldBeRunning(commonVar.Project, componentName+`-app-.[a-z0-9-]*`) + }) + + It("should find files in component container", func() { + helper.Cmd("odo", "exec", "--", "ls", "/project/server.js").ShouldPass() + }) + }) + }) + + When("getting sources, a devfile defining a component, a service and a link, and executing odo push", func() { + + BeforeEach(func() { + helper.CopyExample(filepath.Join("source", "nodejs"), commonVar.Context) + helper.CopyExampleDevFile(filepath.Join("source", "devfiles", "nodejs", "devfile-with-link.yaml"), filepath.Join(commonVar.Context, "devfile.yaml")) + helper.Cmd("odo", "create", "api").ShouldPass() + + helper.Cmd("odo", "push").ShouldPass() + oc.PodsShouldBeRunning(commonVar.Project, `api-app-.[a-z0-9-]*`) + }) + + It("should find files in component container", func() { + helper.Cmd("odo", "exec", "--", "ls", "/project/server.js").ShouldPass() + }) + + It("should find bindings for service", func() { + helper.Cmd("odo", "exec", "--", "ls", "/bindings/etcd-link/clusterIP").ShouldPass() + }) + + It("should find owner references on link and service", func() { + ocArgs := []string{"get", "servicebinding", "etcd-link", "-o", "jsonpath='{.metadata.ownerReferences.*.name}'", "-n", commonVar.Project} + helper.WaitForCmdOut("oc", ocArgs, 1, true, func(output string) bool { + return strings.Contains(output, "api-app") + }) + + ocArgs = []string{"get", "etcdclusters.etcd.database.coreos.com", "myetcd", "-o", "jsonpath='{.metadata.ownerReferences.*.name}'", "-n", commonVar.Project} + helper.WaitForCmdOut("oc", ocArgs, 1, true, func(output string) bool { + return strings.Contains(output, "api-app") + }) + }) + }) + }) +}) diff --git a/tests/integration/operatorhub/cmd_service_test.go b/tests/integration/operatorhub/cmd_service_test.go index bc94bcf20d3..e0955104699 100644 --- a/tests/integration/operatorhub/cmd_service_test.go +++ b/tests/integration/operatorhub/cmd_service_test.go @@ -456,12 +456,13 @@ var _ = Describe("odo service command tests for OperatorHub", func() { BeforeEach(func() { linkName = "link-" + helper.RandString(6) helper.Cmd("odo", "link", "EtcdCluster/"+name, "--name", linkName).ShouldPass() - // for the moment, odo push is not necessary to deploy the link + helper.Cmd("odo", "push").ShouldPass() }) AfterEach(func() { // delete the link helper.Cmd("odo", "unlink", "EtcdCluster/"+name).ShouldPass() + helper.Cmd("odo", "push").ShouldPass() }) It("should create the link with the specified name", func() { @@ -479,12 +480,13 @@ var _ = Describe("odo service command tests for OperatorHub", func() { BeforeEach(func() { linkName = "link-" + helper.RandString(6) helper.Cmd("odo", "link", "EtcdCluster/"+name, "--name", linkName, "--bind-as-files").ShouldPass() - // for the moment, odo push is not necessary to deploy the link + helper.Cmd("odo", "push").ShouldPass() }) AfterEach(func() { // delete the link helper.Cmd("odo", "unlink", "EtcdCluster/"+name).ShouldPass() + helper.Cmd("odo", "push").ShouldPass() }) It("should create a servicebinding resource with bindAsFiles set to true", func() { @@ -607,6 +609,7 @@ spec: It("should link the two components successfully", func() { helper.Cmd("odo", "link", cmp1, "--context", context0).ShouldPass() + helper.Cmd("odo", "push", "--context", context0).ShouldPass() // check the link exists with the specific name ocArgs := []string{"get", "servicebinding", strings.Join([]string{cmp0, cmp1}, "-"), "-o", "jsonpath='{.status.secret}'", "-n", commonVar.Project} @@ -614,9 +617,9 @@ spec: return strings.Contains(output, strings.Join([]string{cmp0, cmp1}, "-")) }) - // delete the link + // delete the link and undeploy it helper.Cmd("odo", "unlink", cmp1, "--context", context0).ShouldPass() - + helper.Cmd("odo", "push", "--context", context0).ShouldPass() commonVar.CliRunner.WaitAndCheckForTerminatingState("servicebinding", commonVar.Project, 1) }) }) From 0f371daacc35cb03613a206829a6294cde50156e Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Wed, 23 Jun 2021 13:22:04 +0200 Subject: [PATCH 02/15] Changelog --- Changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/Changelog.md b/Changelog.md index 3e35d74e796..2b20b8882e1 100644 --- a/Changelog.md +++ b/Changelog.md @@ -9,6 +9,7 @@ - `odo push` undeploys Operator backed services marked as managed by the current devfile not present in this devfile anymore ([#4761](https://github.com/openshift/odo/pull/4761)) - param based `odo service create` for operator backed services ([#4704](https://github.com/openshift/odo/pull/4704)) - add `odo catalog describe service --example` ([#4821](https://github.com/openshift/odo/pull/4821)) +- `odo link` and `odo unlink` write to devfile without deploying to cluster. Deploying happens when running `odo push` ([#4819](https://github.com/openshift/odo/pull/4819)) ### Bug Fixes From d55421fabc40aaa05323befe3246709b62b3da4d Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Wed, 23 Jun 2021 17:15:22 +0200 Subject: [PATCH 03/15] Fix remove duplicates --- pkg/kclient/deployments.go | 19 ++++++++++++------- .../integration/operatorhub/cmd_link_test.go | 10 ++++++++++ 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/pkg/kclient/deployments.go b/pkg/kclient/deployments.go index 8486830b62b..1f493cf15e9 100644 --- a/pkg/kclient/deployments.go +++ b/pkg/kclient/deployments.go @@ -232,7 +232,7 @@ func (c *Client) ApplyDeployment(deploy appsv1.Deployment) (*appsv1.Deployment, klog.V(5).Infoln("Applying Deployment via server-side apply:") klog.V(5).Infoln(resourceAsJson(deploy)) - err = c.removeDuplicateEnv(deploy) + err = c.removeDuplicateEnv(deploy.Name) if err != nil { return nil, err } @@ -246,14 +246,15 @@ func (c *Client) ApplyDeployment(deploy appsv1.Deployment) (*appsv1.Deployment, // removeDuplicateEnv removes duplicate environment variables from containers, due to a bug in Service Binding Operator: // https://github.com/redhat-developer/service-binding-operator/issues/983 -func (c *Client) removeDuplicateEnv(deployment appsv1.Deployment) error { - _, err := c.KubeClient.AppsV1().Deployments(c.Namespace).Get(context.Background(), deployment.Name, metav1.GetOptions{}) +func (c *Client) removeDuplicateEnv(deploymentName string) error { + deployment, err := c.KubeClient.AppsV1().Deployments(c.Namespace).Get(context.Background(), deploymentName, metav1.GetOptions{}) if kerrors.IsNotFound(err) { return nil } if err != nil { return err } + changes := false containers := deployment.Spec.Template.Spec.Containers for i, container := range containers { duplicates := []corev1.EnvVar{} @@ -267,6 +268,7 @@ func (c *Client) removeDuplicateEnv(deployment appsv1.Deployment) error { } } if len(duplicates) > 0 { + changes = true newEnv := []corev1.EnvVar{} // first remove duplicates for _, env := range envs { @@ -286,11 +288,14 @@ func (c *Client) removeDuplicateEnv(deployment appsv1.Deployment) error { containers[i].Env = newEnv } } - _, err = c.KubeClient.AppsV1().Deployments(c.Namespace).Update(context.Background(), &deployment, metav1.UpdateOptions{}) - if kerrors.IsNotFound(err) { - return nil + if changes { + _, err = c.KubeClient.AppsV1().Deployments(c.Namespace).Update(context.Background(), deployment, metav1.UpdateOptions{}) + if kerrors.IsNotFound(err) { + return nil + } + return err } - return err + return nil } // DeleteDeployment deletes the deployments with the given selector diff --git a/tests/integration/operatorhub/cmd_link_test.go b/tests/integration/operatorhub/cmd_link_test.go index 7f1fa925540..936bde785bd 100644 --- a/tests/integration/operatorhub/cmd_link_test.go +++ b/tests/integration/operatorhub/cmd_link_test.go @@ -7,6 +7,7 @@ import ( "strings" . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" "github.com/openshift/odo/tests/helper" ) @@ -82,6 +83,11 @@ var _ = Describe("odo link command tests for OperatorHub", func() { It("should find files in component container", func() { helper.Cmd("odo", "exec", "--", "ls", "/project/server.js").ShouldPass() }) + + It("should find the link environment variable", func() { + stdOut := helper.Cmd("odo", "exec", "--", "sh", "-c", "echo $ETCDCLUSTER_CLUSTERIP").ShouldPass().Out() + Expect(stdOut).To(Not(BeEmpty())) + }) }) When("a link with between the component and the service is created with --bind-as-files and deployed", func() { @@ -95,6 +101,10 @@ var _ = Describe("odo link command tests for OperatorHub", func() { It("should find files in component container", func() { helper.Cmd("odo", "exec", "--", "ls", "/project/server.js").ShouldPass() }) + + It("should find bindings for service", func() { + helper.Cmd("odo", "exec", "--", "ls", "/bindings/etcd-link/clusterIP").ShouldPass() + }) }) }) From 0017a78d51f32fe89e11f57f025546c342a8d00f Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 24 Jun 2021 08:32:41 +0200 Subject: [PATCH 04/15] Do not set owner references again --- pkg/service/service.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pkg/service/service.go b/pkg/service/service.go index 06fe7621b75..fe67af61415 100644 --- a/pkg/service/service.go +++ b/pkg/service/service.go @@ -1048,6 +1048,16 @@ func UpdateKubernetesInlineComponentsOwnerReferences(client *kclient.Client, k8s return err } + found := false + for _, ownerRef := range u.GetOwnerReferences() { + if ownerRef.UID == ownerReference.UID { + found = true + break + } + } + if found { + continue + } u.SetOwnerReferences(append(u.GetOwnerReferences(), ownerReference)) err = client.UpdateDynamicResource(group, version, resource, crdName, u) From b5612036b84b011f4cc3e83e3422f9cfeeb1c641 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 25 Jun 2021 11:58:32 +0200 Subject: [PATCH 05/15] Fix wait for pod terminating --- .../adapters/kubernetes/component/adapter.go | 7 ++-- pkg/kclient/deployments.go | 34 +++++++++++++++++++ pkg/service/service.go | 2 +- 3 files changed, 39 insertions(+), 4 deletions(-) diff --git a/pkg/devfile/adapters/kubernetes/component/adapter.go b/pkg/devfile/adapters/kubernetes/component/adapter.go index ff59bafa0ed..f473b8dd090 100644 --- a/pkg/devfile/adapters/kubernetes/component/adapter.go +++ b/pkg/devfile/adapters/kubernetes/component/adapter.go @@ -177,11 +177,12 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) { } if componentExists && needRestart { - time.Sleep(time.Second) - a.deployment, err = a.Client.GetKubeClient().WaitForDeploymentRollout(a.deployment.Name) + s = log.Spinner("Waiting for component to be reconfigured") + err = a.Client.GetKubeClient().WaitForPodNotRunning(podName) if err != nil { - return errors.Wrap(err, "error while waiting for deployment rollout") + return err } + s.End(true) } log.Infof("\nCreating Kubernetes resources for component %s", a.ComponentName) diff --git a/pkg/kclient/deployments.go b/pkg/kclient/deployments.go index 1f493cf15e9..522c33c9f6b 100644 --- a/pkg/kclient/deployments.go +++ b/pkg/kclient/deployments.go @@ -116,6 +116,40 @@ func (c *Client) ListDeployments(selector string) (*appsv1.DeploymentList, error }) } +func (c *Client) WaitForPodNotRunning(name string) error { + watch, err := c.KubeClient.CoreV1().Pods(c.Namespace).Watch(context.TODO(), metav1.ListOptions{FieldSelector: "metadata.name=" + name}) + if err != nil { + return err + } + defer watch.Stop() + + if _, err = c.KubeClient.CoreV1().Pods(c.Namespace).Get(context.TODO(), name, metav1.GetOptions{}); kerrors.IsNotFound(err) { + fmt.Printf("\npod not found\n") + return nil + } + + for { + select { + case <-time.After(time.Minute): + return errors.New("timeout") + + case val, ok := <-watch.ResultChan(): + if !ok { + return errors.New("error getting value from resultchan") + } + if pod, ok := val.Object.(*corev1.Pod); ok { + for _, cond := range pod.Status.Conditions { + if cond.Type == "Ready" { + if cond.Status == corev1.ConditionFalse { + return nil + } + } + } + } + } + } +} + // WaitForDeploymentRollout waits for deployment to finish rollout. Returns the state of the deployment after rollout. func (c *Client) WaitForDeploymentRollout(deploymentName string) (*appsv1.Deployment, error) { klog.V(3).Infof("Waiting for %s deployment rollout", deploymentName) diff --git a/pkg/service/service.go b/pkg/service/service.go index fe67af61415..54543ec2c25 100644 --- a/pkg/service/service.go +++ b/pkg/service/service.go @@ -963,7 +963,7 @@ func PushServiceFromKubernetesInlineComponents(client *kclient.Client, k8sCompon } if _, found := deployed[cr+"/"+crdName]; !found && isLinkResource(cr) { - // If creating the ServiceBinding, first delete the component + // If creating the ServiceBinding, the component will restart needRestart = true } From b8ee82e16fa5f03e02edeee490d3fffc3109cb66 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 25 Jun 2021 12:18:58 +0200 Subject: [PATCH 06/15] Simplify env vars deduplication --- pkg/kclient/deployments.go | 35 ++++++++--------------------------- 1 file changed, 8 insertions(+), 27 deletions(-) diff --git a/pkg/kclient/deployments.go b/pkg/kclient/deployments.go index 522c33c9f6b..666ae18a80b 100644 --- a/pkg/kclient/deployments.go +++ b/pkg/kclient/deployments.go @@ -290,37 +290,18 @@ func (c *Client) removeDuplicateEnv(deploymentName string) error { } changes := false containers := deployment.Spec.Template.Spec.Containers - for i, container := range containers { - duplicates := []corev1.EnvVar{} + for i := range containers { found := map[string]bool{} - envs := container.Env - for _, env := range envs { - if _, ok := found[env.Name]; ok { - duplicates = append(duplicates, env) - } else { + var newEnv []corev1.EnvVar + for _, env := range containers[i].Env { + if _, ok := found[env.Name]; !ok { found[env.Name] = true + newEnv = append(newEnv, env) + } else { + changes = true } } - if len(duplicates) > 0 { - changes = true - newEnv := []corev1.EnvVar{} - // first remove duplicates - for _, env := range envs { - found := false - for _, duplicate := range duplicates { - if env.Name == duplicate.Name { - found = true - break - } - } - if !found { - newEnv = append(newEnv, env) - } - } - // next add them as single values - newEnv = append(newEnv, duplicates...) - containers[i].Env = newEnv - } + containers[i].Env = newEnv } if changes { _, err = c.KubeClient.AppsV1().Deployments(c.Namespace).Update(context.Background(), deployment, metav1.UpdateOptions{}) From 108a91c91aaecb9f8b6f9ec6566c792a58a89942 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 25 Jun 2021 13:45:23 +0200 Subject: [PATCH 07/15] Add comment --- pkg/service/service.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/service/service.go b/pkg/service/service.go index 54543ec2c25..e86c06dc244 100644 --- a/pkg/service/service.go +++ b/pkg/service/service.go @@ -1010,6 +1010,8 @@ func PushServiceFromKubernetesInlineComponents(client *kclient.Client, k8sCompon return needRestart, nil } +// UpdateKubernetesInlineComponentsOwnerReferences adds an owner reference to an inlined Kubernetes resource +// if not already present in the list of owner references func UpdateKubernetesInlineComponentsOwnerReferences(client *kclient.Client, k8sComponents []devfile.Component, ownerReference metav1.OwnerReference) error { for _, c := range k8sComponents { // get the string representation of the YAML definition of a CRD From 416e3ab023f9bb8ded2252e2df6a80dd739f0940 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 25 Jun 2021 13:45:34 +0200 Subject: [PATCH 08/15] Dont use oc --- tests/integration/operatorhub/cmd_link_test.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/tests/integration/operatorhub/cmd_link_test.go b/tests/integration/operatorhub/cmd_link_test.go index 936bde785bd..847c46cb76c 100644 --- a/tests/integration/operatorhub/cmd_link_test.go +++ b/tests/integration/operatorhub/cmd_link_test.go @@ -14,12 +14,10 @@ import ( var _ = Describe("odo link command tests for OperatorHub", func() { var commonVar helper.CommonVar - var oc helper.OcRunner BeforeEach(func() { commonVar = helper.CommonBeforeEach() helper.Chdir(commonVar.Context) - oc = helper.NewOcRunner("oc") }) AfterEach(func() { @@ -65,7 +63,8 @@ var _ = Describe("odo link command tests for OperatorHub", func() { helper.Cmd("odo", "service", "create", etcdCluster, serviceName, "--project", commonVar.Project).ShouldPass() helper.Cmd("odo", "push").ShouldPass() - oc.PodsShouldBeRunning(commonVar.Project, componentName+`-app-.[a-z0-9-]*`) + name := commonVar.CliRunner.GetRunningPodNameByComponent(componentName, commonVar.Project) + Expect(name).To(Not(BeEmpty())) }) It("should find files in component container", func() { @@ -77,7 +76,8 @@ var _ = Describe("odo link command tests for OperatorHub", func() { BeforeEach(func() { helper.Cmd("odo", "link", svcFullName).ShouldPass() helper.Cmd("odo", "push").ShouldPass() - oc.PodsShouldBeRunning(commonVar.Project, componentName+`-app-.[a-z0-9-]*`) + name := commonVar.CliRunner.GetRunningPodNameByComponent(componentName, commonVar.Project) + Expect(name).To(Not(BeEmpty())) }) It("should find files in component container", func() { @@ -95,7 +95,8 @@ var _ = Describe("odo link command tests for OperatorHub", func() { BeforeEach(func() { helper.Cmd("odo", "link", svcFullName, "--bind-as-files").ShouldPass() helper.Cmd("odo", "push").ShouldPass() - oc.PodsShouldBeRunning(commonVar.Project, componentName+`-app-.[a-z0-9-]*`) + name := commonVar.CliRunner.GetRunningPodNameByComponent(componentName, commonVar.Project) + Expect(name).To(Not(BeEmpty())) }) It("should find files in component container", func() { @@ -111,12 +112,14 @@ var _ = Describe("odo link command tests for OperatorHub", func() { When("getting sources, a devfile defining a component, a service and a link, and executing odo push", func() { BeforeEach(func() { + componentName := "api" // this is the name of the component in the devfile helper.CopyExample(filepath.Join("source", "nodejs"), commonVar.Context) helper.CopyExampleDevFile(filepath.Join("source", "devfiles", "nodejs", "devfile-with-link.yaml"), filepath.Join(commonVar.Context, "devfile.yaml")) - helper.Cmd("odo", "create", "api").ShouldPass() + helper.Cmd("odo", "create", componentName).ShouldPass() helper.Cmd("odo", "push").ShouldPass() - oc.PodsShouldBeRunning(commonVar.Project, `api-app-.[a-z0-9-]*`) + name := commonVar.CliRunner.GetRunningPodNameByComponent(componentName, commonVar.Project) + Expect(name).To(Not(BeEmpty())) }) It("should find files in component container", func() { From 78985171a2b0fc050a77b75d821542ba070b7520 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 25 Jun 2021 13:50:27 +0200 Subject: [PATCH 09/15] link type --- pkg/odo/cli/component/common_link.go | 36 +++++++++++----------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/pkg/odo/cli/component/common_link.go b/pkg/odo/cli/component/common_link.go index 217c6d04a87..6fbd31824ea 100644 --- a/pkg/odo/cli/component/common_link.go +++ b/pkg/odo/cli/component/common_link.go @@ -52,6 +52,14 @@ func newCommonLinkOptions() *commonLinkOptions { return &commonLinkOptions{} } +func (o *commonLinkOptions) getLinkType() string { + linkType := "component" + if o.isTargetAService { + linkType = "service" + } + return linkType +} + // Complete completes LinkOptions after they've been created func (o *commonLinkOptions) complete(name string, cmd *cobra.Command, args []string, context string) (err error) { o.csvSupport, _ = svc.IsCSVSupported() @@ -166,11 +174,6 @@ func (o *commonLinkOptions) run() (err error) { return o.linkOperator() } - linkType := "Component" - if o.isTargetAService { - linkType = "Service" - } - var component string if o.Context.EnvSpecificInfo != nil { component = o.EnvSpecificInfo.GetName() @@ -186,9 +189,9 @@ func (o *commonLinkOptions) run() (err error) { switch o.operationName { case "link": - log.Successf("%s %s has been successfully linked to the component %s\n", linkType, o.suppliedName, component) + log.Successf("The %s %s has been successfully linked to the component %s\n", o.getLinkType(), o.suppliedName, component) case "unlink": - log.Successf("%s %s has been successfully unlinked from the component %s\n", linkType, o.suppliedName, component) + log.Successf("The %s %s has been successfully unlinked from the component %s\n", o.getLinkType(), o.suppliedName, component) default: return fmt.Errorf("unknown operation %s", o.operationName) } @@ -339,11 +342,9 @@ func (o *commonLinkOptions) completeForOperator() (err error) { // validateForOperator validates the options when svc is supported func (o *commonLinkOptions) validateForOperator() (err error) { var svcFullName string - var linkType string if o.isTargetAService { // let's validate if the service exists - linkType = "service" svcFullName = strings.Join([]string{o.serviceType, o.serviceName}, "/") svcExists, err := svc.OperatorSvcExists(o.KClient, svcFullName) if err != nil { @@ -354,7 +355,6 @@ func (o *commonLinkOptions) validateForOperator() (err error) { } } else { o.serviceType = "Service" - linkType = "component" svcFullName = o.serviceName if o.suppliedName == o.EnvSpecificInfo.GetName() { return fmt.Errorf("the component %q cannot be linked with itself", o.suppliedName) @@ -375,7 +375,7 @@ func (o *commonLinkOptions) validateForOperator() (err error) { return err } if !found { - return fmt.Errorf("failed to unlink the %s %q since no link was found in the configuration referring this %s", linkType, svcFullName, linkType) + return fmt.Errorf("failed to unlink the %s %q since no link was found in the configuration referring this %s", o.getLinkType(), svcFullName, o.getLinkType()) } return nil } @@ -450,18 +450,14 @@ func (o *commonLinkOptions) linkOperator() (err error) { return err } if found { - return fmt.Errorf("component %q is already linked with the service %q", o.Context.EnvSpecificInfo.GetName(), o.suppliedName) + return fmt.Errorf("component %q is already linked with the %s %q", o.Context.EnvSpecificInfo.GetName(), o.getLinkType(), o.suppliedName) } err = svc.AddKubernetesComponentToDevfile(string(yamlDesc), o.serviceBinding.Name, o.EnvSpecificInfo.GetDevfileObj()) if err != nil { return err } - targetType := "component" - if o.isTargetAService { - targetType = "service" - } - log.Successf("Successfully created link between component %q and %s %q\n", o.Context.EnvSpecificInfo.GetName(), targetType, o.suppliedName) + log.Successf("Successfully created link between component %q and %s %q\n", o.Context.EnvSpecificInfo.GetName(), o.getLinkType(), o.suppliedName) log.Italic("To apply the link, please use `odo push`") return err } @@ -480,11 +476,7 @@ func (o *commonLinkOptions) unlinkOperator() (err error) { return err } - targetType := "component" - if o.isTargetAService { - targetType = "service" - } - log.Successf("Successfully unlinked component %q from %s %q\n", o.Context.EnvSpecificInfo.GetName(), targetType, o.suppliedName) + log.Successf("Successfully unlinked component %q from %s %q\n", o.Context.EnvSpecificInfo.GetName(), o.getLinkType(), o.suppliedName) log.Italic("To apply the changes, please use `odo push`") return nil } From ac6cfd63f5a675eabbc1eadfc345aecb29f5ae37 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Mon, 28 Jun 2021 10:20:49 +0200 Subject: [PATCH 10/15] fix --- tests/integration/operatorhub/cmd_link_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/integration/operatorhub/cmd_link_test.go b/tests/integration/operatorhub/cmd_link_test.go index 847c46cb76c..19e627a547e 100644 --- a/tests/integration/operatorhub/cmd_link_test.go +++ b/tests/integration/operatorhub/cmd_link_test.go @@ -92,8 +92,11 @@ var _ = Describe("odo link command tests for OperatorHub", func() { When("a link with between the component and the service is created with --bind-as-files and deployed", func() { + var bindingName string + BeforeEach(func() { - helper.Cmd("odo", "link", svcFullName, "--bind-as-files").ShouldPass() + bindingName = "sbr-" + helper.RandString(6) + helper.Cmd("odo", "link", svcFullName, "--bind-as-files", "--name", bindingName).ShouldPass() helper.Cmd("odo", "push").ShouldPass() name := commonVar.CliRunner.GetRunningPodNameByComponent(componentName, commonVar.Project) Expect(name).To(Not(BeEmpty())) @@ -104,7 +107,7 @@ var _ = Describe("odo link command tests for OperatorHub", func() { }) It("should find bindings for service", func() { - helper.Cmd("odo", "exec", "--", "ls", "/bindings/etcd-link/clusterIP").ShouldPass() + helper.Cmd("odo", "exec", "--", "ls", "/bindings/"+bindingName+"/clusterIP").ShouldPass() }) }) }) From 2bc451f470b78f45be2a7d9a46def633946cb5f5 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Mon, 28 Jun 2021 10:34:05 +0200 Subject: [PATCH 11/15] Fix spinner --- pkg/devfile/adapters/kubernetes/component/adapter.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/devfile/adapters/kubernetes/component/adapter.go b/pkg/devfile/adapters/kubernetes/component/adapter.go index f473b8dd090..da970cf9a68 100644 --- a/pkg/devfile/adapters/kubernetes/component/adapter.go +++ b/pkg/devfile/adapters/kubernetes/component/adapter.go @@ -161,6 +161,7 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) { s.End(true) log.Info("\nUpdating services") + s = log.Spinner("Updating services") // fetch the "kubernetes inlined components" to create them on cluster // from odo standpoint, these components contain yaml manifest of an odo service or an odo link k8sComponents, err := a.Devfile.Data.GetComponents(parsercommon.DevfileOptions{ @@ -177,13 +178,12 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) { } if componentExists && needRestart { - s = log.Spinner("Waiting for component to be reconfigured") err = a.Client.GetKubeClient().WaitForPodNotRunning(podName) if err != nil { return err } - s.End(true) } + s.End(true) log.Infof("\nCreating Kubernetes resources for component %s", a.ComponentName) From eed6739a3f05ecb9372a6caba35039c1292ac149 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Mon, 28 Jun 2021 14:34:14 +0200 Subject: [PATCH 12/15] Fix tests --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index a0d2c009ee4..87c448c3bcb 100644 --- a/Makefile +++ b/Makefile @@ -344,7 +344,7 @@ openshiftci-presubmit-unittests: .PHONY: test-operator-hub test-operator-hub: ## Run OperatorHub tests - $(RUN_GINKGO) $(GINKGO_FLAGS) -focus="odo service command tests" tests/integration/operatorhub/ + $(RUN_GINKGO) $(GINKGO_FLAGS) tests/integration/operatorhub/ .PHONY: test-cmd-devfile-describe test-cmd-devfile-describe: From 70b434565cfd10ea259bf2386bd10888d4e35571 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Tue, 29 Jun 2021 13:58:27 +0200 Subject: [PATCH 13/15] Fic error message for link/unlink --- pkg/odo/cli/component/common_link.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/odo/cli/component/common_link.go b/pkg/odo/cli/component/common_link.go index 6fbd31824ea..4a3c4633b2c 100644 --- a/pkg/odo/cli/component/common_link.go +++ b/pkg/odo/cli/component/common_link.go @@ -357,7 +357,11 @@ func (o *commonLinkOptions) validateForOperator() (err error) { o.serviceType = "Service" svcFullName = o.serviceName if o.suppliedName == o.EnvSpecificInfo.GetName() { - return fmt.Errorf("the component %q cannot be linked with itself", o.suppliedName) + if o.operationName == unlink { + return fmt.Errorf("the component %q cannot be unlinked from itself", o.suppliedName) + } else { + return fmt.Errorf("the component %q cannot be linked with itself", o.suppliedName) + } } _, err := o.Context.Client.GetKubeClient().GetService(o.suppliedName) From f8592f0298e2daa984a440d85a79fe9b94beac72 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Tue, 29 Jun 2021 15:20:30 +0200 Subject: [PATCH 14/15] Review --- pkg/devfile/adapters/kubernetes/component/adapter.go | 2 +- pkg/kclient/deployments.go | 6 +++--- pkg/service/service.go | 7 +++++++ 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/devfile/adapters/kubernetes/component/adapter.go b/pkg/devfile/adapters/kubernetes/component/adapter.go index da970cf9a68..289095fb7bc 100644 --- a/pkg/devfile/adapters/kubernetes/component/adapter.go +++ b/pkg/devfile/adapters/kubernetes/component/adapter.go @@ -178,7 +178,7 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) { } if componentExists && needRestart { - err = a.Client.GetKubeClient().WaitForPodNotRunning(podName) + err = a.Client.GetKubeClient().WaitForPodNotReady(podName) if err != nil { return err } diff --git a/pkg/kclient/deployments.go b/pkg/kclient/deployments.go index 666ae18a80b..90f6bf736ff 100644 --- a/pkg/kclient/deployments.go +++ b/pkg/kclient/deployments.go @@ -116,7 +116,8 @@ func (c *Client) ListDeployments(selector string) (*appsv1.DeploymentList, error }) } -func (c *Client) WaitForPodNotRunning(name string) error { +// WaitForPodNotReady waits for the status of the given pod to be not ready, or the pod to be deleted +func (c *Client) WaitForPodNotReady(name string) error { watch, err := c.KubeClient.CoreV1().Pods(c.Namespace).Watch(context.TODO(), metav1.ListOptions{FieldSelector: "metadata.name=" + name}) if err != nil { return err @@ -124,14 +125,13 @@ func (c *Client) WaitForPodNotRunning(name string) error { defer watch.Stop() if _, err = c.KubeClient.CoreV1().Pods(c.Namespace).Get(context.TODO(), name, metav1.GetOptions{}); kerrors.IsNotFound(err) { - fmt.Printf("\npod not found\n") return nil } for { select { case <-time.After(time.Minute): - return errors.New("timeout") + return fmt.Errorf("timeout while waiting for %q pod to stop", name) case val, ok := <-watch.ResultChan(): if !ok { diff --git a/pkg/service/service.go b/pkg/service/service.go index e86c06dc244..09f48bc2ab9 100644 --- a/pkg/service/service.go +++ b/pkg/service/service.go @@ -925,6 +925,7 @@ func PushServiceFromKubernetesInlineComponents(client *kclient.Client, k8sCompon } } needRestart := false + madeChange := false // create an object on the kubernetes cluster for all the Kubernetes Inlined components for _, c := range k8sComponents { @@ -987,6 +988,7 @@ func PushServiceFromKubernetesInlineComponents(client *kclient.Client, k8sCompon } else { log.Successf("Created service %q on the cluster; refer %q to know how to link it to the component", strings.Join([]string{kind, name}, "/"), "odo link -h") } + madeChange = true } for key, val := range deployed { @@ -1001,12 +1003,17 @@ func PushServiceFromKubernetesInlineComponents(client *kclient.Client, k8sCompon } else { log.Successf("Deleted service %q from the cluster", key) } + madeChange = true if val.DoesDeleteRestartsComponent { needRestart = true } } + if !madeChange { + log.Success("Services and Links are in sync with the cluster, no changes are required") + } + return needRestart, nil } From b3205028f7e7b48e8ccf1a03832918520e84006a Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Wed, 30 Jun 2021 13:14:00 +0200 Subject: [PATCH 15/15] No spinner --- pkg/devfile/adapters/kubernetes/component/adapter.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/devfile/adapters/kubernetes/component/adapter.go b/pkg/devfile/adapters/kubernetes/component/adapter.go index 289095fb7bc..8c87e38ffbe 100644 --- a/pkg/devfile/adapters/kubernetes/component/adapter.go +++ b/pkg/devfile/adapters/kubernetes/component/adapter.go @@ -161,7 +161,6 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) { s.End(true) log.Info("\nUpdating services") - s = log.Spinner("Updating services") // fetch the "kubernetes inlined components" to create them on cluster // from odo standpoint, these components contain yaml manifest of an odo service or an odo link k8sComponents, err := a.Devfile.Data.GetComponents(parsercommon.DevfileOptions{ @@ -183,7 +182,6 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) { return err } } - s.End(true) log.Infof("\nCreating Kubernetes resources for component %s", a.ComponentName)