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

Rewrite of entry.sh to make it more robust and modular #139

Merged
merged 30 commits into from
Mar 4, 2024

Conversation

HaraldR42
Copy link
Contributor

After having a number of regressions esp. on logging to the console, I decided to make a rewrite of the script.
Please consider my changes, sorry for them being so massive.
I'm open for any question/discussion.
Thx in advance.

@sidey79
Copy link
Contributor

sidey79 commented Sep 17, 2023

Nice,

There seems to be a bug, maybe more, in the script.

One i already fixed to get it working local.
Not sure how i can test everything. I'll also check the script via shellcheck.

@HaraldR42
Copy link
Contributor Author

Thanks,
I fully admit that I did't test all cases.
I mainly concentrated on the whole console and process management stuff.
The build tests obviously discovered at least one.
Let me know how I could assist.
BTW: How do you run the build checks locally?

@sidey79
Copy link
Contributor

sidey79 commented Sep 19, 2023

The build check only verify if the container starts proper and ist healthy.
This is currently failing. I add a fix for this.

The functions of the entry.sh script are not tested currently.
Local build of the container is possible but not extreme easy, because it is expected, that FHEM ist cloened from SVN before. Then you can start docker build process and start the Image.

I changed the entry.sh this a little bit and added a svn checkout during startup.
This makes it easier to start the container for testing. If no FHEM ist available to the container a checkout will be done.

To get some options to test the functions in the entry script, i looked at the bats-core projekt.
It seems to be possible to add automated checks but we need to separate the functions from the main script.
Then some checks can bei added.
After this, i think we can spend some time to the shellcheck results.

@sidey79
Copy link
Contributor

sidey79 commented Sep 23, 2023

@HaraldR42

I figured out, that there are some changes in the entry.sh script.

As i understand, the attributes for the logfile are not changed, this is now a read only operation. So no changes will be done.

I figured out, that the telnetdefinition was missing, so i added code to apply the definition if it does not exists on the specified port.

@HaraldR42
Copy link
Contributor Author

@sidey79
Actually I left out the telnet definitions intentionally.
It was only used for triggering the shutdown, but sending a SIGTERm is fully equivalent (even checked it in the fhem.pl code). That is used now.
Also I couldn‘t find any other place where it was used except the health check, which does it‘s own analysis.
So, shouldn‘t we leave the telnet stuff out if it‘s not used?

PS: Sorry for being a bit unresponsive, but real job, family …

@sidey79
Copy link
Contributor

sidey79 commented Sep 23, 2023

Also I couldn‘t find any other place where it was used except the health check, which does it‘s own analysis. So, shouldn‘t we leave the telnet stuff out if it‘s not used?

Currently, the healthcheck isn't working without a telnet device. May we can patch this.

@HaraldR42
Copy link
Contributor Author

I was now thinking back and forth if it's wise to get rid of the telnet device. My conclusions:

  • If don't have the telnet device, we would need to do all of the health check stuff via web interface.
  • This might either imply
    1. Loosening security restrictions that one might have in place to make it automatically executable (e.g. login)
    2. Or adding another local and open web interface
      Variant 1) is not acceptable, variant 2) is quite equivalent to having a telnet device.
      => Imho we simply should stick with the telnet device (even if I don't like it ;-)

But I would suggest the following improvements:
I think we could assume, that the telnet port and even the web interfaces are stable for each container launch.
=> We could make the analysis of the telnet port and the web interfaces once at startup of FHEM, and write the results to file(s).
=> Then we could simplify the health check by just reading this file(s), instead of making all the analysis over and over again. And in case of the telnet port we would be sure that everything is consistent.

Your opinion?
If you consent, I would do an implementation accordingly.

@sidey79
Copy link
Contributor

sidey79 commented Sep 24, 2023

  • If don't have the telnet device, we would need to do all of the health check stuff via web interface.
  • This might either imply

The telnet device itself is only used, to query all the fhemweb devices.
For fhem.cfg varaint, this also could be archived via grep or the other way query via http(s) interface.

  1. Loosening security restrictions that one might have in place to make it automatically executable (e.g. login)

The only option i see is, that we can create a separate telnet device, without an password but with filter towards localhost.

  1. Or adding another local and open web interface
    Variant 1) is not acceptable, variant 2) is quite equivalent to having a telnet device.
    => Imho we simply should stick with the telnet device (even if I don't like it ;-)

I was thinking about this also, and added the telnet device explicit for docker with a meaningfull name, if there is no telnet device already present at the port, it will be created. We don't need to expose this port or allow others as localhost.

But I would suggest the following improvements: I think we could assume, that the telnet port and even the web interfaces are stable for each container launch. => We could make the analysis of the telnet port and the web interfaces once at startup of FHEM, and write the results to file(s). => Then we could simplify the health check by just reading this file(s), instead of making all the analysis over and over again. And in case of the telnet port we would be sure that everything is consistent.

I was thinking about the same, that querying the fhemweb devices every 10 seconds is not needed.
May we could also provide envrioment settings for controlling the health check, but for most users already using the docker image it should work as before, so i think, we can check this at container startup.

Your opinion? If you consent, I would do an implementation accordingly.

May we can make the first step, of writing a static file which is used by the healthcheck script doing it's work and then adapt the health_check script working with that file.

There is an option, extending the 98_DockerInfo.pm writing this Information at FHEM start. This way it should be very easy to provide an up to date information about the fhemweb devices to use for the healthcheck.
The 98_Dockerinfo.pm can write the information to a temp file, and if needed also update it.

It would be great if you make a proposal of this file.

@HaraldR42
Copy link
Contributor Author

HaraldR42 commented Sep 25, 2023

May we can make the first step, of writing a static file which is used by the healthcheck script doing it's work and then adapt the health_check script working with that file.

There is an option, extending the 98_DockerInfo.pm writing this Information at FHEM start. This way it should be very easy to provide an up to date information about the fhemweb devices to use for the healthcheck. The 98_Dockerinfo.pm can write the information to a temp file, and if needed also update it.

It would be great if you make a proposal of this file.

Very good idea. I just committed a new health-check.sh and a corresponding 98_DockerInfo.pm.
The two now cooperate as follows:

  • 98_DockerInfo.pm writes a file after FHEM startup containing all URLs of web servers enabled for health checking.
  • The health-check.sh looks for this file and checks the URLs
  • The results are written to a second file
  • 98_DockerInfo.pm periodically (every 30s) checks the result file and reads the content into STATUS

This way we completely get rid of the telnet in the health-check.sh, and even the config-file parsing in health-check.sh is not needed any more. This makes it even suitable for setups using a db.

@sidey79
Copy link
Contributor

sidey79 commented Nov 11, 2023

@HaraldR42

I started adding some tests via bats.

There is one test "check setGlobal_LOGFILE from fhem.cfg" i think, the result for this test is not correct, but i was not able to identify why.

src/entry.sh Outdated Show resolved Hide resolved
chown ${FHEM_UID}:${FHEM_GID} "$realLogFile"
tailFileToConsoleStart "$realLogFile" # Start writing the logfile to the console

# setTelnet_DEFINITION
Copy link
Contributor

Choose a reason for hiding this comment

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

@HaraldR42

I disabled the creation of a telnet device for docker, because i think with this improved healh check this is not needed anymore

@sidey79
Copy link
Contributor

sidey79 commented Nov 18, 2023

Added serval unittetss testing most of the bash script functions

image

@sidey79 sidey79 linked an issue Mar 2, 2024 that may be closed by this pull request
@sidey79 sidey79 merged commit 2b9a58b into fhem:dev Mar 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite of entry.sh / question on contributing
2 participants