configuration option to avoid unncessary restarts of process

[maxandersen]

Not sure how this fits in with devfile 2.0 spec but it is/was a problem of odo behavior causing a full restart of the running server when files changed - that is not wanted for things like springboot and quarkus.

see odo issue at redhat-developer/odo#3043

[sbose78]

Good point, looks like redhat-developer/odo#3043 should be resolving this?

[maxandersen]

Yes, 3043 is I understand it odo's approach to support it but not part of the devfile spec.

[l0rd]

It's not clear to me how this would translate in the devfile spec. @kadel any idea?

[kadel]

It's not clear to me how this would translate in the devfile spec. @kadel any idea?

I'm not sure. It could be an additional option for exec component. But it might look weird as it make sense only for some type of commands.

This is why we are currently using attributes for this.

Would something like this make sense for Che? As far as I know, Che doesn't do any automatic application restarts. The Che user is in control when and if the application process should be restarted.

[sbose78]

Yes, options which must not be added as a strongly typed attribute, should go into the loosely typed list of attributes in attributes?

[pdaverh]

I imagine options that do not apply to a tool can be ignored by the tool. For example, if this is added as a devfile spec (RESTART=No), then Che can ignore it but odo can respect it.

[kadel]

Here is an example of how to use the "restart" attribute in devfile v2. (works only in odo)
https://github.com/odo-devfiles/registry/blob/master/devfiles/quarkus/devfile.yaml#L35-L36

[kadel]

Proposal

The commands that get executed can be of two types:

• one-shot command This is a command that does its job and exits. The typical case is build command. Tool executing this command should wait for this command to finish and report its status.
• long-running command This is command is running and doing their job without exiting (like servers). The typical case is run command. The tool should not wait for it to finish. The tool needs to ensure that the command is kept running even when it is not attached to the process.

In addition, the long-running process can be also of two types:

• hot-reload capable
• needs to be restarted to pick up changes

This is especially important for debug and run command groups. Some tools like for example odo need to know if the command needs to be restart to pick up changes or not.

This means that we need two additional fields for exec commands.

• background: (true|false)
• restartOnChange: (true|false)

Each command group will have different defaults based on its typical use:

restartOnChange- tells tool to restart the process when there is a change that needs to be applied
restartOnChange: false - hotreload capable process
restartOnChange: true - tool (odo/Che) needs to restart the process when the source changes

• build
  • background: false
  • restartOnChange: true
• run
  • background: true
  • restartOnChange: false
• debug
  • background: true
  • restartOnChange: false
• test
  • background: false
  • restartOnChange: true

Example

commands:
    - exec:
        id: install-deps
        component: dev 
        commandLine: "npm package"
        group:
          kind: build
          isDefault: true
    - exec:
        id: run
        component: dev
        commandLine: "npm run start"
        restartOnChange: true
        group:
          kind: run
          isDefault: true
    - exec:
        id: run-dev
        component: dev
        commandLine: "npm run debug"
        restartOnChange: true
        group:
          kind: run

[amisevsk]

@kadel I've left a review on the PR #96 (review) but also wanted to ask a question here: how does a command that is

background: false
restartOnChange: true

behave? Presumably we would check that the command hasn't exited yet, and restart it if changes are detected, but I'm not sure where this functionality is useful. Some cases for a test task:

• I'm writing tests for one Java class while testing another, independent class. This setting would continually restart my tests, running identical code
• I'm running a test command and want to make a change, but the "change detected" event races the completion of the test. I would need to be paying attention to the progress of my test to know whether my current changes have been tested, and the user flow is opposite depending on whether tests have completed (have to re-run command if it's completed, but don't if it hasn't)

Personally, I think restartOnChange doesn't make sense for non background tasks, since there is an intentionality to starting a completing task, that I think is the default mode for using such functionality.

If there's some use-case I'm not thinking of, I would like to put forth keeping restartOnChange as default false for background: false commands, as I believe this is the kind of flow typically expected from other tooling.

[adisky]

@kadel not sure about che use cases here, but currently for odo we determine background value from kind of command.
so adding the field background is really required?

If only use case is hot reload, which indirectly does background build+reload, could this be introduced as a separate kind?

[kadel]

@kadel not sure about che use cases here, but currently for odo we determine background value from kind of command.
so adding the field background is really required?

That is a good point.
odo currently treats all run and debug commands as they would have background: true.
Would this make sense for che? @davidfestal @amisevsk

[elsony]

@kadel not sure about che use cases here, but currently for odo we determine background value from kind of command.
so adding the field background is really required?

That is a good point.
odo currently treats all run and debug commands as they would have background: true.
Would this make sense for che? @davidfestal @amisevsk

For build command, we always want to have it as background false since we want to block odo push until the build is completed and return after the build is completed. For run and debug, it is bit more tricky. When we are dealing with a single run and debug command, just go by the type will work and run as background true. However, it is not that straight forward for composite command. If someone has a composite run command that contain multiple commands, we'd probably want to execute all of them as background false and the last one as background true. I am not sure if we want to deal with multiple background commands at the same time. We may still need the background field to tell has which is the last one that we need to keep it running in the background but it also means that we can have one and only one background command within the composite run.

[amisevsk]

I believe the current implementation in Che is fairly simple, so we're not directly impacted by this proposal at the moment (we just run processes and report return codes). I agree with @elsony that this becomes complicated with more complicated composite commands, but don't know the Che-side plans well enough to say one way or another what makes sense for us. I defer to @davidfestal on that.

I'm a little uncomfortable in general with defaults being tied to the value of another field, as that makes functionality more subtle and forces us to impose categories on what a given type of task looks like for all languages/platforms, but if it hits 90% of cases well and is documented well, I'm fine either way.

[adisky]

@elsony if there are composite commands, During execution we could easily determine the last one.

I checked about composite commands, as per the comment here #18 (comment) they could contain mixed commands and they are executed in order.

They also have there own group, so the composite group would override individual command group? if yes then how we will execute it, depending upon the individual command background field or the overridden group defaults?

May be i am missing something here but little confused, when seeing background, command group and composite command group altogether.

[kadel]

However, it is not that straight forward for composite command. If someone has a composite run command that contain multiple commands, we'd probably want to execute all of them as background false and the last one as background true. I am not sure if we want to deal with multiple background commands at the same time

Composite commands are going to be especially tricky :-(
That is another reason why I was thinking that it might be better to just have fixed default background behavior for each command type, rather expose it as an option.

Composite commands should be treated as a series of one-shot commands (background: false).
When run command is composite, we could essentially wrap all commands, execute them one by one and the whole wrapper would then become a background process. So it will be up to the devfile created to make sure that it behaves correctly.
But then we have another problem, and that is composite with parallel: true, I don't have an idea of how to deal with the situation when there is a composite run command with parallel: true

[elsony]

Composite commands should be treated as a series of one-shot commands (background: false).
When run command is composite, we could essentially wrap all commands, execute them one by one and the whole wrapper would then become a background process. So it will be up to the devfile created to make sure that it behaves correctly.
But then we have another problem, and that is composite with parallel: true, I don't have an idea of how to deal with the situation when there is a composite run command with parallel: true

I like this approach to have fixed default background behaviour and treat all commands in a composite command as one-shot commands and using a wrapper to implement background in run. This will probably still work with parallel: true. In that case, the devfile creator will need to ensure there is at least one of those command is long running.

[kadel]

in PR #96 I have renamed restartOnChange to hotReloadCapable so we can have a default value of this flag false.

Boolean fields in Json files with default value true are complicated to handle in Go.