create services together with deployment during devfile push #2679

yangcao77 · 2020-03-04T20:23:53Z

What type of PR is this?
/kind feature

What does does this PR do / why we need it:
This PR creates service during devfile push using the port name and number provided within the devfile. Also updates the service if the deployment get updated.

$ kubectl describe service springtest1
Name:              springtest1
Namespace:         che
Labels:            component=springtest1
Annotations:       <none>
Selector:          component=springtest1
Type:              ClusterIP
IP:                172.30.100.64
Port:              9090-tcp  9090/TCP
TargetPort:        9090/TCP
Endpoints:         10.129.0.231:9090
Port:              8080-tcp  8080/TCP
TargetPort:        8080/TCP
Endpoints:         10.129.0.231:8080
Session Affinity:  None
Events:            <none>

Which issue(s) this PR fixes:
#2622

How to test changes / Special notes to the reviewer:

To test create
git clone Clone https://github.com/rajivnathan/openLibertyDevfile

odo preference set experimental true
 ./odo push --devfile ./devfile.yaml

Verify a service with the component name has been created.
Describe the service, verify the port matches the endpoint in the devfile.

To test update:
modify the port number to 8080 in devfile

 ./odo push --devfile ./devfile.yaml

Describe the service, verify the port has been changed to 8080

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

openshift-ci-robot · 2020-03-04T20:24:08Z

Hi @yangcao77. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2020-03-04T20:35:52Z

Codecov Report

Merging #2679 into master will increase coverage by 0.35%.
The diff coverage is 69.84%.

@@            Coverage Diff             @@
##           master    #2679      +/-   ##
==========================================
+ Coverage   43.39%   43.74%   +0.35%     
==========================================
  Files          86       87       +1     
  Lines        7907     7967      +60     
==========================================
+ Hits         3431     3485      +54     
- Misses       4136     4143       +7     
+ Partials      340      339       -1

Impacted Files	Coverage Δ
pkg/kclient/services.go	`100% <100%> (ø)`
pkg/kclient/generators.go	`92.63% <100%> (+1.38%)`	⬆️
...g/devfile/adapters/kubernetes/component/adapter.go	`34.19% <38.7%> (+0.25%)`	⬆️
pkg/odo/cli/service/create.go	`20% <0%> (ø)`	⬆️
pkg/devfile/parser/apiVersion.go	`76.47% <0%> (ø)`	⬆️
pkg/devfile/parser/content.go	`83.33% <0%> (+0.72%)`	⬆️
pkg/component/watch.go	`74% <0%> (+6.66%)`	⬆️
pkg/devfile/parser/fs.go	`100% <0%> (+100%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57756ed...e80367d. Read the comment docs.

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

mik-dass · 2020-03-05T08:37:56Z

/ok-to-test

amitkrout

@yangcao77 Can you please add integration test for the changes. The test will be mostly user facing scenario like what you describe in How to test changes of your pr description.

How to write:

Create a separate test file, for example devfile_test.go in tests/integration/

devfile_test.go

package integration

import (
	"os"
	"path/filepath"
	"time"

	. "github.com/onsi/ginkgo"
	. "github.com/onsi/gomega"
	"github.com/openshift/odo/tests/helper"
)

var _ = Describe("odo storage command tests", func() {
	var project string
	var context string

	// This is run after every Spec (It)
	var _ = BeforeEach(func() {
		SetDefaultEventuallyTimeout(10 * time.Minute)
		SetDefaultConsistentlyDuration(30 * time.Second)
		context = helper.CreateNewContext()
		os.Setenv("GLOBALODOCONFIG", filepath.Join(context, "config.yaml"))
		project = helper.CreateRandProject()
		oc = helper.NewOcRunner("oc")
	})

	// Clean up after the test
	// This is run after every Spec (It)
	var _ = AfterEach(func() {
		helper.DeleteProject(project)
		helper.DeleteDir(context)
		os.Unsetenv("GLOBALODOCONFIG")
	})

	Context("when running devfile push", func() {
		It("should create services together with deployment", func() {
			helper.CmdShouldPass("git", "clone", "https://github.com/rajivnathan/openLibertyDevfile", filepath.Join(context,"openLibertyDevfile"))
			helper.CmdShouldPass("odo", "preference", "set", "experimental", "true")
			helper.CmdShouldPass("odo", "push", "--devfile", filepath.Join(context,"openLibertyDevfile","devfile.yaml"))
			
			// modify the port number to 8080 in devfile. You need to write a 
			// new helper function in tests/integration/helper/helper_filesystem.go
			// and call it here for example helper.yamlupdate()

			helper.CmdShouldPass("odo", "push", "--devfile", filepath.Join(context,"openLibertyDevfile","devfile.yaml"))

			// verify Describe the service using oc command, verify the port has been changed to 8080

		})
	})
})

Basically we use ginkgo framework for integration test. You can visit ginkgo documentation for more details - https://onsi.github.io/ginkgo/ plus go through odo integration tests here in tests/integration/ for how we have been doing it. I am sure you will get more tips for writing the integration test.

yangcao77 · 2020-03-05T15:43:23Z

@amitkrout We do not have any integration tests for devfile support yet. The integration tests will be added later on. We have an issue opened to track that: #2673
(Currently the integration tests issue is blocked by this issue: #2684 - no namespace saved in env.yaml file)

amitkrout · 2020-03-06T08:04:28Z

@amitkrout We do not have any integration tests for devfile support yet. The integration tests will be added later on. We have an issue opened to track that: #2673
(Currently the integration tests issue is blocked by this issue: #2684 - no namespace saved in env.yaml file)

Understood. Thanks

adisky · 2020-03-09T08:15:29Z

pkg/kclient/services.go

+
+	service, err := c.KubeClient.CoreV1().Services(c.Namespace).Create(&svc)
+	if err != nil {
+		return nil, errors.Wrapf(err, "unable to create Service for %s", commonObjectMeta.Name)


is wrapping error is necessary here?? IMO we should return bare errors from client functions, as they are intended to be called my multiple functions and it helps in checking conditions based on error codes.

I'm following the error format for all the other functions under pkg/kclient (pod.go, volumes.go, deployments.go).

ya I see, resolving it for your PR :), will raise a separate concern.

amitkrout · 2020-03-09T21:36:18Z

/test v4.1-integration-e2e-benchmark

yangcao77 · 2020-03-10T17:37:08Z

This PR currently blocks the work for #2592 which is targeted for sprint 180.
@amitkrout Can you help review this PR? Thanks.

kadel · 2020-03-11T17:07:25Z

/refresh

kadel

if two containers expose the same port it fails

▶ odo push
 •  Push devfile component spring-boot-http-booster  ...
 ✗  Failed to start component with name spring-boot-http-booster.
Error: Failed to start the component: unable to update Service for spring-boot-http-booster: Service "spring-boot-http-booster" is invalid: [spec.ports[1].name: Duplicate value: "8080-tcp", spec.ports[1]: Duplicate value: core.ServicePort{Name:"", Protocol:"TCP", Port:8080, TargetPort:intstr.IntOrString{Type:0, IntVal:0, StrVal:""}, NodePort:0}]

devfile.yaml

apiVersion: 1.0.0
metadata:
  name: test
components:
  - type: dockerimage
    alias: tools
    image: quay.io/eclipse/che-java8-maven:nightly
    memoryLimit: 768Mi
    endpoints:
      - name: "8080/tcp"
        port: 8080
    mountSources: true
    volumes:
      - name: m2
        containerPath: /home/user/.m2
  - type: dockerimage
    alias: tools2
    image: quay.io/eclipse/che-java8-maven:nightly
    memoryLimit: 768Mi
    endpoints:
      - name: "8080/tcp"
        port: 8080
    mountSources: true
    volumes:
      - name: m2
        containerPath: /home/user/.m2
commands:
  - name: build
    actions:
      - type: exec
        component: tools
        command: "mvn clean install"
  - name: run 
    actions:
      - type: exec
        component: tools
        command: java-jar target/*.jar

kadel · 2020-03-11T17:19:46Z

if two containers expose the same port it fails

▶ odo push
 •  Push devfile component spring-boot-http-booster  ...
 ✗  Failed to start component with name spring-boot-http-booster.
Error: Failed to start the component: unable to update Service for spring-boot-http-booster: Service "spring-boot-http-booster" is invalid: [spec.ports[1].name: Duplicate value: "8080-tcp", spec.ports[1]: Duplicate value: core.ServicePort{Name:"", Protocol:"TCP", Port:8080, TargetPort:intstr.IntOrString{Type:0, IntVal:0, StrVal:""}, NodePort:0}]

devfile.yaml

apiVersion: 1.0.0
metadata:
  name: test
components:
  - type: dockerimage
    alias: tools
    image: quay.io/eclipse/che-java8-maven:nightly
    memoryLimit: 768Mi
    endpoints:
      - name: "8080/tcp"
        port: 8080
    mountSources: true
    volumes:
      - name: m2
        containerPath: /home/user/.m2
  - type: dockerimage
    alias: tools2
    image: quay.io/eclipse/che-java8-maven:nightly
    memoryLimit: 768Mi
    endpoints:
      - name: "8080/tcp"
        port: 8080
    mountSources: true
    volumes:
      - name: m2
        containerPath: /home/user/.m2
commands:
  - name: build
    actions:
      - type: exec
        component: tools
        command: "mvn clean install"
  - name: run 
    actions:
      - type: exec
        component: tools
        command: java-jar target/*.jar

I take this back 😇
The devfile that I used actually doesn't make sense, as all the dockerimage components are containers in one pod, and they can't expose same port.

/appprove

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

maysunfaisal · 2020-03-12T15:15:55Z

pkg/devfile/adapters/kubernetes/utils/utils.go

-		resourceReqs := GetResourceReqs(comp)
-		container := kclient.GenerateContainer(*comp.Alias, *comp.Image, false, comp.Command, comp.Args, envVars, resourceReqs)
-		containers = append(containers, *container)
+	// Only components with aliases are considered because without an alias commands cannot reference them


Think you over wrote the master changes during rebase, this logic has been moved to common.utils.go GetSupportedComponents()

can you remove the comment as well, that comment is for func GetAliasedComponents()

maysunfaisal · 2020-03-12T15:22:19Z

pkg/devfile/adapters/kubernetes/component/adapter.go

 	} else {
 		_, err = a.Client.CreateDeployment(*deploymentSpec)
 		if err != nil {
 			return err
 		}
 		glog.V(3).Infof("Successfully created component %v", componentName)
+		_, err = a.Client.CreateService(objectMeta, *serviceSpec)


Question about something you brought up before, are we creating only one service for all the components?

One service for all ports in devfile.

maysunfaisal · 2020-03-12T15:23:56Z

pkg/devfile/adapters/kubernetes/utils/utils.go

+	for _, endpoint := range endpoints {
+		name := strings.TrimSpace(util.GetDNS1123Name(strings.ToLower(*endpoint.Name)))
+		containerPorts = append(containerPorts, corev1.ContainerPort{
+			Name:          name,


does this need to be trimmed to below 63 char? I had a similar issue for "vol name", I later found out not only Kubernetes resources but other names are also capped at 63. Can you pls confirm this?

Thanks for pointing this out. Tested that the max length for port name is 15 characters. Push a new commit to trim it down to 15 characters.

maysunfaisal · 2020-03-12T15:25:26Z

pkg/devfile/adapters/kubernetes/component/adapter.go

+	for _, c := range deploymentSpec.Template.Spec.Containers {
+		if len(containerPorts) == 0 {
+			containerPorts = c.Ports
+		} else {
+			containerPorts = append(containerPorts, c.Ports...)
+		}
+	}


what happens if multiple components have the same endpoint, are we appending the same thing twice here? What happens when that is passed to GenerateServiceSpec

There cannot be multiple components have the same endpoint, see Tomas's comment:

all the dockerimage components are containers in one pod, and they can't expose same port.

oh ok thx, We should see if push's devfile parse validation is doing it for the endpoints or else add it in 🤔

Tomas has already tested it:

▶ odo push • Push devfile component spring-boot-http-booster ... ✗ Failed to start component with name spring-boot-http-booster. Error: Failed to start the component: unable to update Service for spring-boot-http-booster: Service "spring-boot-http-booster" is invalid: [spec.ports[1].name: Duplicate value: "8080-tcp", spec.ports[1]: Duplicate value: core.ServicePort{Name:"", Protocol:"TCP", Port:8080, TargetPort:intstr.IntOrString{Type:0, IntVal:0, StrVal:""}, NodePort:0}]

kadel · 2020-03-12T16:05:43Z

/approve

openshift-ci-robot · 2020-03-12T16:06:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [kadel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

maysunfaisal · 2020-03-12T21:06:04Z

pkg/devfile/adapters/kubernetes/utils/utils.go

+	for _, endpoint := range endpoints {
+		name := strings.TrimSpace(util.GetDNS1123Name(strings.ToLower(*endpoint.Name)))
+		if len(name) > 15 {
+			name = name[:15]


can you use the util function util.TruncateString instead here and have a const for 15

maysunfaisal · 2020-03-12T21:28:17Z

pkg/devfile/adapters/kubernetes/utils/utils.go

-		resourceReqs := GetResourceReqs(comp)
-		container := kclient.GenerateContainer(*comp.Alias, *comp.Image, false, comp.Command, comp.Args, envVars, resourceReqs)
-		containers = append(containers, *container)
+	// Only components with aliases are considered because without an alias commands cannot reference them


can you remove the comment as well, that comment is for func GetAliasedComponents()

maysunfaisal · 2020-03-12T21:34:11Z

pkg/kclient/generators.go

+	// generate Service Spec
+	var svcPorts []corev1.ServicePort
+	for _, containerPort := range containerPorts {
+		svcPort := corev1.ServicePort{


if we're reconverting from corev1.ContainerPort to corev1.ServicePort, can we just use corev1.ServicePort in utils.go ConvertPorts()? Then your GenerateServiceSpec() can just take corev1.ServicePort directly and does not need to convert

The pod spec need it to be in format of corev1.ContainerPort when we generate it use the containers returned from utils.GetContainers()

maysunfaisal · 2020-03-12T21:41:16Z

pkg/devfile/adapters/kubernetes/component/adapter.go

 		if err != nil {
 			return err
 		}
 		glog.V(3).Infof("Successfully created component %v", componentName)
-		_, err = a.Client.CreateService(objectMeta, *serviceSpec)
+		ownerReference := kclient.GenerateOwnerReference(deployment)
+		objectMetaTemp := objectMeta


I understand why you have an objectMetaTemp for update, but i think you dont really need it for create..

maysunfaisal · 2020-03-12T22:06:28Z

pkg/kclient/services.go

+		Spec:       svcSpec,
+	}
+
+	service, err := c.KubeClient.CoreV1().Services(c.Namespace).Create(&svc)


So I tried odo push with multiple components having the same endpoints, and i noticed that there is no devfile validation..

Services.Create() fails because it cannot create service with two same endpoints, but this results in a deployment and a pod that is left hanging because it could not create pvc which is after service creation.

NAME READY STATUS RESTARTS AGE LABELS pod/spring-devfile-85d64fdd5b-dxn4j 0/2 Pending 0 9s component=spring-devfile,pod-template-hash=4182098816 NAME DESIRED CURRENT UP-TO-DATE AVAILABLE AGE LABELS deployment.extensions/spring-devfile 1 1 1 0 9s component=spring-devfile

Maybe we should validate the data in devfile before we do odo push but I think we dont have it now. Could be done later with an another PR perhaps, if we want to do it...

Added validation for duplicate ports

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

yangcao77 · 2020-03-16T21:44:47Z

/test v4.3-integration-e2e-benchmark
/test v4.2-integration-e2e-benchmark
/test v4.1-integration-e2e-benchmark

yangcao77 · 2020-03-17T17:36:36Z

/test v4.3-integration-e2e-benchmark
/test v4.2-integration-e2e-benchmark
/test v4.1-integration-e2e-benchmark

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

kadel · 2020-03-18T08:21:13Z

/test v4.3-integration-e2e-benchmark
/test v4.2-integration-e2e-benchmark
/test v4.1-integration-e2e-benchmark

@yangcao77 you can save some typing using /retest
This command automatically restarts all failed Prow tests ;-)

/retest

yangcao77 · 2020-03-18T15:25:23Z

/test v4.2-integration-e2e-benchmark

yangcao77 · 2020-03-18T17:35:11Z

/test v4.2-integration-e2e-benchmark

yangcao77 · 2020-03-18T20:46:22Z

/test v4.2-integration-e2e-benchmark

kadel · 2020-03-19T11:28:31Z

/retest

maysunfaisal · 2020-03-19T22:43:12Z

Thx for the change, works well 👍

/lgtm

openshift-bot · 2020-03-20T01:17:47Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-03-20T03:42:53Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-03-20T03:55:48Z

/retest

Please review the full test history for this PR and help us cut down flakes.

yangcao77 added 4 commits March 2, 2020 11:31

able to create service using first container port

283e1e0

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

move serviceSpec creation to generator.go

cd5a21c

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

finish create and update service, add generator tests

b3254dc

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

add service test

72aca68

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

openshift-ci-robot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Mar 4, 2020

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. size/L labels Mar 4, 2020

openshift-ci-robot requested review from dharmit and mohammedzee1000 March 4, 2020 20:24

sanitize the port name

f28981c

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. labels Mar 5, 2020

amitkrout suggested changes Mar 5, 2020

View reviewed changes

openshift-ci-robot assigned amitkrout Mar 5, 2020

adisky reviewed Mar 9, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Mar 11, 2020

kadel requested changes Mar 11, 2020

View reviewed changes

openshift-ci-robot assigned kadel Mar 11, 2020

resolve conflict with master

df54fb9

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Mar 11, 2020

maysunfaisal suggested changes Mar 12, 2020

View reviewed changes

openshift-ci-robot assigned maysunfaisal Mar 12, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Mar 12, 2020

yangcao77 added 2 commits March 12, 2020 14:37

trim down the port name to 15 characters

048a517

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

add ownerRef for service created

bfdb5da

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

maysunfaisal suggested changes Mar 12, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Mar 16, 2020

yangcao77 added 2 commits March 16, 2020 14:26

add validation for duplicate port number

8f7242c

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

merge from master

d3affe5

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Mar 16, 2020

yangcao77 added 2 commits March 17, 2020 14:17

add a prefix to the random dir created ty bypass service name issue

3da087a

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

create prefix-componentname as directory name for devfile test

e80367d

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Mar 19, 2020

openshift-merge-robot merged commit f940c55 into redhat-developer:master Mar 20, 2020

rm3l added estimated-size/L (20-40) Rough sizing for Epics. About 2 sprints of work for a person. and removed size/L labels Jun 18, 2023

create services together with deployment during devfile push #2679

create services together with deployment during devfile push #2679

Conversation

yangcao77 commented Mar 4, 2020 • edited

openshift-ci-robot commented Mar 4, 2020

codecov bot commented Mar 4, 2020 • edited

Codecov Report

mik-dass commented Mar 5, 2020

amitkrout left a comment

Choose a reason for hiding this comment

yangcao77 commented Mar 5, 2020 • edited

amitkrout commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitkrout commented Mar 9, 2020

yangcao77 commented Mar 10, 2020

kadel commented Mar 11, 2020

kadel left a comment

Choose a reason for hiding this comment

kadel commented Mar 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Mar 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel commented Mar 12, 2020

openshift-ci-robot commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangcao77 Mar 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Mar 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangcao77 commented Mar 16, 2020

yangcao77 commented Mar 17, 2020

kadel commented Mar 18, 2020 • edited

yangcao77 commented Mar 18, 2020

yangcao77 commented Mar 18, 2020

yangcao77 commented Mar 18, 2020

kadel commented Mar 19, 2020

maysunfaisal commented Mar 19, 2020

openshift-bot commented Mar 20, 2020

openshift-bot commented Mar 20, 2020

openshift-bot commented Mar 20, 2020

yangcao77 commented Mar 4, 2020 •

edited

codecov bot commented Mar 4, 2020 •

edited

yangcao77 commented Mar 5, 2020 •

edited

maysunfaisal Mar 12, 2020 •

edited

yangcao77 Mar 12, 2020 •

edited

maysunfaisal Mar 12, 2020 •

edited

kadel commented Mar 18, 2020 •

edited