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 (was: WIP) #67

Merged
merged 22 commits into from
Jan 18, 2017
Merged

Rewrite (was: WIP) #67

merged 22 commits into from
Jan 18, 2017

Conversation

bebehei
Copy link
Contributor

@bebehei bebehei commented Dec 21, 2016

Issues to fix:

  • include graphite module
  • some refactoring
  • docs improvement
  • navigation items are lost after creation of new container
    • Resolved: I forgot to mount volume on /etc/icingaweb2
  • module information on status pages is not
    • fix: add module path declaration and add /etc/icingaweb2/modules
  • unhardcode passwords (Protecting web access with password #40)

@bebehei
Copy link
Contributor Author

bebehei commented Dec 21, 2016

Ok, to clearify this. I'll push many changes here. If you could review them, that'll be great.

But please don't merge them by now. I will add more (and bigger) commits in the next days/weeks. If you would merge these all one by one, that would give a very confusing commit history.

@@ -219,5 +212,5 @@ echo "==================================================================="
touch ${initfile}
fi

echo "Starting Supervisor. You can safely CTRL-C and the container will continue to run with or without the -d (daemon) option"
echo "Starting Supervisor."
Copy link
Owner

Choose a reason for hiding this comment

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

I added this note to let users know it was OK to break out as there seemed to be a lot of confusion for new users. I think we should keep it in for now 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I experienced differently. Every time i hit Ctrl+C it stops.

Probably this is, because docker run bridges the sequence to the supervisor.

using docker 1.12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've heard docker minor versions are often not compatible to each other. In fact I'm quite new to docker software and I have only used the 1.12 version yet. But I have to say, that this is sadly not true in 1.12.

[bebe:~/code/icinga2-docker] devtest ± docker run --rm -ti jordan/icinga2
[...]
Starting Supervisor.  You can safely CTRL-C and the container will continue to run with or without the -d (daemon) option
^C
[bebe:~/code/icinga2-docker] devtest ± docker ps
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
[bebe:~/code/icinga2-docker] devtest ±

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's because you're running the container with the -i interactive flag so you're actually sending the break to supervisor and not the host.

Copy link
Owner

@jjethwa jjethwa Dec 22, 2016

Choose a reason for hiding this comment

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

Just tested it out and it does appear to be the case. To detach from an interactive session, you would need to use sequence of CTRL + p and CTRL + q sequence.

Unless there's a reason to start a container in interactive mode, I think the general recommendation is to run with the -d daemon flag or since this container uses supervisor to run the services, you can run it without the -d daemon flag and just escape out

@jjethwa
Copy link
Owner

jjethwa commented Dec 22, 2016

Hi @bebehei

Thanks! I really appreciate all the work you've done. I've added you as a collaborator. Docker Hub is set up to build master -> latest, dev -> latest, and tagged as their tag numbers. For this PR, do you mind submitting it to the dev branch? Then once it's checked out from the Docker hub dev container, you can submit a PR from dev to master?

Unfortunately, I don't have tests set up externally. My tests run through basic scenarios using the environment variables and volumes (empty + pre-existing configs, database, etc)

@bebehei
Copy link
Contributor Author

bebehei commented Jan 5, 2017

Thanks! I really appreciate all the work you've done. I've added you as a collaborator. Docker Hub is set up to build master -> latest, dev -> latest, and tagged as their tag numbers. For this PR, do you mind submitting it to the dev branch? Then once it's checked out from the Docker hub dev container, you can submit a PR from dev to master?

Thanks, I appreciate to be a "Member" of this repository. But actually, I prefer working on in this PR. This makes force pushing more convenient than doing it on an official branch. Or are there any improvements?

Unfortunately, I don't have tests set up externally. My tests run through basic scenarios using the environment variables and volumes (empty + pre-existing configs, database, etc)

Do you have tests? If yes, where are these located? Currently, I just test it manuall with every change.

@jjethwa
Copy link
Owner

jjethwa commented Jan 5, 2017

Hi @bebehei

Agreed on using PRs for work like you have been. Added you as a collaborator so you could merge PRs and perform other repo tasks as needed as I don't want to hold you back 😄

Right now the tests are not in a good shape to share as they are hardcoded to use my local infra and are not using a proper framework like Selenium. We should look into setting up CircleCI, Travis, etc to build the container and run tests against them in the future.

@bebehei
Copy link
Contributor Author

bebehei commented Jan 6, 2017

Hi @jjethwa,

I would like to merge this now. Could you please review this? This might have a bug I have not hit (yet).

  • refactored code:
    • split everything related to setting up the service into a separate file
    • many simplifications
  • initfile is not neccessary anymore. All changes, which are changeable with an ENV-Variable are now safe to reexecute without overwriting manual settings on already installed ones, too.
  • icingaweb2-log in /var/log/icingaweb2
  • modulepath includes now /etc/icingaweb2/modules, too
  • added support for login-credentials (067fb3d)
  • added graphite web support (7916a30)

things not done yet: (PRs will follow)

  • more detailed README
  • http -> https redirection
  • detailed docker-compose.yml with graphite and maybe support for external mysql DB

@bebehei
Copy link
Contributor Author

bebehei commented Jan 6, 2017

And as you may know: docker pull bebehei/icinga2:smaller-install gives you this PR-branch prebuilt by docker hub.

show_stacktraces = "1"
config_backend = "db"
config_resource = "icingaweb_db"
module_path = "/usr/share/icingaweb2/modules:/etc/icingaweb2/modules"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, after reconciling the the module-path change, I believe it is still incorrect.

Currently, all modules get installed into /etc/icingaweb2/modules/. But AFAIK, there should be only configurations. Additionally bad, we install the modules onto a volume, what we won't update anymore, if there is already configuration.

It might be better to install generally the modules into a container-only-folder like /opt/icingaweb2-modules/.

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed. The RC version of Icinga Web 2 had the modules installed under /etc/icingaweb2/modules instead of /use/share/icingaweb2/modules. I believe the RPM version still does that, but the DEB version was updated later to install the modules under /use/share/icingaweb2/modules. Since this container is already in the wild and being used by users in Production, we'd need to make sure that any changes don't break the current set up.

Perhaps install the module to their correct path under /usr/share/icingaweb2/modules then symlink from /etc/icingaweb2/modules to /usr/share/icingaweb2/modules ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is the module_path for icingaweb2. It just works like a normal path-variable and tells icignacli/icingaweb2 where to search for plugins. I think we have to define 3 locations:

  • for self-installed modules from the user -> /etc/icingaweb2/modules-user (on mounted volume)
  • for the ones installed from docker-container -> /opt/icingaweb2/modules (not on volume)
  • for the ones by debian-packages -> /usr/share/icingaweb2/modules

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good! Right now, the only example of a docker from the docker-container would be https://github.com/findmypast/icingaweb2-module-graphite right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the dockerfile currently uses the official module. I thought about using findmypast's module, but it has got some technical disadvantages.

Copy link
Owner

Choose a reason for hiding this comment

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

I use the findmypast's module, but you also need to use the official module to send the data to carbon

GRANT SELECT, INSERT, UPDATE, DELETE, DROP, CREATE VIEW, INDEX, EXECUTE ON icinga.* TO 'icinga'@'localhost' IDENTIFIED BY '${ICINGA_PASSWORD}';
END

mysql icinga < /usr/share/icinga2-ido-mysql/schema/mysql.sql >> /var/log/icinga2/mysql-schema.log 2>&1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Verify, that consecutive schema-executions are safe to do.

During testing, I have not experienced any issues, but: I have seen no proof, that this is safe.

Copy link
Owner

Choose a reason for hiding this comment

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

Is there a reason to run the schema again? Is this to handle icinga2-ido-mysql upgrades?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The key thought on this change had been:

On a docker-container you're able to configure your stuff via ENV-variables. And you save the stuff via volumes. So during boot, the docker-container should process the variables and change the volume data to represent the ENV-settings. The current master uses a logfile approach and changes everything on first container boot and touches a lockfile.

While volumes are persistent over container creation, ENVs are not. This leads to a new problem: If you change your ENVs, the volume data does not change automatically. So, my intention is to run the complete setup on every boot, but only change neccessary values without overwriting other stuff (which may be created manually by the user). This is why, I removed the sed-calls with ini_set, etc.

I created this initial note, to remember, that I haven't tested this actively. It worked, but I didn't know if it worked correctly.

As you mentioned, schema upgrades are also another topic, which this image still requires manual interaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested it: MySQL just stops after the first failing command, because there are duplicate columns in DB-Table. And first failing command is the first command, if the DB is already created and filled. So nothing bad will happen. I would "mark" this as done.

@bebehei
Copy link
Contributor Author

bebehei commented Jan 8, 2017

Except those small showstoppers, I'd like to finalise it and merge this heavy PR.

I'd like to get Feedback, of course. ;-)

| /var/lib/mysql | rw | MySQL Database |
| /var/lib/icinga2 | rw | Icinga2 Data |
| /var/log/apache2 | rw | logfolder for apache2 (not neccessary) |
| /var/log/icinga2 | rw | logfolder for icinga2 (not neccessary) |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Have all the log-dirs been chowned by the setup-scripts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed this now, too.

@bebehei
Copy link
Contributor Author

bebehei commented Jan 16, 2017

So, I added some fixes again. Additionally, I want to fix correct support on module_path. I will work on this in a few hours again. The code should then contain no known issues anymore.

@bebehei
Copy link
Contributor Author

bebehei commented Jan 16, 2017

I want to fix correct support on module_path. I will work on this in a few hours again.

Fixed. I'll test it the next days, but I guess this is now safe to merge.

@bebehei
Copy link
Contributor Author

bebehei commented Jan 16, 2017

I looked on the issues in the issue tracker and I have to conclude: By merging this PR, you fix everything in this repo, except my feature request ;-)

@jjethwa
Copy link
Owner

jjethwa commented Jan 18, 2017

Thanks @bebehei

Re-running the tests. Let's put a freeze on any new changes to the branch/PR unless it's a major bug so we can get it fully tested and merged. 😛

@bebehei
Copy link
Contributor Author

bebehei commented Jan 18, 2017

Yes! For the last days, I'm doing this, too.

@jjethwa
Copy link
Owner

jjethwa commented Jan 18, 2017

@bebehei Looks good on my end and all my test cases seem to work. I did have to update one old config file under /etc/icinga2 but that was my fault for not keeping it updated. Thanks again for all of your hard work! 😄

@jjethwa jjethwa merged commit 7b62f16 into jjethwa:master Jan 18, 2017
@bebehei
Copy link
Contributor Author

bebehei commented Jan 18, 2017

Thanks. :stunned:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants