Build and create tomcat, fix several other issues with management server #238
Conversation
|
cloudstack-pull-requests #122 SUCCESS |
|
Looks good, I see you've remove tomcat requirement as a package dependency. Is this embedding tomcat so we won't need to install on the distro? I can help test it next week. |
|
Yes, it completely removes the need to have tomcat installed in the distro, BTW Rohit, i noticed you cleaned up awsapi (i had to merge de spec files, RF On Fri, May 8, 2015 at 4:59 PM, Rohit Yadav [email protected]
|
|
Thanks @rsafonseca, it sounds great. While I think I had removed awsapi related tomcat stuff with commit 58999da it's possible I may have missed few spots, or a recent merge on master could have brought few things back. Can you also help fix the fedora related specs etc, in packaging/{fedora20, fedora21}/ to maintain consistency wrt not using distro provided tomcat? |
|
Yes, I'll take care of that.. i just haven't removed the old tomcat files On Fri, May 8, 2015 at 8:07 PM, Rohit Yadav [email protected]
|
|
Hi @bhaisaab |
|
cloudstack-pull-requests #128 SUCCESS |
|
cloudstack-pull-requests #129 SUCCESS |
|
cloudstack-pull-requests #131 SUCCESS |
|
@rsafonseca thanks for the updated PR, really like the last update to bring down the mgmt package size. Unfortunately I'm caught up with dayjob and since it's a big change I could only review/test/merge next week or a week after that, unless someone else could help out here. |
|
Hi @bhaisaab :) |
ui/index.jsp
Outdated
There was a problem hiding this comment.
IE (up to last version) have an option not to update content and I believe this is a workaround on that. So at least for IE it is probably still needed.
There was a problem hiding this comment.
lol that article is for IE 4 and 5 :)
The thing is, every time you refresh the page, the value of %now% changes, and it's like a whole new file, so the request is always triggered, and the tomcat doesn't fetch it from its own cache either.. because of different header :)
There was a problem hiding this comment.
The article is for IE 5, but when I have last seen an IE (I believe 11 on windows 8.1) it still had the same configurations and behavior.
There was a problem hiding this comment.
I know the behavior, I too would prefer to get rid of this and I am not personally interested in supporting IE users, but still there may be users who use ACS with IE. Could you please check if this is no longer needed?
There was a problem hiding this comment.
I've tested with no problems in IE11 and chrome.. don't see how changing this would affect specifically IE users :)
There was a problem hiding this comment.
I have no IE righ here, but the 'never' setting seems to be a problem. I would love to see it going away but if that creates bugreports or confusion for users then it is no fun. Would you agree in having that 'now' thing only for IE for safety?
There was a problem hiding this comment.
@rsafonseca I'll try to test your PR today by building at least the centos and debian packages. Meanwhile, should we have this, as without putting a ?t=timestamp; if someone is developing UI they will hit cache issues (or not?).
There was a problem hiding this comment.
There shouldn't be any cache issues using any of the current browsers (by current i mean since a few years back) with default settings.
If the browser is specifically configured by the user to don't look for updates, the user should just do a ctrl+f5. In normal scenario, browser will always ask for the files and tomcat will respond with HTTP 304 (not modified) unless it actually is modified.. so it shouldn't affect the UI developers.
There are tons of browser settings changes that would break the site completely, disabling static file updates lookup is far from being the worse setting a user can change from the defaults on any browser :)
We should not be hitting everyone's performance badly just to support a scenario where a user changes a browser setting that provokes this specific behavior (on any site).
Any way.. if it that was ever an issue... it would have not been fixed by placing the timestamp... since the timestamp is not on all .js files (just most of them) in index.jsp :)
|
Rafael Fonseca on [email protected] replies: |
|
Rafael, the gzip compression alone indeed does not make the tomcat serve it compressed
|
|
cloudstack-pull-requests #137 FAILURE |
|
Good catch k0kza, I hadn't noticed that StaticResourceServlet class, but i'll do some benchmarking on that vs tomcat 8's connector built in compression features, but it's still a whole lot faster than it was :) |
|
@rsafonseca sorry, the PR fails to cleanly apply on master. Can you rebase against master and send it again, along with any other fixes. Thanks. |
|
@bhaisaab rebased and sent. No new fixes since yesterday, but i rolled back the change of port 9090, to not leave anyone unhappy and make things easier on existing users :) |
|
cloudstack-pull-requests #149 FAILURE |
|
Getting this: I have openjdk installed, which always has worked fine. [root@jenkins packaging]# rpm -qa | grep java | grep devel |
packaging/package.sh
Outdated
There was a problem hiding this comment.
My previous comment is most likely related to this. If the last grep line doesn't match -ge 7, it'll fail
There was a problem hiding this comment.
Yes.. definitely, I didn't test jdk6 silly me... I'll fix that when I can
get my pc back up because I'm currently driving across Europe :-). Just
remove that check from package.sh to test the rest, just punched that in to
remove constraints for java 7 and 8 from the spec files, and still inform
the user if he is missing jdk
On May 13, 2015 11:52 PM, "Erik Weber" [email protected] wrote:
In packaging/package.sh
#238 (comment):@@ -48,6 +48,13 @@ function packaging() {
fiDISTRO=$3
- JDK=$(rpm -qa | grep "java-1...0-openjdk-devel")
- for version in
$JDK; do [ "$ {version:7:1}" -ge "7" ] && echo ${version:7:1}; done;- if [ "$?" -gt "0" ] || [ -z "$JDK" ] ; then
My previous comment is most likely related to this. If the last grep line
doesn't match -ge 7, it'll fail—
Reply to this email directly or view it on GitHub
https://github.com/apache/cloudstack/pull/238/files#r30280222.
|
Hey @rsafonseca, Thanks for improving ACS! ;) What is the status of this PR? I noticed that the last build failed, but it doesn't seem related to your changes (OVM3 failed). Could you just describe with more details what you tested? Did you execute any Marvin tests? I saw above that you tested with CentOS 6/7 and Ubuntu 12.04, but what exactly you tested? Thanks again. Cheers, |
|
cloudstack-pull-requests #165 FAILURE |
|
cloudstack-pull-requests #177 FAILURE |
|
cloudstack-pull-requests #178 FAILURE |
|
Hello @wilderrodrigues :) The OVM3 failure in jenkins is indeed unrelated to my changes, it seems it's failing for a few PRs whenever it gets built on specific jenkins servers... probably due do different jenkins configs. About the testing, i had only run the existing JUnit tests and did some manual testing on the mentioned platforms (service startup/config issues, tomcat performance/startup and java8 supportability for some things that were failing) and did not test the whole platform.. i still didn't have a look at Marvin, i hope i'll have time to start playing with it over the next week :) @bhaisaab all fixed! |
|
cloudstack-pull-requests #184 SUCCESS |
|
cloudstack-pull-requests #186 FAILURE |
|
cloudstack-pull-requests #187 SUCCESS |
|
cloudstack-pull-requests #222 SUCCESS |
|
cloudstack-pull-requests #223 SUCCESS |
|
looks like a couple more things got broken since last rebase with master... need to refix :| |
|
@rsafonseca Thanks for updating the PR. I think a rebase is needed. I see a lot of rc/init files removed esp from centos7, fedora20/21 directories? Also, have you build the packages and test them yourself? I'll be able to help test it by building packages later this week or next week. |
|
cloudstack-pull-requests #234 FAILURE |
|
cloudstack-pull-requests #233 SUCCESS |
There was a problem hiding this comment.
You may also want to get this into the libvirt refactor work you're doing.
This is a copy/paste from:
git/rsafonseca/cloudstack/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/wrapper/citrix/CitrixConsoleProxyLoadCommandWrapper.java
There was a problem hiding this comment.
Hi @rsafonseca
Will you remove the changes from you PR and I add to mine, or should we wait for your stuff to get merged?
A couple of question concerning this change:
- When you run the Unit Tests having this timeout, how do they perform?
- Before your changes get through, would you mind replacing the magic number by a static final variable with a clear description of the timeout? It could be added to the LibvirtUtilitiesHelper.
If the tests get somehow slower due to this, we can also add an accessor method to the timeout value. By doing so we can mock the call and when running the tests we can intercept it and return 0, instead of 5000.
Cheers,
Wilder
|
Hi @wilderrodrigues I'd rather not add any more fixes that are unrelated to the tomcat change into this PR :) |
|
Hi @rsafonseca Cool answer as the PR is hanging for way too long. :) I will apply the changes to a separate branch and create a PR. Cheers, |
|
Hi @rsafonseca, It's clear that this was quite some work to pull off, so first of all: thanks! To be honest, I'm not sure on how to test this properly and help you getting this through. If you can give some pointers on how we can test this, that would help. Otherwise maybe others can test it. The contents of the PR also changed a bit over time. Is the description still covering the changes? Great btw that some stuff ended up in separate PR (that got merged). More general, could you please squash your commits? There are many commits, lots of merge ones and also some that were more work-in-progress than anything useful. Regards, |
|
Hi @remibergsma As for testing this, there is no other way to test this other than manually since marvin doesn't even use tomcat for testing, altough the travis setup scripts download an old version of tomcat into the hard disk the tests do nothing with it :) That should also be cleaned up. If you guys would like, i can also alter the test scripts to use the same tomcat that will be used in the actual product instead of jetty, which will make for a test scenario closer to reality. This can be done in a way which will easily allow switching between tomcat and jetty in the future :) What do you think of that? BR, |
|
Hi @rsafonseca I got your PR to build the RPMs and do some manual tests on it. RPMs were created successfully! I then copied them into a new machine, brand new, and installed common/management RPMs there. When starting the Management server I got the following in the logs: 29-May-2015 03:38:41.413 WARNING [main] org.apache.catalina.startup.ClassLoaderFactory.validateFile Problem with directory [/usr/share/cloudstack-mysql-ha/lib/*jar], exists: [false], isDirectory: [false], canRead: [false] 29-May-2015 03:38:41.567 WARNING [main] org.apache.catalina.startup.Catalina.load Unable to load server configuration from [/usr/share/cloudstack-management/conf/server.xml] 29-May-2015 03:38:41.572 WARNING [main] org.apache.catalina.startup.Catalina.load Unable to load server configuration from [/usr/share/cloudstack-management/conf/server.xml] 29-May-2015 03:38:41.572 SEVERE [main] org.apache.catalina.startup.Catalina.start Cannot start server. Server instance is not configured. Errors are clear for me: /usr/share/cloudstack-management/conf/server.xml and /usr/share/cloudstack-mysql-ha/lib/*jar don't exist. Are those some new stuff? I already asked @remibergsma if he would know about it, but unfortunately he doesn't. Do you have any hints on those ones? :) Thanks a lot, dude! Cheers, |
|
Both the issues you hit are legacy behaviour :) I'm guessing that you've deployed it in debian, because the server.xml issue is handled properly by the RPM installers, but not by the debian one. To have it working after install on debian, just run the cloudstack-setup-management script, which i think is the standard way of setting things up, that's why I didn't bother to get the server.xml properly linked on package install in debian yet. I've been thinking about doing something better (at least that's my opinion, please drop a comment on this) which is setup the management server to ALWAYS run with both HTTP and HTTPS right after setup (using generated cert until user provides one) About that mysql-ha lib i guess it's one of the optionally packed libraries? i still haven done any testing on those. I just left this bit as it was. Happy testing :) RF |
|
Hi @rsafonseca I built the RPM on CentOS7: [root@cs5 ~]# uname -a [root@cs5 ~]# cat /etc/centos-release I will try executing the cloudstack-setup-management script on my centOS as well. Cheers, |
|
Indeed.. it's only setup on centos 6 and not 7, sorry for misleading you there :) The current version on master drops a server.xml in the folder (for Centos7 only), but it doesn't support SSL. I just restored the behaviour to be the same as in other OSs (you get a non-ssl and ssl version and link is handled by the setup scripts). Configuring SSL by default will remove the need to have a separate server-ssl.xml and server-nonssl.xml and will enable to just drop the server.xml config file in /etc/cloudstack/management . The behaviour should be consistent across distros Thanks for taking the time to test it out :) |
|
Hi @rsafonseca I executed the script then... lrwxrwxrwx. 1 root root 44 May 29 05:39 server.xml -> /etc/cloudstack/management/server-nonssl.xml [root@cs5 bin]# systemctl status cloudstack-management May 29 05:39:16 cs5 systemd[1]: Starting CloudStack Management Server... :) I just have to configure the DB to see how it will behave. Perhaps if you create a new PR, with the packaging stuff only, it would be easier to test and get it merged onto master. The steps to test the packaging are quite straight forward and we are glad to help on that. Cheers, |
|
cloudstack-pull-requests #273 FAILURE |
rsafonseca
force-pushed
the
fixpackaging
branch
2 times, most recently
from
0362358 to
1df5284
Compare
Reduced cloudstack-management_4.6.0-snapshot_all.deb size from 211Mb to 94Mb (CentOS is 92Mb) This deb file was packing two copies of the systemvm (which is alread packed in cloudstack-common), so removed like in CentOS RPM specs Fedora 21 support for management :) Removed Fedora specific branches (symlink fedora2x points to centos7 so, using same installer :) Small cleanup of duplicate files
Fixed Maven missing output message, was not interpreting \n
… to timeout Added timeout to URLConnection in LibvirtConsoleProxyLoadCommandWrapper to speed up failed state detection Added surefire options to maven to greatly speed up tests (especially on VMs). Some tests were idle for many minutes before completing. Fix another apidocs build issue, jenkins should be happy now :) Moved tomcat version control to parent pom, fixed minor maven warning about module version Fix master was not building centos63 Fix master was not building deb Cleanups Remove tomcat dependency for debian Fix apidocs tests
|
cloudstack-pull-requests #274 FAILURE |
|
cloudstack-pull-requests #275 FAILURE |
|
cloudstack-pull-requests #276 FAILURE |
|
This has been stale for a while, here's something to spice it up :) If all in agreement will close this one and we'll go for the new one ;) |
|
@rsafonseca hey, looks like the new #372 one some interesting things (such as embedded jetty); do you want to or have already take the good stuff from this PR to 372? If you think the new one is the best way forward, sure close this one. I'll have some time later this week to help you test packaging related stuff. |
|
Hi @bhaisaab :) |
7 participants
Hi guys,
This PR has the following goodies:
Some advantages of packing tomcat (while not getting rid of it completely ;) ):