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

Extend deployByDefault to other components, drop the implicit apply command rule #852

Closed
l0rd opened this issue May 22, 2022 · 12 comments
Closed
Labels
area/api Enhancement or issue related to the api/devfile specification area/devworkspace Improvent or additions to the DevWorkspaces CRD area/library Common devfile library for interacting with devfiles lifecycle/rotten Rotten items. These items have been stale for 60 days and are now closed. lifecycle/stale Stale items. These items have not been updated for 90 days. student Identified work that the students can work on
Milestone

Comments

@l0rd
Copy link
Contributor

l0rd commented May 22, 2022

Which area this feature is related to?

/area api
/area library
/area devworkspace

Which functionality do you think we should add?

Why is this needed? Is your feature request related to a problem?

Devfile adopters struggle to understand what components will be started when a workspace is provisioned.

Detailed description:

Understanding which components are started automatically (or builded in the case of an image) and which are not is currently complicated.

There is an implicit rule to start only components that are NOT referenced by a command of type apply. That's a hidden rule that is hard to understand and remember.

The new deployByDefault and autoBuild fields have improved the situation but only apply to kubernetes/openshift and image components respectively.

Describe the solution you'd like

Add a runOnDemand boolean field for components of type containers (false by default).

Do not rely on the apply rule anymore, but forbid the following configurations (document in the spec, raise an error in the library):

  • a container with runOnDemand: false is referenced by an apply a command
  • a kubernetes/openshift with deployByDefault: true is referenced by an apply command
  • a image with autobuild: true is referenced by an apply command

Describe alternatives you've considered

We should add a new command of type delete that would be the complement of apply.

Related issues

#204
#693

@openshift-ci openshift-ci bot added area/api Enhancement or issue related to the api/devfile specification area/library Common devfile library for interacting with devfiles area/devworkspace Improvent or additions to the DevWorkspaces CRD labels May 22, 2022
@rajibmitra
Copy link

I would love to work on this issue. @l0rd

@viveksahu26
Copy link

Hey @l0rd , Ohh, I am bit late to know about this project. Hey, Myself vivek !! Currently a student, looking for new project to contribute. And it seems to me an interesting project. I have bit of experience of past one year in open source world. Contributed to Kyverno project. Looking forward to know more about this project. Any useful resources ??

@faze-geek
Copy link

Hi @l0rd , this is Anurag Bhat , a student looking to contribute to this project under the LFX mentorship program . I feel this project is a great entry point for me into cloud native development, after contributing to couple of GSoC orgs in the past year. I had submitted my application 3-4 days ago and believe the admission process is in progress now !
Thanks .

@kadel
Copy link
Member

kadel commented May 31, 2022

I'm not a big fan of the "apply rule" so it would be great to have something more explicit.

I'm a bit afraid of creating too many rules like "If this is enabled then you can't enable something else" it might make writing devfiles too complex as it createss too many invalid scenarios. I would avoid adding validation like this unless it creates situation that are ambiguous or completely invalid.

  • a container with runOnDemand: false is referenced by an apply a command

Doing this might look weird, but essentially it is not creating an invalid scenario. You will be applying something that already exists, so it will only result in "noop".

  • a kubernetes/openshift with deployByDefault: true is referenced by an apply command

Similar in as above scenario

  • a image with autobuild: true is referenced by an apply command

I can actually see a valid scenario where I would want to build image with autobuild: true again. I would set autobuild: true to have it build for the first time on startup, and later I might want to apply that image component again to re-build image, when I did some changes.

name runOnDemand looks a bit weird to me, It might not be obvious what it does.

Could we maybe name this field autoStart and have the logic reversed.
By default it would be false so it would be applied only if it is referenced by command.
autoStart: true would mean that is is automatically applied on workspace start, but it could be still referenced by commands.

I will try to create a few examples to verify how it would work.

@l0rd
Copy link
Contributor Author

l0rd commented Jun 10, 2022

@kadel setting autoStart: false by default would add some syntax sugar AND change the semantic of existing Devfile that do not have an autoStart field. Although this is an interesting topic (are devfile containers started more often at startup or after running a command?) I would like to keep this issue about a new more explicit syntax without hanging the behavior of existing Devfiles.

I am ok to call it autoStart and it's a good idea to allow a component to haveautoStart: true and to be referenced by an apply command for now. We can decide later, based on feedback, if we need to avoid it or not.

@l0rd
Copy link
Contributor Author

l0rd commented Jun 10, 2022

@rajibmitra is going to work on this issue as part of the LXF mentorship program.

@kadel
Copy link
Member

kadel commented Jun 10, 2022

@kadel setting autoStart: false by default would add some syntax sugar AND change the semantic of existing Devfile that do not have an autoStart field. Although this is an interesting topic (are devfile containers started more often at startup or after running a command?) I would like to keep this issue about a new more explicit syntax without hanging the behavior of existing Devfiles.

I got a little bit carried away 😇 . You are right this might not be a good idea right now as it would be incompatible with current Devfile behaviors, but it would simplify a lot of things :-)

I am ok to call it autoStart and it's a good idea to allow a component to haveautoStart: true and to be referenced by an apply command for now. We can decide later, based on feedback, if we need to avoid it or not.

I'm afraid that if we want to keep default behaviors as autoStart: true we will need a different name for this field :-/
Some time back we agreed that we should try to keep default values for boolean fields false, as it makes it much easier to deal with them.
This means that we won't be able to use autoStart. We will need a different name that will have inverted logic (like the originally proposed runOnDemand).
runOnDemand sounds a little bit weird to me, but currently, I can't think of a better name for this.

@l0rd
Copy link
Contributor Author

l0rd commented Jul 1, 2022

During the Devfile Cabal of the 20th of June we have discussed a couple of proposal as mentioned in another issue to use the field deployStrategy instead of runOnDemand:

Proposal 1:
DeployStrategy: AtStartupInMainPod, AtStartupInSeparatePod, StartWithCommandInMainPod, StartWithCommandInSeparatePod

Proposal 2:
DeployStrategy: AtStartup, StartWithCommand
DedicatedPod: true/false

@kadel Action: Come up with a proposal and examples on how these fields will behave.

@l0rd l0rd added the student Identified work that the students can work on label Jul 6, 2022
@elsony elsony added this to the 2.2 milestone Jul 12, 2022
@kadel
Copy link
Member

kadel commented Jul 15, 2022

# Controlling component creation

Devfile has 4 types of components:

  • container
  • image
  • kubernetes
  • openshift
  • volume

Current situation

By default, there is an "apply" rule: If a component is not referenced by an apply command, the component will be automatically created on startup.

On top of that there is also deployByDefault field for kubernetes and openshift components and autoBuild for image component.

If deployByDefault or autoBuild is true, then the component will be created on startup regardless if it is referenced by the apply command or not.

kubernetes component

deployByDefault: true deployByDefault: false deployByDefault not set
referenced in apply command automatically created on startup created only when the command is invoked created only when the command is invoked
not referenced in apply command automatically created on startup never started ??? automatically created on startup

image component

autoBuild: true autoBuild: false autoBuild not set
referenced in apply command automatically created on startup created only when the command is invoked created only when the command is invoked
not referenced in apply command automatically created on startup never started ??? automatically created on startup

Originally Proposed change

Introduce new field deployStrategy with options atStartup, onDemand

deployStrategy: atStartup deployStrategy: onDemand deployStrategy not set
referenced in apply command automatically created on startup created only when the command is invoked created only when the command is invoked
not referenced in apply command automatically created on startup never created automatically created on startup

For backward compatibility can't remove deployByDefault and autoBuild fields.
They should be marked as deprecated in the docs. But we will have to keep supporting them.

Setting both deployByDefault/autoBuild and deployStrategy should be considered invalid. The validator or tools should error out if Devfile uses the old and new fields at the same time in the same component.

container components

For the container component, the exec command should create the onDemand container the same way apply does.

Problems

There is one big problem with container component and onDemand behavior.

It is not possible to add containers to the existing Pod without restarting Pod.

Let's say that exec creates onDemand controller the same way apply would.

example
components:
  - name: runtime
    container:
      image: registry.access.redhat.com/ubi8/nodejs-16:latest
      endpoints:
        - name: http-3000
          targetPort: 3000

  - name: test
    container:
      image: registry.access.redhat.com/ubi8/nodejs-16:test
      deployStrategy: onDemand
      endpoints:
        - name: http-3000
          targetPort: 3000

commands:
  - id: install
    exec:
      component: runtime
      commandLine: npm install
      workingDir: ${PROJECT_SOURCE}
      group:
        kind: build
        isDefault: true
  - id: run
    exec:
      component: runtime
      commandLine: npm start
      workingDir: ${PROJECT_SOURCE}
      group:
        kind: run
        isDefault: true
  - id: test
    exec:
      component: test
      commandLine: npm run test
      workingDir: ${PROJECT_SOURCE}
      group:
        kind: test
        isDefault: true

deployStrategy could be problematic for container as onDemand will properly work only if the main Pod gets restarted.
The solution could be that we define that onDemand automatically means dedicatedPod: true, or we make onDemand only possible if dedicatedPod: true, but at that point, we are creating a lot of rules that are not directly expressed in the file.

kubernetes/openshift components

Create kubernetes/openshift means simply "apply" the inlined or uri referenced definition.
There is no problem in doing it at the startup or on demand later on.

image components

Create image/ means build the container image and push it to the registry.
There is no problem in doing it at the startup or on demand later on.
Devfile author just needs to make sure that if container or kubernetes component uses that image the build is executed first.

volume components

Creating volume on demand might be problematic. In most cases, volume will be represented as PersistentVolumeClaim resource on the cluster.
Creating PVC without using it in the container doesn't make much sense.
There is no benefit in controlling when the volume will be created. Better would be to simply define that the volume will be created when the container that uses it first is created.

deployStrategy doesn't make sense for volume

Proposal v2

Each component, where makes sense, will get its own field to control "deployStrategy".

Mario's runOnDemand makes sense for container component.

container components

option 1: runOnDemand: true/false

  • When runOnDemand: true, the container is always created in a separate pod.
  • By default, containers are started in one pod at startup.
  • exec command should behave like apply command for container component (container "onDemand" will be just before exec command is executed
runOnDemand: true runOnDemand: false runOnDemand not set
referenced in apply command created when apply command is invoked automatically created on startup created when apply command is invoked
not referenced in apply command never started ??? automatically created on startup automatically created on startup

dedicatedPod:true & runOnDemand:true : OK
dedicatedPod:true & runOnDemand:false : OK
dedicatedPod:true & : OK

dedicatedPod:false & runOnDemand:true : NOTVALID
dedicatedPod:false & runOnDemand:false : OK
dedicatedPod:false & : OK

kubernetes/openshift components

deployByDefault: true/false

deployByDefault: true deployByDefault: false deployByDefault not set
referenced in apply command automatically created on startup created when apply command is invoked created when apply command is invoked
not referenced in apply command automatically created on startup never started ??? automatically created on startup

image components

autoBuild: true/false

autoBuild: true autoBuild: false autoBuild not set
referenced in apply command automatically created on startup created when apply command is invoked created when apply command is invoked
not referenced in apply command automatically created on startup never started ??? automatically created on startup

volume components

no field to control the creation; it gets created automatically when it is used in the container definition.

@kadel
Copy link
Member

kadel commented Aug 11, 2022

Just copying the final proposal to the comment to make it easier to link to.

To see how I got to this point check previous comment #852 (comment)

Proposal v2

Each component, where makes sense, will get its own field to control "deployStrategy".

Mario's runOnDemand makes sense for container component.

  • container: runOnDemand
  • kuberetes/openshift: deployByDefualt
  • image: autoBuild
  • volume: nothing

container components

option 1: runOnDemand: true/false

  • When runOnDemand: true, the container is always created in a separate pod.
  • By default, containers are started in one pod at startup.
  • exec command should behave like apply command for container component (container "onDemand" will be just before exec command is executed
runOnDemand: true runOnDemand: false runOnDemand not set
referenced in apply command created when apply command is invoked automatically created on startup created when apply command is invoked
not referenced in apply command never started ??? automatically created on startup automatically created on startup

dedicatedPod:true & runOnDemand:true : OK
dedicatedPod:true & runOnDemand:false : OK
dedicatedPod:true & : OK

dedicatedPod:false & runOnDemand:true : NOTVALID
dedicatedPod:false & runOnDemand:false : OK
dedicatedPod:false & : OK

kubernetes/openshift components

deployByDefault: true/false

deployByDefault: true deployByDefault: false deployByDefault not set
referenced in apply command automatically created on startup created when apply command is invoked created when apply command is invoked
not referenced in apply command automatically created on startup never started ??? automatically created on startup

image components

autoBuild: true/false

autoBuild: true autoBuild: false autoBuild not set
referenced in apply command automatically created on startup created when apply command is invoked created when apply command is invoked
not referenced in apply command automatically created on startup never started ??? automatically created on startup

volume components

no field to control the creation; it gets created automatically when it is used in the container definition.

@elsony
Copy link
Contributor

elsony commented Sep 15, 2022

Community call discussion:
We will move this out of 2.2.

@elsony elsony modified the milestones: 2.2, 2.3 Sep 15, 2022
rm3l added a commit to rm3l/odo that referenced this issue Mar 10, 2023
rm3l added a commit to rm3l/odo that referenced this issue Mar 13, 2023
rm3l added a commit to rm3l/odo that referenced this issue Mar 21, 2023
rm3l added a commit to rm3l/odo that referenced this issue Mar 29, 2023
rm3l added a commit to rm3l/odo that referenced this issue Mar 31, 2023
rm3l added a commit to rm3l/odo that referenced this issue Apr 4, 2023
openshift-merge-robot pushed a commit to redhat-developer/odo that referenced this issue Apr 5, 2023
…nShift components (#6654)

* Add sample Devfile with multiple autoBuild/deployByDefault combinations

It will be used for integration tests.

* Add helper function to update a given Devfile Command Group

This will allow supporting cases where we need to run a custom command,
but this is not implemented yet on odo (cases with 'odo dev --debug'
and 'odo deploy').
In this case, this helper will allow updating the Devfile for example to
unmark the current default command as non-default, and mark the custom
one as default.

* Add test cases for 'odo dev'

* Add test cases for 'odo deploy'

* Add test cases for 'odo build-images'

'odo build-images' should build all images regardless of the 'autoBuild' property.

* Display the spinner when creating or updating Kubernetes resources

This helps understand what resources are being patched.

* Handle deployByDefault on K8s and OpenShift components

* Add 'devfile.GetImageComponentsToPush' functions that returns the list of image components to auto-create

See devfile/api#852 (comment) for more context.

* Handle automatic image component creation for 'odo dev' on Kubernetes

* Handle automatic image component creation for 'odo dev' on Podman

* Handle automatic image and K8s/OpenShift component creation for 'odo deploy'

* Bump Devfile library to the latest commit at this time (c1b23d2)

This includes the fix for [1], which provides a way not to set default values automatically

[1] devfile/api#1067

* Do not set default values when parsing a Devfile

See [1] for the rationale

[1] https://github.com/redhat-developer/odo/issues/5694\#issuecomment-1465778398

* Add documentation in the Devfile reference page

* Revert "Display the spinner when creating or updating Kubernetes resources"

This reverts commit 6ad073e63cb0e685f165eed767619a90997393a3.

* Avoid re-applying Image components multiple times

'adapter#Push' might get called several times when there are
state transitions (either on the Deployment in the cluster,
or from 'odo').
It might be confusing to apply Image components over and over again
(and also it can be slower if we need to push images to remote registries).

* Move GetK8sAndOcComponentsToPush and GetImageComponentsToPush to libdevfile package

As suggested in review, this should be the responsibility of the devfile library

Co-authored-by: Philippe Martin <phmartin@redhat.com>

* fixup! Handle automatic image and K8s/OpenShift component creation for 'odo deploy'

* fixup! Handle automatic image component creation for 'odo dev' on Podman

* fixup! Avoid re-applying Image components multiple times

* Apply suggestions from code review

Co-authored-by: Parthvi Vala <pvala@redhat.com>

* fixup! Do not set default values when parsing a Devfile

* Fix devfile-deploy-functional-pods.yaml (used in 'odo logs' tests)

Set deployByDefault to false on innerloop-pod component.
Otherwise, since it is not referenced by any apply command,
it will be automatically created by *both* 'odo dev' and 'odo deploy'.

---------

Co-authored-by: Philippe Martin <phmartin@redhat.com>
Co-authored-by: Parthvi Vala <pvala@redhat.com>
Copy link

github-actions bot commented Aug 1, 2024

This issue is stale because it has been open for 90 days with no activity. Remove stale label or comment or this will be closed in 60 days.

@github-actions github-actions bot added the lifecycle/stale Stale items. These items have not been updated for 90 days. label Aug 1, 2024
@github-actions github-actions bot added the lifecycle/rotten Rotten items. These items have been stale for 60 days and are now closed. label Oct 1, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Enhancement or issue related to the api/devfile specification area/devworkspace Improvent or additions to the DevWorkspaces CRD area/library Common devfile library for interacting with devfiles lifecycle/rotten Rotten items. These items have been stale for 60 days and are now closed. lifecycle/stale Stale items. These items have not been updated for 90 days. student Identified work that the students can work on
Projects
None yet
Development

No branches or pull requests

6 participants