fix cli config command, cli log, jmx setting

[riuvshin]

What does this PR do?

various fixes for cli and che configuration

• fix missing parts and wrong ENV names
• fix cli.log overwrite
• fix config command
• fix jmx settings

What issues does this PR fix or reference?

#4408

Changelog

fix missing parts and wrong ENV names
fix cli.log overwrite
fix config command
fix jmx settings

[riuvshin]

@TylerJewell I have questions about this part https://github.com/eclipse/che/blob/master/dockerfiles/che/entrypoint.sh#L245-L268

it somehow conflict with those changes and looks very strange, there is some replacements which modify che.propr that I generate. this looks like unnecessary things but im not sure

[TylerJewell]

@riuvshin - wow! this seems like a huge set of changes.

Ok, so if I understand what is happening:

1. You are taking che.env
2. You are then extracting values from that
3. You then are using puppet with a che.properties template to generate a che.properties file
4. You then place this custom che.properties into the /instance folder

My question in reviewing the code was that I didn't see where you configured the docker-compose template to configure eclipse/che-server to use that custom properties file. Is that done anywhere?

I think the part where you have questions on the che.properties may or may get in the way. In the entrypoint.sh of the eclipse/che-server, the sed commands were designed to make sure that the che.properties that is loaded has mount paths that the container can properly access.

BTW, if you notice in the configuration of those properties most of the properties are configured with the container mounts of /data/*. But a couple of the mounts (for the agents) are host mounts provided by the end user. If you are getting the right values passed into puppet and assigned, then we can probably these configuration items in the entrypoint. But we should verify that eclipse/che-server can be used without our CLI.

[riuvshin]

@TylerJewell

My question in reviewing the code was that I didn't see where you configured the docker- compose template to configure eclipse/che-server to use that custom properties file. Is that done anywhere?

yes it is set here https://github.com/eclipse/che/blob/reworkcheinit/dockerfiles/init/modules/che/templates/che.env.erb#L1 I just set CHE_LOCAL_CONF_DIR=/conf and che.properties is in this folder

[codenvy-ci]

Build finished.
Build success. $BUILD_URL

[codenvy-ci]

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2164/

[benoitf]

What I dislike is that it introduces a duplicate of che.properties
there is one there https://github.com/eclipse/che/blob/master/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties
and then one inside the init image
So, in short term the two files won't be in sync and ppl won't think to modify both files so it's gonna fail ? (it always happen when a file is duplicated)

What I don't understand is that we allow override over ENV variables, so why do we need to duplicate che.properties ? instead of only providing env variables through the che.env file ?

[riuvshin]

@benoitf https://github.com/eclipse/che/blob/master/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties will be removed in this pr, we already discussed this with @vparfonov

[benoitf]

@riuvshin so it would mean that che-server image will no longer work in standalone mode ? only through che-init ?

[riuvshin]

@benoitf that is exactly what we wanted to do, right?
and actually it will be possible as che.prop is generated and there... we also have che.sh generated which may run che with this config

[benoitf]

@riuvshin this is where I'm not sure. AFAIK I thought che-server should still work as a standalone, cli being only an helper, not the bootstrap but I may be wrong

[TylerJewell]

Yes that is correct. We are trying to allow Che server image to still work stand alone.

[TylerJewell]

Ok - so to summarize after a meeting with @riuvshin - we decided that:

1. We should remove the che.properties that is embedded in WEB-INF.

2. We should then re-test eclipse/che-server to verify that it works with defaults, possibly updating any unnecessary sed statements inside of the entrypoint.sh.

3. We may need to revisit the syntax on this docs page: https://www.eclipse.org/che/docs/setup/docker/index.html

What this does is:

1. Makes the Che & Codenvy model for configuration identical.
2. Removes confusion on the embedded che.properties - so devs know where to apply new configurations.
3. This may also remove some unnecessary complex code from the eclipse/che-server image.
4. This will also fix the broken che.sh native script that is generated in the /instance folder because it can be reconfigured to make use of the customized che.properties.

[benoitf]

--> We should remove the che.properties that is embedded in WEB-INF.
--> We should then re-test eclipse/che-server to verify that it works with defaults

I don't see how che-server can work by default as the default is given by the properties file. Removing the file remove the default then it will start with "missing property when performing guice injection"

[TylerJewell]

I thought we updated the core code such that every property had some sort of embedded default? The error that you cited would imply otherwise.

[riuvshin]

@TylerJewell no, @benoitf is right it will not work without che.properties at all.
but with cli or che.sh we will have che.prop generated with puppet this is similar to codenvy

[TylerJewell]

@riuvshin - do you have any guess on what aspects break and whether we can take those defaults and moved them into Java code, so we can get rid of che.properties formally?

[riuvshin]

@TylerJewell i don't think we can put all props dedaults in java code, but what is the problem if che will not start without cli or che.sh ? this is exactly what we wanted before. And it is similar to codenvy, you cant run codenvy without cli, codenvy never had embedded "default" properties.

[benoitf]

maybe we could during che-server build, reuse properties from che-init ?
so property file is still in che-init, but che-server includes a copy

[TylerJewell]

I think roman that it is just poor design in that situation. When I look at all of the popular single server docker images at docker hub you just don't see any that require a properties file to work properly.

So it goes to the integrity of the eclipse/che-server image as a stand alone server.

[riuvshin]

@TylerJewell there is a huge amount of apps that uses config file and this is totally ok, for example haproxy it comes with default config and it will not start without config

[TylerJewell]

So it sounds like both florent and Roman that you both like the idea of discontinuing the Che server stand alone image and instead require uses to provide a Che properties (no more embedded properties inside the image). And so if you use the cli everything is ok. But if you just run the server image by itself then you need to provide a props file.

[riuvshin]

im start thinking maybe we should codenvy to be similar to che....
that approach with ENV vars is easier than generarting prop files and much more flexible...

[TylerJewell]

Yes, I agree with this direction. It will be easier to maintain going forward.

[riuvshin]

so we just talked with @TylerJewell and we both agreed that we have to close this PR and make codenvy work same as CHE. through ENV vars instead of generation properties, which will be much easier to maintain and flexible way to deal with configuration.
@benoitf WDYT?

[benoitf]

@riuvshin I'm fine with that approach

alternate way was as part of the build of che-server image, inject a properties file build from init image (dependency from init to server)

[benoitf]

@riuvshin it's closed or open ? :-)

[riuvshin]

@benoitf @TylerJewell i've figured out that there i did some fixes for cli and init, to not lose it im updating pr to keep only those changes

[benoitf]

what are these ports ? JMX ?
I would say they should be exposed only if a property is defined

[riuvshin]

yes jmx and it is always defined as we always generate jmx config

[benoitf]

yes but should it be exposed only if --debug is enabled for example ?

[riuvshin]

i don't think this is related to debug,in codenvy we have this always available. for example some monitoring tools require jmx connection and you may monitor app in production mode

[benoitf]

well I would say that it should be turned off by default (or an option to turn it off) as you may not require JMX at all

[riuvshin]

i've added possibility to enable/disable jmx (disabled by default)

[benoitf]

wonderful

[codenvy-ci]

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2216/

[TylerJewell]

Why did you add skip config into the file you placed it into? Isn't it already in the 02 loader as a global check?

[riuvshin]

@TylerJewell maybe I did it wrong but without it it fails with:

INFO: (che config): Generating che configuration...
/scripts/base/commands/cmd_config.sh: line 84: skip_config: command not found

I've found same function in cmd_start https://github.com/eclipse/che/blob/master/dockerfiles/base/scripts/base/commands/cmd_start.sh#L302 that is why i decided to add it there.

[TylerJewell]

I see. Ok, so there are two locations for skip_ functions:

1. Globally in the 02.sh file, or:
2. If not needed in multiple locations, in the cmd_*.sh file.

Prevoiusly, the only place that skip config was needed was the cmd_start, so it is located there. However, if you need to reuse it, don't duplicate it into cmd_config and instead move it into the 02.sh file please.

[riuvshin]

@TylerJewell done

[codenvy-ci]

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2219/