You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Alena Prokharchyk <Al...@citrix.com> on 2014/07/02 22:29:41 UTC
CLOUDSTACK-6242 - the fix needs to be reverted temporarily
Daan,
We have to temporarily revert the commit (looks like it exists in master branch only)
commit c031eb7d38200d680da85ef57367b21df3483c41
Author: Ding Yuan <yu...@eecg> 2014-04-14 14:02:03
Committer: Daan Hoogland <da...@onecht.net> 2014-04-14 23:07:15
CLOUDSTACK-6242: exception handling improvements
Or at least revert the changes done by the commit in ConfigurationServerImpl – the fix enabled mysql exception logging. It causes following bugs on the RPM setup:
https://issues.apache.org/jira/browse/CLOUDSTACK-7039
https://issues.apache.org/jira/browse/CLOUDSTACK-7040
https://issues.apache.org/jira/browse/CLOUDSTACK-7041
https://issues.apache.org/jira/browse/CLOUDSTACK-7042
Why the fix has to be reverted:
ConfigurationServerImpl.SaveUser method is responsible for saving System and Admin default users. It runs on initial management server startup. The reason why exception have been swallowed before is – if you do install from RPM, the system users are inserted by some system script (can’t recall the name) invoked during the installation. This script is NOT executed when do dev build. Then upon the management server startup ConfigurationServerImpl tries to insert the same users again. It passes for the dev environment – as the users weren’t present yet – but fails for the QA. And the failure – mysqlexception – gets swallowed.
The correct fix would be:
1. Fix install scripts the way all default info – system users, default security groups, etc – are being inserted in the same manner for Dev and RPM (QA use/test this one) builds.
2. Re-enable the exception logging now.
And in the future, whoever does the changes to the exception logging/handling, please do more research on why things were done certain way to avoid such pitfalls. I’m not saying that not logging the exception is right, but just enabling them when they were previously disabled, or changing the exception logic, without investigation why it was done this way, can affect the rest of the code behavior. The fix would most likely involve much more than just changing the exception handling logic.
-Alena.
Re: CLOUDSTACK-6242 - the fix needs to be reverted temporarily
Posted by Alena Prokharchyk <Al...@citrix.com>.
Sure.
On 7/7/14, 2:01 PM, "Daan Hoogland" <da...@gmail.com> wrote:
>Alena, I had no time at all to look at cloudstack today, could you
>apply it please. My $dayjob duties are requiring a slightly different
>focus the coming week(s)
>
>thanks
>
>On Mon, Jul 7, 2014 at 6:42 PM, Alena Prokharchyk
><Al...@citrix.com> wrote:
>> Yes, that¹s what I mean - by now, we should log them, but without
>>logging
>> the trace. So your fix is correct.
>>
>> Later, we should fix the installation process so the user is inserted
>>only
>> once; and change the logic in ConfigurationServer - instead of logging
>>the
>> exception, we should throw the RuntimeException indicating that
>>essential
>> CS configuration failed, and that exception should fail MS startup. I
>>will
>> file a bug for that after you submit your fix.
>>
>> Thank you,
>> Alena.
>>
>> On 7/3/14, 1:38 PM, "Daan Hoogland" <da...@gmail.com> wrote:
>>
>>>Alena, I really want to fix issues in this line, because I really want
>>>us to use exceptions properly and never ignore them. So I would like
>>>handle them or log at least. Thanks for your patients.
>>>
>>>I am not sure of what you mean. Is this close:
>>>
>>>diff --git a/server/src/com/cloud/server/ConfigurationServerImpl.java
>>>b/server/src/com/cloud/server/ConfigurationServerImpl.java
>>>index b66e52d..a166372 100755
>>>--- a/server/src/com/cloud/server/ConfigurationServerImpl.java
>>>+++ b/server/src/com/cloud/server/ConfigurationServerImpl.java
>>>@@ -462,21 +462,21 @@
>>>
>>> try {
>>> PreparedStatement stmt =
>>>txn.prepareAutoCloseStatement(insertSql);
>>> stmt.executeUpdate();
>>> } catch (SQLException ex) {
>>>- s_logger.debug("Caught SQLException when
>>>inserting system account ", ex);
>>>+ s_logger.debug("Caught SQLException when
>>>inserting system account: " + ex.getLocalizedMessage());
>>> }
>>> // insert system user
>>> insertSql = "INSERT INTO `cloud`.`user` (id, uuid,
>>>username, password, account_id, firstname, lastname, created,
>>>user.default)"
>>> + " VALUES (1, UUID(), 'system', RAND(), 1,
>>>'system', 'cloud', now(), 1)";
>>>
>>> try {
>>> PreparedStatement stmt =
>>>txn.prepareAutoCloseStatement(insertSql);
>>> stmt.executeUpdate();
>>> } catch (SQLException ex) {
>>>- s_logger.debug("Caught SQLException when
>>>inserting system user ", ex);
>>>+ s_logger.debug("Caught SQLException when
>>>inserting system user: " + ex.getLocalizedMessage());
>>> }
>>>
>>> // insert admin user, but leave the account disabled
>>>until we set a
>>> // password with the user authenticator
>>> long id = 2;
>>>@@ -489,22 +489,22 @@
>>> + "', '1', '1', 1)";
>>> try {
>>> PreparedStatement stmt =
>>>txn.prepareAutoCloseStatement(insertSql);
>>> stmt.executeUpdate();
>>> } catch (SQLException ex) {
>>>- s_logger.debug("Caught SQLException when creating
>>>admin account ", ex);
>>>+ s_logger.debug("Caught SQLException when creating
>>>admin account: " + ex.getLocalizedMessage());
>>> }
>>>
>>> // now insert the user
>>> insertSql = "INSERT INTO `cloud`.`user` (id, uuid,
>>>username, password, account_id, firstname, lastname, created, state,
>>>user.default) " + "VALUES (" + id
>>> + ", UUID(), '" + username + "', RAND(), 2,
>>>'" + firstname + "','" + lastname + "',now(), 'disabled', 1)";
>>>
>>> try {
>>> PreparedStatement stmt =
>>>txn.prepareAutoCloseStatement(insertSql);
>>> stmt.executeUpdate();
>>> } catch (SQLException ex) {
>>>- s_logger.debug("Caught SQLException when
>>>inserting user ", ex);
>>>+ s_logger.debug("Caught SQLException when
>>>inserting user " + ex.getLocalizedMessage());
>>> }
>>>
>>> try {
>>> String tableName = "security_group";
>>> try {
>>>
>>>On Thu, Jul 3, 2014 at 10:23 PM, Alena Prokharchyk
>>><Al...@citrix.com> wrote:
>>>> Daan, there are similar problem in saveaccount/saveuser methods in thr
>>>>same class - introduced by this commit as well. I can fix it myself on
>>>>Monday (Its holiday days today and tomorrow at Citrix, usa); or you can
>>>>revert them as well along with the fix you do for network groups.
>>>>
>>>> Let me know, and thank you for the follow up.
>>>>
>>>> -alena
>>>>
>>>>> On Jul 2, 2014, at 11:43 PM, "Daan Hoogland"
>>>>><da...@gmail.com>
>>>>>wrote:
>>>>>
>>>>> On Thu, Jul 3, 2014 at 12:27 AM, Alena Prokharchyk
>>>>> <Al...@citrix.com> wrote:
>>>>>> In any case, fixes done to ConfigurationManagerImpl are not correct,
>>>>>>and
>>>>>> logging should be fixed by reverting/reapplying the commit by
>>>>>>following
>>>>>> the rules defined in a) or b).
>>>>>
>>>>>
>>>>> changing to
>>>>> } catch (Exception ex) {
>>>>> // if network_groups table exists, create the
>>>>> default security group there
>>>>> s_logger.debug("Caught (SQL?)Exception: no
>>>>> network_group " + ex.getLocalizedMessage());
>>>>> }
>>>>> for now
>>>>>
>>>>> --
>>>>> Daan
>>>
>>>
>>>
>>>--
>>>Daan
>>
>
>
>
>--
>Daan
Re: CLOUDSTACK-6242 - the fix needs to be reverted temporarily
Posted by Daan Hoogland <da...@gmail.com>.
Alena, I had no time at all to look at cloudstack today, could you
apply it please. My $dayjob duties are requiring a slightly different
focus the coming week(s)
thanks
On Mon, Jul 7, 2014 at 6:42 PM, Alena Prokharchyk
<Al...@citrix.com> wrote:
> Yes, that¹s what I mean - by now, we should log them, but without logging
> the trace. So your fix is correct.
>
> Later, we should fix the installation process so the user is inserted only
> once; and change the logic in ConfigurationServer - instead of logging the
> exception, we should throw the RuntimeException indicating that essential
> CS configuration failed, and that exception should fail MS startup. I will
> file a bug for that after you submit your fix.
>
> Thank you,
> Alena.
>
> On 7/3/14, 1:38 PM, "Daan Hoogland" <da...@gmail.com> wrote:
>
>>Alena, I really want to fix issues in this line, because I really want
>>us to use exceptions properly and never ignore them. So I would like
>>handle them or log at least. Thanks for your patients.
>>
>>I am not sure of what you mean. Is this close:
>>
>>diff --git a/server/src/com/cloud/server/ConfigurationServerImpl.java
>>b/server/src/com/cloud/server/ConfigurationServerImpl.java
>>index b66e52d..a166372 100755
>>--- a/server/src/com/cloud/server/ConfigurationServerImpl.java
>>+++ b/server/src/com/cloud/server/ConfigurationServerImpl.java
>>@@ -462,21 +462,21 @@
>>
>> try {
>> PreparedStatement stmt =
>>txn.prepareAutoCloseStatement(insertSql);
>> stmt.executeUpdate();
>> } catch (SQLException ex) {
>>- s_logger.debug("Caught SQLException when
>>inserting system account ", ex);
>>+ s_logger.debug("Caught SQLException when
>>inserting system account: " + ex.getLocalizedMessage());
>> }
>> // insert system user
>> insertSql = "INSERT INTO `cloud`.`user` (id, uuid,
>>username, password, account_id, firstname, lastname, created,
>>user.default)"
>> + " VALUES (1, UUID(), 'system', RAND(), 1,
>>'system', 'cloud', now(), 1)";
>>
>> try {
>> PreparedStatement stmt =
>>txn.prepareAutoCloseStatement(insertSql);
>> stmt.executeUpdate();
>> } catch (SQLException ex) {
>>- s_logger.debug("Caught SQLException when
>>inserting system user ", ex);
>>+ s_logger.debug("Caught SQLException when
>>inserting system user: " + ex.getLocalizedMessage());
>> }
>>
>> // insert admin user, but leave the account disabled
>>until we set a
>> // password with the user authenticator
>> long id = 2;
>>@@ -489,22 +489,22 @@
>> + "', '1', '1', 1)";
>> try {
>> PreparedStatement stmt =
>>txn.prepareAutoCloseStatement(insertSql);
>> stmt.executeUpdate();
>> } catch (SQLException ex) {
>>- s_logger.debug("Caught SQLException when creating
>>admin account ", ex);
>>+ s_logger.debug("Caught SQLException when creating
>>admin account: " + ex.getLocalizedMessage());
>> }
>>
>> // now insert the user
>> insertSql = "INSERT INTO `cloud`.`user` (id, uuid,
>>username, password, account_id, firstname, lastname, created, state,
>>user.default) " + "VALUES (" + id
>> + ", UUID(), '" + username + "', RAND(), 2,
>>'" + firstname + "','" + lastname + "',now(), 'disabled', 1)";
>>
>> try {
>> PreparedStatement stmt =
>>txn.prepareAutoCloseStatement(insertSql);
>> stmt.executeUpdate();
>> } catch (SQLException ex) {
>>- s_logger.debug("Caught SQLException when
>>inserting user ", ex);
>>+ s_logger.debug("Caught SQLException when
>>inserting user " + ex.getLocalizedMessage());
>> }
>>
>> try {
>> String tableName = "security_group";
>> try {
>>
>>On Thu, Jul 3, 2014 at 10:23 PM, Alena Prokharchyk
>><Al...@citrix.com> wrote:
>>> Daan, there are similar problem in saveaccount/saveuser methods in thr
>>>same class - introduced by this commit as well. I can fix it myself on
>>>Monday (Its holiday days today and tomorrow at Citrix, usa); or you can
>>>revert them as well along with the fix you do for network groups.
>>>
>>> Let me know, and thank you for the follow up.
>>>
>>> -alena
>>>
>>>> On Jul 2, 2014, at 11:43 PM, "Daan Hoogland" <da...@gmail.com>
>>>>wrote:
>>>>
>>>> On Thu, Jul 3, 2014 at 12:27 AM, Alena Prokharchyk
>>>> <Al...@citrix.com> wrote:
>>>>> In any case, fixes done to ConfigurationManagerImpl are not correct,
>>>>>and
>>>>> logging should be fixed by reverting/reapplying the commit by
>>>>>following
>>>>> the rules defined in a) or b).
>>>>
>>>>
>>>> changing to
>>>> } catch (Exception ex) {
>>>> // if network_groups table exists, create the
>>>> default security group there
>>>> s_logger.debug("Caught (SQL?)Exception: no
>>>> network_group " + ex.getLocalizedMessage());
>>>> }
>>>> for now
>>>>
>>>> --
>>>> Daan
>>
>>
>>
>>--
>>Daan
>
--
Daan
Re: CLOUDSTACK-6242 - the fix needs to be reverted temporarily
Posted by Alena Prokharchyk <Al...@citrix.com>.
Yes, that¹s what I mean - by now, we should log them, but without logging
the trace. So your fix is correct.
Later, we should fix the installation process so the user is inserted only
once; and change the logic in ConfigurationServer - instead of logging the
exception, we should throw the RuntimeException indicating that essential
CS configuration failed, and that exception should fail MS startup. I will
file a bug for that after you submit your fix.
Thank you,
Alena.
On 7/3/14, 1:38 PM, "Daan Hoogland" <da...@gmail.com> wrote:
>Alena, I really want to fix issues in this line, because I really want
>us to use exceptions properly and never ignore them. So I would like
>handle them or log at least. Thanks for your patients.
>
>I am not sure of what you mean. Is this close:
>
>diff --git a/server/src/com/cloud/server/ConfigurationServerImpl.java
>b/server/src/com/cloud/server/ConfigurationServerImpl.java
>index b66e52d..a166372 100755
>--- a/server/src/com/cloud/server/ConfigurationServerImpl.java
>+++ b/server/src/com/cloud/server/ConfigurationServerImpl.java
>@@ -462,21 +462,21 @@
>
> try {
> PreparedStatement stmt =
>txn.prepareAutoCloseStatement(insertSql);
> stmt.executeUpdate();
> } catch (SQLException ex) {
>- s_logger.debug("Caught SQLException when
>inserting system account ", ex);
>+ s_logger.debug("Caught SQLException when
>inserting system account: " + ex.getLocalizedMessage());
> }
> // insert system user
> insertSql = "INSERT INTO `cloud`.`user` (id, uuid,
>username, password, account_id, firstname, lastname, created,
>user.default)"
> + " VALUES (1, UUID(), 'system', RAND(), 1,
>'system', 'cloud', now(), 1)";
>
> try {
> PreparedStatement stmt =
>txn.prepareAutoCloseStatement(insertSql);
> stmt.executeUpdate();
> } catch (SQLException ex) {
>- s_logger.debug("Caught SQLException when
>inserting system user ", ex);
>+ s_logger.debug("Caught SQLException when
>inserting system user: " + ex.getLocalizedMessage());
> }
>
> // insert admin user, but leave the account disabled
>until we set a
> // password with the user authenticator
> long id = 2;
>@@ -489,22 +489,22 @@
> + "', '1', '1', 1)";
> try {
> PreparedStatement stmt =
>txn.prepareAutoCloseStatement(insertSql);
> stmt.executeUpdate();
> } catch (SQLException ex) {
>- s_logger.debug("Caught SQLException when creating
>admin account ", ex);
>+ s_logger.debug("Caught SQLException when creating
>admin account: " + ex.getLocalizedMessage());
> }
>
> // now insert the user
> insertSql = "INSERT INTO `cloud`.`user` (id, uuid,
>username, password, account_id, firstname, lastname, created, state,
>user.default) " + "VALUES (" + id
> + ", UUID(), '" + username + "', RAND(), 2,
>'" + firstname + "','" + lastname + "',now(), 'disabled', 1)";
>
> try {
> PreparedStatement stmt =
>txn.prepareAutoCloseStatement(insertSql);
> stmt.executeUpdate();
> } catch (SQLException ex) {
>- s_logger.debug("Caught SQLException when
>inserting user ", ex);
>+ s_logger.debug("Caught SQLException when
>inserting user " + ex.getLocalizedMessage());
> }
>
> try {
> String tableName = "security_group";
> try {
>
>On Thu, Jul 3, 2014 at 10:23 PM, Alena Prokharchyk
><Al...@citrix.com> wrote:
>> Daan, there are similar problem in saveaccount/saveuser methods in thr
>>same class - introduced by this commit as well. I can fix it myself on
>>Monday (Its holiday days today and tomorrow at Citrix, usa); or you can
>>revert them as well along with the fix you do for network groups.
>>
>> Let me know, and thank you for the follow up.
>>
>> -alena
>>
>>> On Jul 2, 2014, at 11:43 PM, "Daan Hoogland" <da...@gmail.com>
>>>wrote:
>>>
>>> On Thu, Jul 3, 2014 at 12:27 AM, Alena Prokharchyk
>>> <Al...@citrix.com> wrote:
>>>> In any case, fixes done to ConfigurationManagerImpl are not correct,
>>>>and
>>>> logging should be fixed by reverting/reapplying the commit by
>>>>following
>>>> the rules defined in a) or b).
>>>
>>>
>>> changing to
>>> } catch (Exception ex) {
>>> // if network_groups table exists, create the
>>> default security group there
>>> s_logger.debug("Caught (SQL?)Exception: no
>>> network_group " + ex.getLocalizedMessage());
>>> }
>>> for now
>>>
>>> --
>>> Daan
>
>
>
>--
>Daan
Re: CLOUDSTACK-6242 - the fix needs to be reverted temporarily
Posted by Daan Hoogland <da...@gmail.com>.
Alena, I really want to fix issues in this line, because I really want
us to use exceptions properly and never ignore them. So I would like
handle them or log at least. Thanks for your patients.
I am not sure of what you mean. Is this close:
diff --git a/server/src/com/cloud/server/ConfigurationServerImpl.java
b/server/src/com/cloud/server/ConfigurationServerImpl.java
index b66e52d..a166372 100755
--- a/server/src/com/cloud/server/ConfigurationServerImpl.java
+++ b/server/src/com/cloud/server/ConfigurationServerImpl.java
@@ -462,21 +462,21 @@
try {
PreparedStatement stmt =
txn.prepareAutoCloseStatement(insertSql);
stmt.executeUpdate();
} catch (SQLException ex) {
- s_logger.debug("Caught SQLException when
inserting system account ", ex);
+ s_logger.debug("Caught SQLException when
inserting system account: " + ex.getLocalizedMessage());
}
// insert system user
insertSql = "INSERT INTO `cloud`.`user` (id, uuid,
username, password, account_id, firstname, lastname, created,
user.default)"
+ " VALUES (1, UUID(), 'system', RAND(), 1,
'system', 'cloud', now(), 1)";
try {
PreparedStatement stmt =
txn.prepareAutoCloseStatement(insertSql);
stmt.executeUpdate();
} catch (SQLException ex) {
- s_logger.debug("Caught SQLException when
inserting system user ", ex);
+ s_logger.debug("Caught SQLException when
inserting system user: " + ex.getLocalizedMessage());
}
// insert admin user, but leave the account disabled
until we set a
// password with the user authenticator
long id = 2;
@@ -489,22 +489,22 @@
+ "', '1', '1', 1)";
try {
PreparedStatement stmt =
txn.prepareAutoCloseStatement(insertSql);
stmt.executeUpdate();
} catch (SQLException ex) {
- s_logger.debug("Caught SQLException when creating
admin account ", ex);
+ s_logger.debug("Caught SQLException when creating
admin account: " + ex.getLocalizedMessage());
}
// now insert the user
insertSql = "INSERT INTO `cloud`.`user` (id, uuid,
username, password, account_id, firstname, lastname, created, state,
user.default) " + "VALUES (" + id
+ ", UUID(), '" + username + "', RAND(), 2,
'" + firstname + "','" + lastname + "',now(), 'disabled', 1)";
try {
PreparedStatement stmt =
txn.prepareAutoCloseStatement(insertSql);
stmt.executeUpdate();
} catch (SQLException ex) {
- s_logger.debug("Caught SQLException when
inserting user ", ex);
+ s_logger.debug("Caught SQLException when
inserting user " + ex.getLocalizedMessage());
}
try {
String tableName = "security_group";
try {
On Thu, Jul 3, 2014 at 10:23 PM, Alena Prokharchyk
<Al...@citrix.com> wrote:
> Daan, there are similar problem in saveaccount/saveuser methods in thr same class - introduced by this commit as well. I can fix it myself on Monday (Its holiday days today and tomorrow at Citrix, usa); or you can revert them as well along with the fix you do for network groups.
>
> Let me know, and thank you for the follow up.
>
> -alena
>
>> On Jul 2, 2014, at 11:43 PM, "Daan Hoogland" <da...@gmail.com> wrote:
>>
>> On Thu, Jul 3, 2014 at 12:27 AM, Alena Prokharchyk
>> <Al...@citrix.com> wrote:
>>> In any case, fixes done to ConfigurationManagerImpl are not correct, and
>>> logging should be fixed by reverting/reapplying the commit by following
>>> the rules defined in a) or b).
>>
>>
>> changing to
>> } catch (Exception ex) {
>> // if network_groups table exists, create the
>> default security group there
>> s_logger.debug("Caught (SQL?)Exception: no
>> network_group " + ex.getLocalizedMessage());
>> }
>> for now
>>
>> --
>> Daan
--
Daan
Re: CLOUDSTACK-6242 - the fix needs to be reverted temporarily
Posted by Alena Prokharchyk <Al...@citrix.com>.
Daan, there are similar problem in saveaccount/saveuser methods in thr same class - introduced by this commit as well. I can fix it myself on Monday (Its holiday days today and tomorrow at Citrix, usa); or you can revert them as well along with the fix you do for network groups.
Let me know, and thank you for the follow up.
-alena
> On Jul 2, 2014, at 11:43 PM, "Daan Hoogland" <da...@gmail.com> wrote:
>
> On Thu, Jul 3, 2014 at 12:27 AM, Alena Prokharchyk
> <Al...@citrix.com> wrote:
>> In any case, fixes done to ConfigurationManagerImpl are not correct, and
>> logging should be fixed by reverting/reapplying the commit by following
>> the rules defined in a) or b).
>
>
> changing to
> } catch (Exception ex) {
> // if network_groups table exists, create the
> default security group there
> s_logger.debug("Caught (SQL?)Exception: no
> network_group " + ex.getLocalizedMessage());
> }
> for now
>
> --
> Daan
Re: CLOUDSTACK-6242 - the fix needs to be reverted temporarily
Posted by Daan Hoogland <da...@gmail.com>.
On Thu, Jul 3, 2014 at 12:27 AM, Alena Prokharchyk
<Al...@citrix.com> wrote:
> In any case, fixes done to ConfigurationManagerImpl are not correct, and
> logging should be fixed by reverting/reapplying the commit by following
> the rules defined in a) or b).
changing to
} catch (Exception ex) {
// if network_groups table exists, create the
default security group there
s_logger.debug("Caught (SQL?)Exception: no
network_group " + ex.getLocalizedMessage());
}
for now
--
Daan
Re: CLOUDSTACK-6242 - the fix needs to be reverted temporarily
Posted by Alena Prokharchyk <Al...@citrix.com>.
Also SQLException is a checked exception meaning that the CS code knows
how to handle it:
} catch (SQLException ex) {
// if network_groups table exists, create the
default security group there
+ s_logger.debug("Caught SQLException: no
network_group ", ex);
If CS code prefers to ignore it - there is no action in the catch() clause
- then logging the stack trace is not correct. We should either:
a) log it as a one liner error indicating what went wrong if you can
recover from the situation. Just like its done in ResourceManagerImpl by
the same commit for NumberFormatException:
long dcId = -1;
DataCenterVO dc = _dcDao.findByName(dataCenter);
if (dc == null) {
try {
dcId = Long.parseLong(dataCenter);
dc = _dcDao.findById(dcId);
} catch (final NumberFormatException e) {
s_logger.debug("Cannot parse " + dataCenter + " into
Long.”); // logging that was added
}
}
if (dc == null) {
throw new IllegalArgumentException("Host " +
startup.getPrivateIpAddress() + " sent incorrect data center: " +
dataCenter);
}
Or
B) throw CloudStackRuntimeException because inability to save the
system/admin user should block the MS startup as initial login won’t be
possible without the default user saved.
I think b) should be preferable. But b) should come with the changes for
the install script for RPM build.
In any case, fixes done to ConfigurationManagerImpl are not correct, and
logging should be fixed by reverting/reapplying the commit by following
the rules defined in a) or b).
Thank you,
Alena.
On 7/2/14, 2:03 PM, "Daan Hoogland" <da...@gmail.com> wrote:
>I have no idea, the scripts in the scripts/installer dir where written
>by Manuel Amador before moving to apache.
>
>On Wed, Jul 2, 2014 at 10:54 PM, Alena Prokharchyk
><Al...@citrix.com> wrote:
>> Its ok to log as long as the original problem - with the install script
>>on
>> RPM setup - is fixed along. I would prefer it to be fixed in a single
>> commit, otherwise the QA will continue seeing this bug and bringing it
>> over and over again. The fix for logging shouldn¹t go to the 4.5 release
>> w/o the original problem fixed, as in this case customers will see the
>> exceptions as well.
>>
>> That¹s why I said ³temporarily².
>>
>> Daan, do you know who might be familiar with installation scripts area?
>>If
>> so, could you CC this person to bring the bug to their attention?
>>
>> Thank you,
>> Alena.
>>
>> On 7/2/14, 1:46 PM, "Daan Hoogland" <da...@gmail.com> wrote:
>>
>>>On Wed, Jul 2, 2014 at 10:29 PM, Alena Prokharchyk
>>><Al...@citrix.com> wrote:
>>>> c031eb7d38200d680da85ef57367b21df3483c41
>>>
>>>
>>>So please amend and not revert? I suppose you are talking about
>>>@@ -508,8 +512,9 @@ public class ConfigurationServerImpl extends
>>>ManagerBase implements Configuratio
>>> PreparedStatement stmt =
>>>txn.prepareAutoCloseStatement(checkSql);
>>> stmt.executeQuery();
>>> tableName = "network_group";
>>>- } catch (Exception ex) {
>>>+ } catch (SQLException ex) {
>>> // if network_groups table exists, create the
>>>default security group there
>>>+ s_logger.debug("Caught SQLException: no
>>>network_group ", ex);
>>> }
>>>
>>> insertSql = "SELECT * FROM " + tableName + "
>>>where account_id=2 and name='default'";
>>>
>>>Let's catch and log a generic exception. This is independent of
>>>changing the script. All those other changes are improvements and in
>>>fact this one is as well, no one would have looked at this hidden
>>>feature of the dev env without it. Ignoring exceptions is bad practice
>>>precisely for this reason.
>>>
>>>--
>>>Daan
>>
>
>
>
>--
>Daan
Re: CLOUDSTACK-6242 - the fix needs to be reverted temporarily
Posted by Daan Hoogland <da...@gmail.com>.
I have no idea, the scripts in the scripts/installer dir where written
by Manuel Amador before moving to apache.
On Wed, Jul 2, 2014 at 10:54 PM, Alena Prokharchyk
<Al...@citrix.com> wrote:
> Its ok to log as long as the original problem - with the install script on
> RPM setup - is fixed along. I would prefer it to be fixed in a single
> commit, otherwise the QA will continue seeing this bug and bringing it
> over and over again. The fix for logging shouldn¹t go to the 4.5 release
> w/o the original problem fixed, as in this case customers will see the
> exceptions as well.
>
> That¹s why I said ³temporarily².
>
> Daan, do you know who might be familiar with installation scripts area? If
> so, could you CC this person to bring the bug to their attention?
>
> Thank you,
> Alena.
>
> On 7/2/14, 1:46 PM, "Daan Hoogland" <da...@gmail.com> wrote:
>
>>On Wed, Jul 2, 2014 at 10:29 PM, Alena Prokharchyk
>><Al...@citrix.com> wrote:
>>> c031eb7d38200d680da85ef57367b21df3483c41
>>
>>
>>So please amend and not revert? I suppose you are talking about
>>@@ -508,8 +512,9 @@ public class ConfigurationServerImpl extends
>>ManagerBase implements Configuratio
>> PreparedStatement stmt =
>>txn.prepareAutoCloseStatement(checkSql);
>> stmt.executeQuery();
>> tableName = "network_group";
>>- } catch (Exception ex) {
>>+ } catch (SQLException ex) {
>> // if network_groups table exists, create the
>>default security group there
>>+ s_logger.debug("Caught SQLException: no
>>network_group ", ex);
>> }
>>
>> insertSql = "SELECT * FROM " + tableName + "
>>where account_id=2 and name='default'";
>>
>>Let's catch and log a generic exception. This is independent of
>>changing the script. All those other changes are improvements and in
>>fact this one is as well, no one would have looked at this hidden
>>feature of the dev env without it. Ignoring exceptions is bad practice
>>precisely for this reason.
>>
>>--
>>Daan
>
--
Daan
Re: CLOUDSTACK-6242 - the fix needs to be reverted temporarily
Posted by Alena Prokharchyk <Al...@citrix.com>.
Its ok to log as long as the original problem - with the install script on
RPM setup - is fixed along. I would prefer it to be fixed in a single
commit, otherwise the QA will continue seeing this bug and bringing it
over and over again. The fix for logging shouldn¹t go to the 4.5 release
w/o the original problem fixed, as in this case customers will see the
exceptions as well.
That¹s why I said ³temporarily².
Daan, do you know who might be familiar with installation scripts area? If
so, could you CC this person to bring the bug to their attention?
Thank you,
Alena.
On 7/2/14, 1:46 PM, "Daan Hoogland" <da...@gmail.com> wrote:
>On Wed, Jul 2, 2014 at 10:29 PM, Alena Prokharchyk
><Al...@citrix.com> wrote:
>> c031eb7d38200d680da85ef57367b21df3483c41
>
>
>So please amend and not revert? I suppose you are talking about
>@@ -508,8 +512,9 @@ public class ConfigurationServerImpl extends
>ManagerBase implements Configuratio
> PreparedStatement stmt =
>txn.prepareAutoCloseStatement(checkSql);
> stmt.executeQuery();
> tableName = "network_group";
>- } catch (Exception ex) {
>+ } catch (SQLException ex) {
> // if network_groups table exists, create the
>default security group there
>+ s_logger.debug("Caught SQLException: no
>network_group ", ex);
> }
>
> insertSql = "SELECT * FROM " + tableName + "
>where account_id=2 and name='default'";
>
>Let's catch and log a generic exception. This is independent of
>changing the script. All those other changes are improvements and in
>fact this one is as well, no one would have looked at this hidden
>feature of the dev env without it. Ignoring exceptions is bad practice
>precisely for this reason.
>
>--
>Daan
Re: CLOUDSTACK-6242 - the fix needs to be reverted temporarily
Posted by Daan Hoogland <da...@gmail.com>.
On Wed, Jul 2, 2014 at 10:29 PM, Alena Prokharchyk
<Al...@citrix.com> wrote:
> c031eb7d38200d680da85ef57367b21df3483c41
So please amend and not revert? I suppose you are talking about
@@ -508,8 +512,9 @@ public class ConfigurationServerImpl extends
ManagerBase implements Configuratio
PreparedStatement stmt =
txn.prepareAutoCloseStatement(checkSql);
stmt.executeQuery();
tableName = "network_group";
- } catch (Exception ex) {
+ } catch (SQLException ex) {
// if network_groups table exists, create the
default security group there
+ s_logger.debug("Caught SQLException: no
network_group ", ex);
}
insertSql = "SELECT * FROM " + tableName + "
where account_id=2 and name='default'";
Let's catch and log a generic exception. This is independent of
changing the script. All those other changes are improvements and in
fact this one is as well, no one would have looked at this hidden
feature of the dev env without it. Ignoring exceptions is bad practice
precisely for this reason.
--
Daan