[GitHub] cloudstack pull request: CLOUDSTACK-8272: Python based file-lock f...
Github user vincentbernat commented on the pull request: https://github.com/apache/cloudstack/pull/106#issuecomment-78031911 Some remarks: - Use `with lock:` instead of `lock.acquire()` then `try/finally` (compatible with Python 2.6) - Use `with file(...) as f:` (idem) - `global passMap` and `global lock` are useless since you don't do direct assignments - `getPassword()` could be `passMap.get(..., None)` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request:
Github user vincentbernat commented on the pull request:
https://github.com/apache/cloudstack/commit/8e953fe8593cf624c37213d94f79ea9e17a12f27#commitcomment-10118901
In systemvm/patches/debian/config/opt/cloud/bin/passwd_server_ip.py:
In systemvm/patches/debian/config/opt/cloud/bin/passwd_server_ip.py on line
68:
```python
try:
with file(getPasswordFile()) as f:
for line in f:
if '=' not in line: continue
key, val = line.strip().split('=', 1)
passMap[key] = val
except IOError:
pass
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request:
Github user vincentbernat commented on the pull request:
https://github.com/apache/cloudstack/commit/8e953fe8593cf624c37213d94f79ea9e17a12f27#commitcomment-10118935
In systemvm/patches/debian/config/opt/cloud/bin/passwd_server_ip.py:
In systemvm/patches/debian/config/opt/cloud/bin/passwd_server_ip.py on line
76:
```python
with file(getPasswordFile(), 'w'):
for ip in passMap:
f.write('%s=%s\n' % (ip, passMap[ip])
```
(use of `with` and iteration on a dictionary is done on keys by default)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: sysctl: don't modify /etc/sysctl.conf
GitHub user vincentbernat opened a pull request: https://github.com/apache/cloudstack/pull/776 sysctl: don't modify /etc/sysctl.conf To configure firewall rules, CloudStack modifies `/etc/sysctl.conf` and execute those modifications. This may be harmful for several reasons: 1. `/etc/sysctl.conf` may be managed by some configuration management system. Such a system will constantly restore the previous version. 2. `/etc/sysctl.conf` may contain additional properties that have been changed later by some system administrator (for example, once a firewall has been configured, forwarding may have been activated while it is disabled in `/etc/sysctl.conf`). Executing the file again at a later time may disrupt the system. 3. Entries are added again and again. `/etc/sysctl.conf` will contain the same directives repeated several times. Using a configuration file is not needed as `sysctl` is able to directly modify sysctl values with `-w` flag. Signed-off-by: Vincent Bernat You can merge this pull request into a Git repository by running: $ git pull https://github.com/exoscale/cloudstack fix/firewall-sysctl Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/776.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #776 commit f2b8f2eade26166adc329a4d334fad034c22fd54 Author: Vincent Bernat Date: 2015-09-04T12:31:09Z sysctl: don't modify /etc/sysctl.conf To configure firewall rules, CloudStack modifies `/etc/sysctl.conf` and execute those modifications. This may be harmful for several reasons: 1. `/etc/sysctl.conf` may be managed by some configuration management system. Such a system will constantly restore the previous version. 2. `/etc/sysctl.conf` may contain additional properties that have been changed later by some system administrator (for example, once a firewall has been configured, forwarding may have been activated while it is disabled in `/etc/sysctl.conf`). Executing the file again at a later time may disrupt the system. 3. Entries are added again and again. `/etc/sysctl.conf` will contain the same directives repeated several times. Using a configuration file is not needed as `sysctl` is able to directly modify sysctl values with `-w` flag. Signed-off-by: Vincent Bernat --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: sysctl: don't modify /etc/sysctl.conf
Github user vincentbernat commented on the pull request: https://github.com/apache/cloudstack/pull/776#issuecomment-137765042 @resmo Yes, it could be in `/etc/sysctl.d` instead (and no sysctl call would be needed). I don't have a strong opinion on this (except in this case, the code should then be removed from security_group.py). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: sysctl: don't modify /etc/sysctl.conf
Github user vincentbernat commented on the pull request: https://github.com/apache/cloudstack/pull/776#issuecomment-138473948 @resmo the only problem with shipping a `/etc/sysctl.d/cloudstack.conf` is that this is more a packaging issue. It has to be handled for all distributions. And what about people installing from source? This doesn't seem an easy problem. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1541: Systemd packaging for Ubuntu 16.04
Github user vincentbernat commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1541#discussion_r73830499 --- Diff: debian/cloudstack-agent.install --- @@ -18,12 +18,11 @@ /etc/cloudstack/agent/agent.properties /etc/cloudstack/agent/environment.properties /etc/cloudstack/agent/log4j-cloud.xml +/etc/default/cloudstack-agent /etc/profile.d/cloudstack-agent-profile.sh /etc/logrotate.d/cloudstack-agent -/etc/init.d/cloudstack-agent /usr/bin/cloudstack-setup-agent --- End diff -- You can keep the init.d file for everybody. If the the system is running systemd, a service with the same basename will override the init.d file. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1541: Systemd packaging for Ubuntu 16.04
Github user vincentbernat commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1541#discussion_r73830557 --- Diff: debian/control --- @@ -3,7 +3,7 @@ Section: libs Priority: extra Maintainer: Wido den Hollander Build-Depends: debhelper (>= 9), openjdk-8-jdk | openjdk-7-jdk, genisoimage, - python-mysql.connector, maven (>= 3) | maven3, python (>= 2.7) + python-mysql.connector, maven (>= 3) | maven3, python (>= 2.7), lsb-release --- End diff -- For systemd integration, you need to Build-Depends on dh-systemd. Unfortunately, this will make the package unbuildable on Precise. See below. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1541: Systemd packaging for Ubuntu 16.04
Github user vincentbernat commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1541#discussion_r73830730 --- Diff: debian/rules --- @@ -5,9 +5,18 @@ PACKAGE = $(shell dh_listpackages|head -n 1|cut -d '-' -f 1) SYSCONFDIR = "/etc" DESTDIR = "debian/tmp" +ifeq ($(shell lsb_release -sr), 14.04) +SUBSTVARS = -Vjre:Depends="openjdk-7-jre-headless" -Vjdk:Depends="openjdk-7-jdk" -Vinit:"Depends:jsvc" +else +SUBSTVARS = -Vjre:Depends="openjdk-8-jre-headless" -Vjdk:Depends="openjdk-8-jdk" -Vinit:"Depends:systemd" +endif + %: dh $@ --with python2 --- End diff -- You need to do `--with python2,systemd`. Otherwise, systemd unit won't be enabled automatically and the integration won't happen (no restart of the service on upgrade for example). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1541: Systemd packaging for Ubuntu 16.04
Github user vincentbernat commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1541#discussion_r73830801 --- Diff: debian/rules --- @@ -116,17 +132,24 @@ override_dh_auto_install: # cloudstack-usage mkdir $(DESTDIR)/$(SYSCONFDIR)/$(PACKAGE)/usage - mkdir $(DESTDIR)/var/log/$(PACKAGE)/usage mkdir $(DESTDIR)/usr/share/$(PACKAGE)-usage mkdir $(DESTDIR)/usr/share/$(PACKAGE)-usage/plugins install -D usage/target/cloud-usage-$(VERSION).jar $(DESTDIR)/usr/share/$(PACKAGE)-usage/lib/$(PACKAGE)-usage.jar install -D usage/target/dependencies/* $(DESTDIR)/usr/share/$(PACKAGE)-usage/lib/ cp usage/target/transformed/db.properties $(DESTDIR)/$(SYSCONFDIR)/$(PACKAGE)/usage/ cp usage/target/transformed/log4j-cloud_usage.xml $(DESTDIR)/$(SYSCONFDIR)/$(PACKAGE)/usage/log4j-cloud.xml - install -D packaging/debian/init/cloud-usage $(DESTDIR)/$(SYSCONFDIR)/init.d/$(PACKAGE)-usage + + install -d -m0755 debian/$(PACKAGE)-usage/etc/init.d + install -D -m0755 packaging/debian/$(PACKAGE)-usage.init debian/$(PACKAGE)-usage/etc/init.d/$(PACKAGE)-usage + install -d -m0755 debian/$(PACKAGE)-usage/lib/systemd/system + install -m0644 packaging/debian/$(PACKAGE)-usage.service debian/$(PACKAGE)-usage/lib/systemd/system/$(PACKAGE)-usage.service + install -m0644 packaging/debian/$(PACKAGE)-usage.default $(DESTDIR)/$(SYSCONFDIR)/default/$(PACKAGE)-usage override_dh_installinit: dh_installinit -pcloudstack-management -pcloudstack-agent -pcloudstack-usage --onlyscripts --no-start +override_dh_systemd_enable: + dh_systemd_enable -pcloudstack-agent -pcloudstack-usage + --- End diff -- This bit needs the `--with systemd` bit mentioned above. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1541: Systemd packaging for Ubuntu 16.04
Github user vincentbernat commented on the issue: https://github.com/apache/cloudstack/pull/1541 The only change I have noticed is the change in postinst. This could bypass the local administrator policy. It would not restart the agent on upgrade either. I would suggest to : - Build-Depends on dh-systemd - Add --with systemd, python2 do dh invocation - Remove the postinst snippet If you don't want to add a dependency to dh-systemd, the override in debian/rules is useless and the postinst snippet should be more complex. Check for example `/var/lib/dpkg/info/openssh-server.postinst` for the sections `dh_systemd_enable`. Note that there are other snippets in postrm. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1541: Systemd packaging for Ubuntu 16.04
Github user vincentbernat commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1541#discussion_r74198886 --- Diff: debian/rules --- @@ -136,5 +156,8 @@ override_dh_auto_install: override_dh_installinit: dh_installinit -pcloudstack-management -pcloudstack-agent -pcloudstack-usage --onlyscripts --no-start +override_dh_systemd_enable: + dh_systemd_enable -pcloudstack-agent -pcloudstack-usage --with systemd --- End diff -- I didn't mean the `--with systemd` to be here, just on the `dh` invocation in the `%` target (like it now is). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1541: Systemd packaging for Ubuntu 16.04
Github user vincentbernat commented on the issue: https://github.com/apache/cloudstack/pull/1541 Except the extra `--with systemd`, LGTM --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1647: [lts] CLOUDSTACK-9462: Systemd support for Ubuntu 16...
Github user vincentbernat commented on the issue: https://github.com/apache/cloudstack/pull/1647 LGTM for the Ubuntu part. You could share `/etc/sysconfig/*` and `/etc/default/*` files between RHEL/Ubuntu (same files, just installed at different locations). Same for the service files: ``` [service] EnvironmentFile=-/etc/default/cloudstack-agent EnvironmentFile=-/etc/sysconfig/cloudstack-agent ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1647: [lts] CLOUDSTACK-9462: Systemd support for Ubuntu 16...
Github user vincentbernat commented on the issue: https://github.com/apache/cloudstack/pull/1647 it is not possible to have systemd support and build on 12.04 at the same time. dh-systemd doesn't exist on 12.04 and its actions are quite important. If you remove it and just install the units manually, they won't start on boot or on upgrade. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1647: [lts] CLOUDSTACK-9462: Systemd support for Ub...
Github user vincentbernat commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1647#discussion_r76039128 --- Diff: debian/rules --- @@ -5,18 +5,9 @@ PACKAGE = $(shell dh_listpackages|head -n 1|cut -d '-' -f 1) SYSCONFDIR = "/etc" DESTDIR = "debian/tmp" -ifeq ($(shell lsb_release -sr), 14.04) -SUBSTVARS = -Vjre:Depends="openjdk-7-jre-headless" -Vjdk:Depends="openjdk-7-jdk" -Vinit:"Depends:jsvc" -else -SUBSTVARS = -Vjre:Depends="openjdk-8-jre-headless" -Vjdk:Depends="openjdk-8-jdk" -Vinit:"Depends:systemd" -endif - --- End diff -- I would have kept something like that but using init-system-helpers instead of systemd. It is not unheard of for people to not install recommends. However, init-system-helpers is likely to be installed on the target system already (since everything that comes with a systemd unit pulls it). So, it's a bit nitpicking. About Java, you can change the dependency to: ``` default-jre-headless (>= 1:1.8) | java8-runtime-headless | java8-runtime ``` This would enable people to use Oracle JRE instead. Most packages for non-OpenJDK JRE are providing `java8-runtime-headless` or `java8-runtime`. For JDK, this would be: ``` default-jdk-headless (>= 1:1.8) | java8-sdk | java8-jdk ``` The normal virtual package is `java8-sdk` but some unofficial packages are using `java8-jdk` instead. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1647: [lts] CLOUDSTACK-9462: Systemd support for Ubuntu 16...
Github user vincentbernat commented on the issue: https://github.com/apache/cloudstack/pull/1647 Otherwise, I think the change to be compatible with 12.04 is OK (no Upstart script should ever be provided in this case). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1647: [lts] CLOUDSTACK-9462: Systemd support for Ub...
Github user vincentbernat commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1647#discussion_r76200059 --- Diff: debian/rules --- @@ -5,18 +5,9 @@ PACKAGE = $(shell dh_listpackages|head -n 1|cut -d '-' -f 1) SYSCONFDIR = "/etc" DESTDIR = "debian/tmp" -ifeq ($(shell lsb_release -sr), 14.04) -SUBSTVARS = -Vjre:Depends="openjdk-7-jre-headless" -Vjdk:Depends="openjdk-7-jdk" -Vinit:"Depends:jsvc" -else -SUBSTVARS = -Vjre:Depends="openjdk-8-jre-headless" -Vjdk:Depends="openjdk-8-jdk" -Vinit:"Depends:systemd" -endif - --- End diff -- That's why I think that the use of `$SUBSTVARS` and `dh_gencontrol` override was a good idea. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1647: [lts] CLOUDSTACK-9462: Systemd support for Ub...
Github user vincentbernat commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1647#discussion_r76206826 --- Diff: debian/rules --- @@ -5,18 +5,9 @@ PACKAGE = $(shell dh_listpackages|head -n 1|cut -d '-' -f 1) SYSCONFDIR = "/etc" DESTDIR = "debian/tmp" -ifeq ($(shell lsb_release -sr), 14.04) -SUBSTVARS = -Vjre:Depends="openjdk-7-jre-headless" -Vjdk:Depends="openjdk-7-jdk" -Vinit:"Depends:jsvc" -else -SUBSTVARS = -Vjre:Depends="openjdk-8-jre-headless" -Vjdk:Depends="openjdk-8-jdk" -Vinit:"Depends:systemd" -endif - --- End diff -- Oh, OK, that's why you removed it. OK. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1647: [lts] CLOUDSTACK-9462: Systemd support for Ub...
Github user vincentbernat commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1647#discussion_r76225900
--- Diff: packaging/centos7/cloud.spec ---
@@ -482,7 +488,7 @@ pip install --upgrade
/usr/share/cloudstack-marvin/Marvin-*.tar.gz
%dir %attr(0770,root,cloud) %{_localstatedir}/cache/%{name}/management/work
%dir %attr(0770,root,cloud) %{_localstatedir}/cache/%{name}/management/temp
%dir %attr(0770,root,cloud) %{_localstatedir}/log/%{name}/management
-%config(noreplace) %{_sysconfdir}/sysconfig/%{name}-management
--- End diff --
RHEL users won't expect this file to be in /etc/default. In the systemd
unit:
```
EnvironmentFile=-/etc/default/cloudstack-agent
EnvironmentFile=-/etc/sysconfig/cloudstack-agent
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1647: [lts] CLOUDSTACK-9462: Systemd support for Ub...
Github user vincentbernat commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1647#discussion_r76226318
--- Diff: debian/cloudstack-agent.postinst ---
@@ -41,6 +41,12 @@ case "$1" in
mkdir /etc/libvirt/hooks
fi
cp -a /usr/share/cloudstack-agent/lib/libvirtqemuhook
/etc/libvirt/hooks/qemu
+
+# Update JAVA_HOME in /etc/default/cloudstack-agent to default JRE
+JAVA_HOME=$(readlink -f /usr/bin/java | sed 's/\/jre.*java//g')
+if [ -d "$JAVA_HOME" ]; then
+sed -i "s:^JAVA_HOME=.*:JAVA_HOME=${JAVA_HOME}:"
/etc/default/cloudstack-agent
+fi
--- End diff --
This would overwrite a change made by a user during upgrades. I would
suggest to move the first line directly in /etc/default/cloudstack-agent (if
it's compatible with RHEL). A user that didn't change anything will get the new
file and it will work as expected. A user that did alter the file will get a
prompt to solve the situation.
However, this won't work with systemd without further ugly hacks.
On my system, I notice that `/usr/lib/jvm/default-java` is a symlink. It
also seems to be the case on Precise. Maybe you could patch
`/etc/default/cloudstack-agent` after installing it in `debian/rules`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1647: [lts] CLOUDSTACK-9462: Systemd support for Ub...
Github user vincentbernat commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1647#discussion_r76226635
--- Diff: packaging/centos7/cloud.spec ---
@@ -482,7 +488,7 @@ pip install --upgrade
/usr/share/cloudstack-marvin/Marvin-*.tar.gz
%dir %attr(0770,root,cloud) %{_localstatedir}/cache/%{name}/management/work
%dir %attr(0770,root,cloud) %{_localstatedir}/cache/%{name}/management/temp
%dir %attr(0770,root,cloud) %{_localstatedir}/log/%{name}/management
-%config(noreplace) %{_sysconfdir}/sysconfig/%{name}-management
--- End diff --
I am a Debian guy, so I can't give you an authoritative answer. I didn't
know that `/etc/default` already existed on CentOS too. I suppose that's up to
you. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1647: [lts] CLOUDSTACK-9462: Systemd support for Ub...
Github user vincentbernat commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1647#discussion_r76281415 --- Diff: packaging/systemd/cloudstack-usage.default --- @@ -0,0 +1,25 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +JAVA_HOME=/usr/lib/jvm/jre +if [ ! -f "$JAVA_HOME"/bin/java ] ; then +JAVA_HOME=$(update-alternatives --display java | grep 'currently points to' | sed 's:.*currently points to ::' | sed 's:/bin/java::') +fi --- End diff -- How is that supposed to work with systemd? Moreover, the `readlink -f` solution was far more robust than parsing `update-alternatives` output. Let me propose something simpler inside `debian/rules` instead. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1647: [lts] CLOUDSTACK-9462: Systemd support for Ub...
Github user vincentbernat commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1647#discussion_r76281544 --- Diff: debian/rules --- @@ -35,13 +37,20 @@ override_dh_auto_install: # cloudstack-agent mkdir $(DESTDIR)/$(SYSCONFDIR)/$(PACKAGE)/agent mkdir $(DESTDIR)/$(SYSCONFDIR)/profile.d - mkdir $(DESTDIR)/var/log/$(PACKAGE)/agent mkdir $(DESTDIR)/usr/share/$(PACKAGE)-agent mkdir $(DESTDIR)/usr/share/$(PACKAGE)-agent/plugins install -D agent/target/cloud-agent-$(VERSION).jar $(DESTDIR)/usr/share/$(PACKAGE)-agent/lib/$(PACKAGE)-agent.jar install -D plugins/hypervisors/kvm/target/cloud-plugin-hypervisor-kvm-$(VERSION).jar $(DESTDIR)/usr/share/$(PACKAGE)-agent/lib/ install -D plugins/hypervisors/kvm/target/dependencies/* $(DESTDIR)/usr/share/$(PACKAGE)-agent/lib/ - install -D packaging/debian/init/cloud-agent $(DESTDIR)/$(SYSCONFDIR)/init.d/$(PACKAGE)-agent + + install -m0755 packaging/debian/$(PACKAGE)-agent.init $(DESTDIR)/$(SYSCONFDIR)/init.d/$(PACKAGE)-agent + install -d -m0755 debian/$(PACKAGE)-agent/lib/systemd/system + # Fix libvirt service name for Debian/Ubuntu + sed -i 's/Requires=libvirtd.service/Requires=libvirt-bin.service/g' packaging/systemd/$(PACKAGE)-agent.service + sed -i 's/After=libvirtd.service/After=libvirt-bin.service/g' packaging/systemd/$(PACKAGE)-agent.service + install -m0644 packaging/systemd/$(PACKAGE)-agent.service debian/$(PACKAGE)-agent/lib/systemd/system/$(PACKAGE)-agent.service + install -m0644 packaging/systemd/$(PACKAGE)-agent.default $(DESTDIR)/$(SYSCONFDIR)/default/$(PACKAGE)-agent --- End diff -- Here: ``` sed 's+JAVA_HOME=.*+JAVA_HOME=/usr/lib/jvm/default-java+' $(DESTDIR)/$(SYSCONFDIR)/default/$(PACKAGE)-agent ``` If people don't want to use the default JRE, they are free to change `/etc/default/cloudstack-agent`. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1647: [lts] CLOUDSTACK-9462: Systemd support for Ub...
Github user vincentbernat commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1647#discussion_r76320192 --- Diff: debian/rules --- @@ -35,13 +37,20 @@ override_dh_auto_install: # cloudstack-agent mkdir $(DESTDIR)/$(SYSCONFDIR)/$(PACKAGE)/agent mkdir $(DESTDIR)/$(SYSCONFDIR)/profile.d - mkdir $(DESTDIR)/var/log/$(PACKAGE)/agent mkdir $(DESTDIR)/usr/share/$(PACKAGE)-agent mkdir $(DESTDIR)/usr/share/$(PACKAGE)-agent/plugins install -D agent/target/cloud-agent-$(VERSION).jar $(DESTDIR)/usr/share/$(PACKAGE)-agent/lib/$(PACKAGE)-agent.jar install -D plugins/hypervisors/kvm/target/cloud-plugin-hypervisor-kvm-$(VERSION).jar $(DESTDIR)/usr/share/$(PACKAGE)-agent/lib/ install -D plugins/hypervisors/kvm/target/dependencies/* $(DESTDIR)/usr/share/$(PACKAGE)-agent/lib/ - install -D packaging/debian/init/cloud-agent $(DESTDIR)/$(SYSCONFDIR)/init.d/$(PACKAGE)-agent + + install -m0755 packaging/debian/$(PACKAGE)-agent.init $(DESTDIR)/$(SYSCONFDIR)/init.d/$(PACKAGE)-agent + install -d -m0755 debian/$(PACKAGE)-agent/lib/systemd/system + # Fix libvirt service name for Debian/Ubuntu + sed -i 's/Requires=libvirtd.service/Requires=libvirt-bin.service/g' packaging/systemd/$(PACKAGE)-agent.service + sed -i 's/After=libvirtd.service/After=libvirt-bin.service/g' packaging/systemd/$(PACKAGE)-agent.service + install -m0644 packaging/systemd/$(PACKAGE)-agent.service debian/$(PACKAGE)-agent/lib/systemd/system/$(PACKAGE)-agent.service + install -m0644 packaging/systemd/$(PACKAGE)-agent.default $(DESTDIR)/$(SYSCONFDIR)/default/$(PACKAGE)-agent --- End diff -- OK, the symlink comes with default-jre-headless package. If you don't have it, you don't have the symlink. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1647: [lts] CLOUDSTACK-9462: Systemd support for Ub...
Github user vincentbernat commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1647#discussion_r76320932 --- Diff: packaging/systemd/cloudstack-agent.default --- @@ -0,0 +1,24 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +JAVA_HOME=/usr/lib/jvm/jre +if [ ! -f "$JAVA_HOME"/bin/java ] ; then +JAVA_HOME=$(readlink -f /usr/bin/java | sed 's:/bin/java::') +fi --- End diff -- Are you sure this will work with systemd? On Ubuntu, the best it could do is to keep `JAVA_HOME=/usr/lib/jvm/jre` which is not valid on Ubuntu. Maybe, it would be easier to push that inside the ExecStart (and equivalent in init.d): ``` if [ -n "$JAVA_HOME" ]; then export JAVA_HOME=$(readlink -f /usr/bin/java | sed 's:/bin/java::') fi ``` Or, keeping it for RHEL in the default file but using in ExecStart: ``` [ -d "$JAVA_HOME" ] || export JAVA_HOME=$(readlink -f /usr/bin/java | sed 's:/bin/java::') ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1647: [lts] CLOUDSTACK-9462: Systemd support for Ub...
Github user vincentbernat commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1647#discussion_r76392345
--- Diff: packaging/systemd/cloudstack-agent.default ---
@@ -0,0 +1,24 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+JAVA_HOME=/usr/lib/jvm/jre
+if [ ! -f "$JAVA_HOME"/bin/java ] ; then
+JAVA_HOME=$(readlink -f /usr/bin/java | sed 's:/bin/java::')
+fi
--- End diff --
I think the result is quite fragile. systemd just reads the first
JAVA_HOME, then the second one, ignoring the condition. You could just have
(without the condition):
```
JAVA_HOME=$(readlink -f /usr/bin/java | sed 's:/bin/java::')
```
The only use of JAVA_HOME seems to be to choose a Java vesion. Why not
just, in `/etc/default/...`:
```
JAVA=/usr/bin/java
```
And in the service file:
```
ExecStart=/bin/sh -ec '\
export ACP=`ls /usr/share/cloudstack-agent/lib/*.jar
/usr/share/cloudstack-agent/plugins/*.jar 2>/dev/null|tr "\\n" ":"`; \
export
CLASSPATH="$ACP:/etc/cloudstack/agent:/usr/share/cloudstack-common/scripts"; \
${JAVA} -Xms${JAVA_HEAP_INITIAL} -Xmx${JAVA_HEAP_MAX} -cp "$CLASSPATH"
$JAVA_CLASS'
```
People could still overload the version of Java used by changing the JAVA
variable.
If you are uncomfortable with this, at least, just remove the condition and
simplify the default file to be:
```
JAVA_HOME=$(readlink -f /usr/bin/java | sed 's:/bin/java::')
JAVA_HEAP_INITIAL=256m
JAVA_HEAP_MAX=2048m
JAVA_CLASS=com.cloud.agent.AgentShell
```
From your tests, it works both on Ubuntu and CentOS.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
