Add support to perform kubernetes build in "odo deploy" using Kaniko build strategy #3751

jaideepr97 · 2020-08-12T18:21:10Z

What type of PR is this?

/kind feature

What does does this PR do / why we need it:

This PR extends the current implementation of odo deploy to make it kubernetes generic and allow performing unprivileged builds

Adds support for performing kaniko builds in odo deploy and pushing the built image to either an external registry or internal registry
Adds support for pushing images built using default BuildConfig path to external registries
Adds an optional dockerconfigjson flag to pass reference to a config.json file

NOTE: This PR builds on top of work contained in #3478 , and must only be merged after #3478 has been merged

Which issue(s) this PR fixes:

Fixes #3583

PR acceptance criteria:

Unit test

How to test changes / Special notes to the reviewer:

create a new directory, cd into it and run odo create nodejs --starter=nodejs-starter
run odo url create
replace created devfile with the following sample :

schemaVersion: 2.1.0
metadata:
  name: nodejs
  version: 1.0.0
  alpha.deployment-manifest: "https://raw.githubusercontent.com/groeges/devfile-registry/master/devfiles/nodejs/deploy_deployment.yaml"
projects:
  - name: nodejs-starter
    git:
      location: "https://github.com/odo-devfiles/nodejs-ex.git"
components:
  - container:
      name: runtime
      image: registry.access.redhat.com/ubi8/nodejs-12:1-36
      memoryLimit: 1024Mi
      mountSources: true
      endpoints:
        - name: http-3000
          targetPort: 3000
          configuration:
            protocol: tcp
            scheme: http
            type: terminal

  - dockerfile:
      name: dockerfile-build
      source: 
         sourceDir: "src"
         location: "https://github.com/ranakan19/golang-ex.git"
      dockerfileLocation: "https://raw.githubusercontent.com/jaideepr97/OpenShift-guides/master/Dockerfile"
      destination:
      Rootless: true 
    
commands:
  - exec:
      id: install
      component: runtime
      commandLine: npm install
      workingDir: /project
      group:
        kind: build
        isDefault: true
  - exec:
      id: run
      component: runtime
      commandLine: npm start
      workingDir: /project
      group:
        kind: run
        isDefault: true
  - exec:
      id: debug
      component: runtime
      commandLine: npm run debug
      workingDir: /project
      group:
        kind: debug
        isDefault: true
  - exec:
      id: test
      component: runtime
      commandLine: npm test
      workingDir: /project
      group:
        kind: test
        isDefault: true

run odo deploy --tag=<imageTag>

NOTE: setting Rootless: true will trigger a kaniko build. The default value is false and will trigger a build using buildConfig

NOTE: Please supply complete destination under --tag
( for ex: image-registry.openshift-image-registry.svc:5000/namespace/imagestream or docker.io/username/repository-name)

NOTE: (Relevant to external registry push only) if --dockerconfigjson is not specified, config.json is picked up from ~/.docker. Please ensure the default config.json file contains a valid auth token. External registry push is currently only configured for Dockerhub

…o parse_dockerfile

…he projects source

Signed-off-by: Steven Groeger <groeges@uk.ibm.com>

- Downloads Dockerfile into memory rather than onto the hosts machine - Validates if a Dockerfile is present in the project source, if one isn't specified by the devfile - Validates the contents of the Dockerfile (that there is a FROM on the first non-comment/non-whitespace line) - Validates the tag passed as an arg conforms to the correct character-set - Copies all project source files into the container rather than new/updated - Additional Testing - More appropriate error/warning messages

Co-authored-by: Cameron McWilliam <cam.mcwilliam@me.com>

* Add test List URL function Signed-off-by: Steven Groeger <groeges@uk.ibm.com> * Update logging within maifest deploy Signed-off-by: Steven Groeger <groeges@uk.ibm.com> * Add preliminary support for multiple manifest in single yaml Signed-off-by: Steven Groeger <groeges@uk.ibm.com> * Update service manifest with ClusterIP value before update. Signed-off-by: Steven Groeger <groeges@uk.ibm.com> * Merge deployDelete code and update to support multiple doc in yaml Signed-off-by: Steven Groeger <groeges@uk.ibm.com> * Fix source formatting Signed-off-by: Steven Groeger <groeges@uk.ibm.com> * Add some extra checks on waitManifestDeployComplete function Signed-off-by: Steven Groeger <groeges@uk.ibm.com>

alpha.deployment-manifest Signed-off-by: Steven Groeger <groeges@uk.ibm.com>

Signed-off-by: Steven Groeger <groeges@uk.ibm.com>

* Remove CommonPushOptions and do some clean up * Move validation, add better user experience, remove devfile.yaml flag

Signed-off-by: Steven Groeger <groeges@uk.ibm.com>

…o odo-deploy

Add unit tests to Kaniko build in "odo deploy"

openshift-ci-robot · 2020-08-12T18:21:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign dharmit
You can assign the PR to them by writing /assign @dharmit in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

openshift-ci-robot · 2020-08-12T18:21:34Z

Hi @jaideepr97. 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.

jaideepr97 · 2020-08-12T18:23:43Z

@wtam2018 @sbose78 @EnriqueL8 @neeraj-laad @groeges @kadel

cc @reginapizza @ranakan19 @Shraddhak22

amitkrout · 2020-08-13T02:12:33Z

/ok-to-test

openshift-ci-robot · 2020-08-13T03:11:08Z

@jaideepr97: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/unit	`a9afed1`	link	`/test unit`
ci/prow/v4.5-integration-e2e	`a9afed1`	link	`/test v4.5-integration-e2e`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

kadel · 2020-08-13T12:05:12Z

/status blocked
let's get #3478 merged first

Also, we need to have devfile at least first draft of 2.1.0 spec first

jaideepr97 · 2020-08-13T12:15:08Z

@kadel Makes sense
Are these prerequisites for the PR to be reviewed or merged?

kadel · 2020-08-13T12:25:46Z

@kadel Makes sense
Are these prerequisites for the PR to be reviewed or merged?

Just for merging.
It would be great if someone from @wtam2018 @reginapizza @ranakan19 @Shraddhak22 reviews it.

girishramnani · 2020-08-20T06:52:11Z

pkg/devfile/adapters/common/interface.go

 	DoesComponentExist(cmpName string) (bool, error)
+	Deploy(parameters DeployParameters) error


I would suggest call it CreateDeployment and DeleteDeployment

Thanks for the feedback @girishramnani
This may be better suited for #3478 as it was introduced there and just carried over into this PR

girishramnani · 2020-08-20T06:53:38Z

pkg/devfile/adapters/kubernetes/adapter.go

+}
+
+// Build creates Kubernetes resources to build an image for the component
+func (k Adapter) DeployDelete(manifest []byte) error {


instead of passing the whole manifest could we consider the minimum information that is necessary?

Thanks for the feedback @girishramnani
This may be better suited for #3478 as it was introduced there and just carried over into this PR

girishramnani · 2020-08-20T06:54:19Z

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

+		"component": a.ComponentName,
+	}
+
+	if err := a.createKanikoBuilderPod(labels, initContainer(initContainerName), builderContainer(containerName, parameters.Tag, isImageRegistryInternal), regcredName); err != nil {


nit: can we make this into multiple lines

girishramnani · 2020-08-20T06:55:20Z

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

+		return errors.Wrap(err, "error while creating kaniko builder pod")
+	}
+
+	podSelector := fmt.Sprintf("component=%s", a.ComponentName)


we have different kind of selectors present in our component/labels package and similarly in application/labels. please consider using those

girishramnani · 2020-08-20T06:55:55Z

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

+		return errors.Wrapf(err, "error while waiting for pod %s", podSelector)
+	}
+
+	defer func() {


lets extract this out into a function

girishramnani · 2020-08-20T06:59:21Z

pkg/kclient/pods.go

@@ -85,6 +85,63 @@ func (c *Client) WaitAndGetPod(watchOptions metav1.ListOptions, desiredPhase cor
 	}
 }

+// WaitAndGetPodOnInitContainerStarted blocks and waits until the named initiContainer is started.
+func (c *Client) WaitAndGetPodOnInitContainerStarted(watchOptions metav1.ListOptions, initContainerName, waitMessage string, hideSpinner bool) (*corev1.Pod, error) {


I think this function could be reduced by using the code from other functions in this package

girishramnani · 2020-08-20T07:05:05Z

pkg/occlient/occlient.go

+// Create a function to wait for deploment completion of any unstructured object
+func (c *Client) waitForManifestDeployCompletion(applicationName string, gvr schema.GroupVersionResource, conditionTypeValue string) (*unstructured.Unstructured, error) {
+	klog.V(4).Infof("Waiting for %s manifest deployment completion", applicationName)
+	w, err := c.dynamicClient.Resource(gvr).Namespace(c.Namespace).Watch(metav1.ListOptions{FieldSelector: "metadata.name=" + applicationName})


from what I understood - this function is only being used to check if knative route is created or not. Can we actually use knative's generated library here. the dynamic client's purpose was to be used at places where the resource is provided by the user and hence cannot be associated to strongly typed resource.

Thanks for the feedback @girishramnani
This may be better suited for #3478 as it was introduced there and just carried over into this PR

girishramnani · 2020-08-20T07:06:09Z

did a partial pass through of the code, will take a look again soon.

jaideepr97 · 2020-08-20T09:53:41Z

did a partial pass through of the code, will take a look again soon.

Thanks for the feedback @girishramnani
Will rebase and work on the changes requested for the code introduced in this PR for now

girishramnani · 2020-09-21T04:37:51Z

Closing this PR as it has diverged

EnriqueL8 and others added 30 commits June 4, 2020 14:39

WIP: Add simple poc build command

90191a4

POC Build

efab329

WIP: Add syncfiles poc for build

ddf14ab

Add ServiceAccount env

037012d

Clean up code

78f28ae

Add defer for delete

ea4a573

First pass downloading Dockerfile specified in devfile

11dec2b

Merge branch 'deploy_command' of https://github.com/EnriqueL8/odo int…

2bc2e4c

…o parse_dockerfile

Fix compilation issue

630ae4a

Moved Dockerfile download to .odo to avoid overwriting users one in t…

4102af3

…he projects source

Merge branch 'deploy_command' into parse_dockerfile

630b9dd

Updates for initial deploy steps

a81f13f

Signed-off-by: Steven Groeger <groeges@uk.ibm.com>

Tidy up following PR comments

603b722

Signed-off-by: Steven Groeger <groeges@uk.ibm.com>

Incorporate changes to determine manifest from devfile.yaml

8733e06

Signed-off-by: Steven Groeger <groeges@uk.ibm.com>

Updates for initial deploy steps

500000b

Fixing seg fault and defer error

0041f07

Add delete deploy (#7)

12658f2

Co-authored-by: Cameron McWilliam <cam.mcwilliam@me.com>

Build config (#8)

1e2c221

updated manifest file and added tests (#10)

1d1423b

Resolve PR comments and change to use alpha.build-dockerfile and (#11)

32beaf9

alpha.deployment-manifest Signed-off-by: Steven Groeger <groeges@uk.ibm.com>

Add some tests for manifest deploy part of odo deploy command (#13)

fe7dbaa

Signed-off-by: Steven Groeger <groeges@uk.ibm.com>

Remove --deployOnly option (#14)

4f53442

Signed-off-by: Steven Groeger <groeges@uk.ibm.com>

General clean up (#12)

1008511

* Remove CommonPushOptions and do some clean up * Move validation, add better user experience, remove devfile.yaml flag

Merge remote-tracking branch 'origin/master' into deploy_command

9d82133

Fix issues in merge conflict

3cc73db

Fix tests following restructure of command messages (#16)

8291abb

Signed-off-by: Steven Groeger <groeges@uk.ibm.com>

Fix issues in merge conflict

431717c

Fix dockerfile test

6790683

wtam2018 and others added 2 commits August 10, 2020 14:28

Merge branch 'deploy_command' of https://github.com/enriquel8/odo int…

69aad9c

…o odo-deploy

Add unit tests for kaniko build in odo deploy (#167)

a9afed1

Add unit tests to Kaniko build in "odo deploy"

openshift-ci-robot requested review from maysunfaisal and prietyc123 August 12, 2020 18:21

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. label Aug 12, 2020

jaideepr97 changed the title ~~Add support to perform kaniko build in "odo deploy"~~ Add support to perform kubernetes build in "odo deploy" using Kaniko build strategy Aug 12, 2020

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 Aug 13, 2020

girishramnani mentioned this pull request Aug 20, 2020

Deploy Command for OCP #3478

Closed

girishramnani added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Aug 20, 2020

girishramnani reviewed Aug 20, 2020

View reviewed changes

Shraddhak22 mentioned this pull request Aug 26, 2020

Add support to perform build in "odo deploy" using S2I strategy #3832

Closed

girishramnani closed this Sep 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to perform kubernetes build in "odo deploy" using Kaniko build strategy #3751

Add support to perform kubernetes build in "odo deploy" using Kaniko build strategy #3751

jaideepr97 commented Aug 12, 2020 •

edited

Loading

openshift-ci-robot commented Aug 12, 2020

openshift-ci-robot commented Aug 12, 2020

jaideepr97 commented Aug 12, 2020 •

edited

Loading

amitkrout commented Aug 13, 2020

openshift-ci-robot commented Aug 13, 2020

kadel commented Aug 13, 2020 •

edited

Loading

jaideepr97 commented Aug 13, 2020 •

edited

Loading

kadel commented Aug 13, 2020

girishramnani Aug 20, 2020

jaideepr97 Aug 20, 2020

girishramnani Aug 20, 2020

jaideepr97 Aug 20, 2020

girishramnani Aug 20, 2020

girishramnani Aug 20, 2020

girishramnani Aug 20, 2020

girishramnani Aug 20, 2020

girishramnani Aug 20, 2020

jaideepr97 Aug 20, 2020

girishramnani commented Aug 20, 2020

jaideepr97 commented Aug 20, 2020

girishramnani commented Sep 21, 2020

		DoesComponentExist(cmpName string) (bool, error)
		Deploy(parameters DeployParameters) error

Add support to perform kubernetes build in "odo deploy" using Kaniko build strategy #3751

Add support to perform kubernetes build in "odo deploy" using Kaniko build strategy #3751

Conversation

jaideepr97 commented Aug 12, 2020 • edited Loading

openshift-ci-robot commented Aug 12, 2020

openshift-ci-robot commented Aug 12, 2020

jaideepr97 commented Aug 12, 2020 • edited Loading

amitkrout commented Aug 13, 2020

openshift-ci-robot commented Aug 13, 2020

kadel commented Aug 13, 2020 • edited Loading

jaideepr97 commented Aug 13, 2020 • edited Loading

kadel commented Aug 13, 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

Choose a reason for hiding this comment

girishramnani commented Aug 20, 2020

jaideepr97 commented Aug 20, 2020

girishramnani commented Sep 21, 2020

jaideepr97 commented Aug 12, 2020 •

edited

Loading

jaideepr97 commented Aug 12, 2020 •

edited

Loading

kadel commented Aug 13, 2020 •

edited

Loading

jaideepr97 commented Aug 13, 2020 •

edited

Loading