Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

preStart events support #4909

Merged

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Jul 13, 2021

What type of PR is this?

/kind feature

What does this PR do / why we need it:

  • support of preStart events
  • Includes initContainers in component deployment
  • use of specific commit of devfile library

Which issue(s) this PR fixes:

Fixes #4901

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

  • Update changelog

  • I have read the test guidelines

How to test changes / Special notes to the reviewer:

  • Run odo create --s2i wildfly wildfly
  • Update the created devfile with the following content
  • Run odo push
  • The component should start correctly
commands:
- exec:
    commandLine: /opt/odo/bin/s2i-setup && /opt/odo/bin/assemble-and-restart
    component: s2i-builder
    group:
      isDefault: true
      kind: build
  id: s2i-assemble
- exec:
    commandLine: /opt/odo/bin/run
    component: s2i-builder
    group:
      isDefault: true
      kind: run
  id: s2i-run
- exec:
    commandLine: /opt/odo/bin/run
    component: s2i-builder
    group:
      isDefault: true
      kind: debug
  id: s2i-debug
- apply:
    component: copy-app-root-container
  id: copy-app-root
events:
  preStart:
  - copy-app-root
components:
- container:
    command:
    - /bin/sh
    - -c
    - "cp -R /opt/app-root /mnt/"
    image: docker.io/openshift/wildfly-100-centos7:latest
    volumeMounts:
    - name: app-root
      path: /mnt/app-root
  name: copy-app-root-container
- container:
    endpoints:
    - name: http-8778
      targetPort: 8778
    - name: http-8080
      targetPort: 8080
    env:
    - name: DEBUG_PORT
      value: "5858"
    - name: ODO_S2I_SCRIPTS_URL
      value: /usr/libexec/s2i
    - name: ODO_S2I_SCRIPTS_PROTOCOL
      value: image://
    - name: ODO_S2I_SRC_BIN_PATH
      value: /tmp
    - name: ODO_S2I_DEPLOYMENT_DIR
      value: /deployments
    - name: ODO_S2I_WORKING_DIR
      value: /home/jboss
    - name: ODO_S2I_BUILDER_IMG
      value: centos7/s2i-base-centos7
    - name: ODO_SRC_BACKUP_DIR
      value: /opt/app-root/src-backup
    - name: ODO_S2I_CONVERTED_DEVFILE
      value: "true"
    image: docker.io/openshift/wildfly-100-centos7:latest
    mountSources: true
    sourceMapping: /tmp/projects
    volumeMounts:
    - name: app-root
      path: /opt/app-root
  name: s2i-builder
- name: app-root
  volume:
    size: 1Gi
metadata:
  name: wildfly
  version: 1.0.0
schemaVersion: 2.0.0

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Jul 13, 2021
@feloy feloy changed the title preStart events support [WIP] preStart events support Jul 13, 2021
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jul 13, 2021
@feloy feloy force-pushed the bugfix-4901/prestart branch from cb4efdd to 1f0ae4b Compare July 13, 2021 14:11
@feloy feloy changed the title [WIP] preStart events support preStart events support Jul 13, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jul 13, 2021
@feloy
Copy link
Contributor Author

feloy commented Jul 13, 2021

/test v4.7-integration-e2e

@feloy feloy force-pushed the bugfix-4901/prestart branch from 1f0ae4b to 1ceb755 Compare July 15, 2021 11:30
@mohammedzee1000
Copy link
Contributor

Verified as working

> mkdir sample
cp -avrf ../odo/tests/examples/source/wildfly/* ./sample
> cd sample
> odo create --s2i wildfly wildfly
Validation
Warning: wildfly is not fully supported by odo, and it is not guaranteed to work
 ✓  Validating component [756ms]
Conversion
 ✓  Successfully generated devfile.yaml and env.yaml for provided S2I component

Please use `odo push` command to create the component with source deployed
> cat devfile.yaml
commands:
- exec:
    commandLine: /opt/odo/bin/s2i-setup && /opt/odo/bin/assemble-and-restart
    component: s2i-builder
    group:
      isDefault: true
      kind: build
  id: s2i-assemble
- exec:
    commandLine: /opt/odo/bin/run
    component: s2i-builder
    group:
      isDefault: true
      kind: run
  id: s2i-run
- exec:
    commandLine: /opt/odo/bin/run
    component: s2i-builder
    group:
      isDefault: true
      kind: debug
  id: s2i-debug
components:
- container:
    endpoints:
    - name: http-8080
      targetPort: 8080
    - name: http-8778
      targetPort: 8778
    env:
    - name: DEBUG_PORT
      value: "5858"
    - name: ODO_S2I_SCRIPTS_URL
      value: /usr/local/s2i
    - name: ODO_S2I_SCRIPTS_PROTOCOL
      value: image://
    - name: ODO_S2I_SRC_BIN_PATH
      value: /tmp
    - name: ODO_S2I_DEPLOYMENT_DIR
      value: /deployments
    - name: ODO_S2I_WORKING_DIR
      value: /home/jboss
    - name: ODO_S2I_BUILDER_IMG
      value: centos7/s2i-base-centos7
    - name: ODO_SRC_BACKUP_DIR
      value: /opt/app-root/src-backup
    - name: ODO_S2I_CONVERTED_DEVFILE
      value: "true"
    image: docker.io/openshift/wildfly-100-centos7:latest
    mountSources: true
    sourceMapping: /tmp/projects
  name: s2i-builder
metadata:
  name: wildfly
  version: 1.0.0
schemaVersion: 2.0.0
> ls
devfile.yaml  pom.xml  src
> vi devfile.yaml
> cat devfile.yaml
commands:
- exec:
    commandLine: /opt/odo/bin/s2i-setup && /opt/odo/bin/assemble-and-restart
    component: s2i-builder
    group:
      isDefault: true
      kind: build
  id: s2i-assemble
- exec:
    commandLine: /opt/odo/bin/run
    component: s2i-builder
    group:
      isDefault: true
      kind: run
  id: s2i-run
- exec:
    commandLine: /opt/odo/bin/run
    component: s2i-builder
    group:
      isDefault: true
      kind: debug
  id: s2i-debug
- apply:
    component: copy-app-root-container
  id: copy-app-root
events:
  preStart:
  - copy-app-root
components:
- container:
    command:
    - /bin/sh
    - -c
    - "cp -R /opt/app-root /mnt/"
    image: docker.io/openshift/wildfly-100-centos7:latest
    volumeMounts:
    - name: app-root
      path: /mnt/app-root
  name: copy-app-root-container
- container:
    endpoints:
    - name: http-8778
      targetPort: 8778
    - name: http-8080
      targetPort: 8080
    env:
    - name: DEBUG_PORT
      value: "5858"
    - name: ODO_S2I_SCRIPTS_URL
      value: /usr/libexec/s2i
    - name: ODO_S2I_SCRIPTS_PROTOCOL
      value: image://
    - name: ODO_S2I_SRC_BIN_PATH
      value: /tmp
    - name: ODO_S2I_DEPLOYMENT_DIR
      value: /deployments
    - name: ODO_S2I_WORKING_DIR
      value: /home/jboss
    - name: ODO_S2I_BUILDER_IMG
      value: centos7/s2i-base-centos7
    - name: ODO_SRC_BACKUP_DIR
      value: /opt/app-root/src-backup
    - name: ODO_S2I_CONVERTED_DEVFILE
      value: "true"
    image: docker.io/openshift/wildfly-100-centos7:latest
    mountSources: true
    sourceMapping: /tmp/projects
    volumeMounts:
    - name: app-root
      path: /opt/app-root
  name: s2i-builder
- name: app-root
  volume:
    size: 1Gi
metadata:
  name: wildfly
  version: 1.0.0
schemaVersion: 2.0.0
> oc new-project myproject
Now using project "myproject" on server "https://api.ci-ln-s3lnirb-f76d1.origin-ci-int-gce.dev.openshift.com:6443".

You can add applications to this project with the 'new-app' command. For example, try:

    oc new-app centos/ruby-25-centos7~https://github.com/sclorg/ruby-ex.git

to build a new example application in Ruby.
> odo push

Validation
 ✓  Validating the devfile [241359ns]

Updating services
 ✓  Services and Links are in sync with the cluster, no changes are required

Creating Kubernetes resources for component wildfly
 ✓  Added storage app-root to wildfly
 ✓  Waiting for component to start [54s]
 ✓  Waiting for component to start [291ms]

Applying URL changes
 ✓  URL http-8778: http://http-8778-app-myproject.apps.ci-ln-s3lnirb-f76d1.origin-ci-int-gce.dev.openshift.com/ created
 ✓  URL http-8080: http://http-8080-app-myproject.apps.ci-ln-s3lnirb-f76d1.origin-ci-int-gce.dev.openshift.com/ created

Syncing to component wildfly
 ✓  Checking files for pushing [2ms]
 ✓  Syncing files to the component [6s]

Executing devfile commands for component wildfly
 ✓  Waiting for component to start [325ms]
 ✓  Executing s2i-assemble command "/opt/odo/bin/s2i-setup && /opt/odo/bin/assemble-and-restart" [2s]
 ✓  Executing s2i-run command "/opt/odo/bin/run" [3s]

Pushing devfile component "wildfly"
 ✓  Changes successfully pushed to component
> oc get deployments
NAME          READY     UP-TO-DATE   AVAILABLE   AGE
wildfly-app   1/1       1            1           104s
> oc describe deployment wildfly-app
...
  Init Containers:
   copy-app-root-container-copy-app-root-1:
    Image:      docker.io/openshift/wildfly-100-centos7:latest
    Port:       <none>
    Host Port:  <none>
    Command:
      /bin/sh
      -c
      cp -R /opt/app-root /mnt/
    Environment:
      PROJECTS_ROOT:   /projects
      PROJECT_SOURCE:  /projects
    Mounts:
      /mnt/app-root from app-root-wildfly-app-vol (rw)
   copy-supervisord:
    Image:      registry.access.redhat.com/ocp-tools-4/odo-init-container-rhel8:1.1.10
    Port:       <none>
    Host Port:  <none>
    Command:
      /usr/bin/cp
    Args:
      -r
      /opt/odo-init/.
      /opt/odo/
    Environment:  <none>
    Mounts:
      /opt/odo/ from odo-supervisord-shared-data (rw)
...

Copy link
Contributor

@mohammedzee1000 mohammedzee1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me
/lgtm

if err != nil {
return err
}

var initContainers []corev1.Container
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fact that we have to do this speaks of a need for improvement on the library side. We should be able to fetch init containers and the remaining containers seperately.
Nice one though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the devfile team has made a change on the library for this recently: devfile/library#108

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 19, 2021
@feloy feloy force-pushed the bugfix-4901/prestart branch from a296af5 to 0531711 Compare July 19, 2021 13:12
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 19, 2021
@feloy
Copy link
Contributor Author

feloy commented Jul 19, 2021

/test v4.7-integration-e2e

@mohammedzee1000
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 19, 2021
Comment on lines 5 to 7
func validatePreStart(preStart []string) (err error) {
// This is odo specific validation. There is still discussion about how PreStart should be implemented.
// https://github.com/devfile/api/issues/204
// https://github.com/openshift/odo/issues/4187
// This is here to prevent anyone from using PreStart event until we have a proper implementation
if len(preStart) != 0 {
return &UnsupportedFieldError{fieldName: "preStart"}
}
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this now, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right, I removed the associated code

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 21, 2021
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@feloy
Copy link
Contributor Author

feloy commented Jul 21, 2021

/test psi-kubernetes-integration-e2e

@valaparthvi
Copy link
Contributor

@feloy Do we need integration tests to test preStart event support?

@feloy
Copy link
Contributor Author

feloy commented Jul 21, 2021

@feloy Do we need integration tests to test preStart event support?

I'm not sure, as it is not a normal use case for users. The support will be tested through the S2I conversion using preStart events with the PR #4918

@feloy
Copy link
Contributor Author

feloy commented Jul 21, 2021

/test psi-kubernetes-integration-e2e

@dharmit
Copy link
Member

dharmit commented Jul 22, 2021

@feloy Do we need integration tests to test preStart event support?

It's not user facing at the moment. It's not possible to use any odo commands to set/unset preStart events in a devfile. So, IMO, it is OK to not have integration tests here.

@feloy
Copy link
Contributor Author

feloy commented Jul 22, 2021

/test psi-kubernetes-integration-e2e

@kadel
Copy link
Member

kadel commented Jul 22, 2021

/approve

@openshift-ci
Copy link

openshift-ci bot commented Jul 22, 2021

[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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jul 22, 2021
@valaparthvi
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 22, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jul 22, 2021

@feloy: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
ci/prow/psi-kubernetes-integration-e2e ad1fb9e link /test psi-kubernetes-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.

@feloy
Copy link
Contributor Author

feloy commented Jul 22, 2021

/retest-required

@openshift-merge-robot openshift-merge-robot merged commit e1391b2 into redhat-developer:main Jul 22, 2021
anandrkskd pushed a commit to anandrkskd/odo that referenced this pull request Jul 27, 2021
* preStart events support

* Changelog

* devfile/library with new getContainers

* Fix unit test

* Remove unnecessary validateEvents
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

preStart lifecyle event support
6 participants