Skip to content

Conversation

@yadvr
Copy link
Member

@yadvr yadvr commented May 4, 2020

This would give users ability to change the timezone

The general assumption is that CloudStack infra/services would run on UTC with ntp/synchronised time. Looks like some users prefer local time in DB.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

yadvr added 2 commits May 4, 2020 14:57
This would give users ability to change the timezone

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
@Doni7722
Copy link

Doni7722 commented May 4, 2020

Hi Rohit

I could check the fix in my lab environment (CloudStack 4.14 RC1, CentOS 7 OS) and can confirm that it fixed the issue, where cloudstack-management service is not able to start as timezone informations are missing.

If users have the usage service too they will need to implement the parameters for the usage DB as well. Otherwise they won't be able to start the cloudstack-usage service.
"db.usage.url.params=" in the /etc/cloudstack/management/db.properties file would be the one for the usage service

Regards

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
@yadvr
Copy link
Member Author

yadvr commented May 4, 2020

Hi @Doni7722 I've added the usage url params as well, thanks.

@Doni7722
Copy link

Doni7722 commented May 4, 2020

Hi @rhtyd there is a small typo. you changed the url params to:
db.usage.url.params=serverTimezone=UTC
but I think it should be:
db.usage.url.params=&serverTimezone=UTC

@yadvr
Copy link
Member Author

yadvr commented May 4, 2020

@Doni7722 the db.cloud.url.params= don't begin with an & - have you tested the value without a & in your env, if it doesn't work for you I'm happy to change per your comment.

@Doni7722
Copy link

Doni7722 commented May 4, 2020

@rhtyd yes works with & and without (tested both now) so I think it's better without & as you wrote the begining should be without.

@yadvr
Copy link
Member Author

yadvr commented May 4, 2020

Thanks for confirming @Doni7722 :)

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

tnx for testing Liridon, sounds good. looks good as well

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@yadvr
Copy link
Member Author

yadvr commented May 4, 2020

@Doni7722 Can you also confirm, if post thing change in your test env if the usage records have the correct time according to configured timezone (in global settings) if you're using a non-UTC timezone in the usage related global settings?

@yadvr yadvr marked this pull request as ready for review May 4, 2020 10:46
@blueorangutan
Copy link

Packaging result: ✔centos7 ✔debian. JID-1210

@yadvr
Copy link
Member Author

yadvr commented May 4, 2020

@blueorangutan test

@blueorangutan
Copy link

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

Trillian test result (tid-1502)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41790 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4055-t1502-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Smoke tests completed. 82 look OK, 1 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_05_deploy_and_upgrade_kubernetes_ha_cluster Error 3609.11 test_kubernetes_clusters.py
test_06_deploy_and_invalid_upgrade_kubernetes_cluster Error 0.04 test_kubernetes_clusters.py
test_07_deploy_and_scale_kubernetes_cluster Error 0.03 test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown Error 34.43 test_kubernetes_clusters.py

@yadvr yadvr added this to the 4.14.0.0 milestone May 5, 2020
@DaanHoogland
Copy link
Contributor

@shwstppr again k8 failures, can you assess these, please?

@shwstppr
Copy link
Contributor

shwstppr commented May 5, 2020

@DaanHoogland looked into the logs, k8s cluster failed to reach the desired state in the given time. I'll try to add further improvements to test and cks backend. Failure not related to this PR changes as other tests from the same smoke test ran fine.

@andrijapanicsb
Copy link
Contributor

andrijapanicsb commented May 5, 2020

Alright....

tested this thoroughly enough, I believe:

ACS 4.13, clean install (no db.properties customisation), usage job runs in CEST!

  • UTC timezone on mgmt server = all times in DB (cloud and cloud_usage!) are in UTC, but i.e. events in GUI displayed with my local time (of my laptop, which is CEST - from my perspective that is how it should be)
  • Changed mgmt server time to CEST, rebooted server (for fun) = no changes in DB/GUI

Upgraded to 4.14 with adding "serverTimezone=UTC" so that mgmt starts fine

  • SAME AS BEFORE - i.e. all times still in UTC in DBs and CEST in GUI (my local time) = good/consistency
  • changed db.properties to serverTimezone=Europe/Belgrade on both lines (this is CEST actually)
    • no changes in DB at all...I'm talking both about events in "cloud" and the usage records (both raw and digested in cloud_usage table) - all stay in UTC times - i.e. DB is not affected by this change - which is funny - but I'm OK with this
    • GUI is now showing times from UTC (i.e. my time minus 2h) - which is wrong - i.e. we are not really honouring the timezone in DB records, but it IS reflected in GUI - this is not optimal - but is also NOT critical whatsoever...

/cc @rhtyd @DaanHoogland @Doni7722

Copy link
Contributor

@andrijapanicsb andrijapanicsb left a comment

Choose a reason for hiding this comment

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

LGTM

Manually tested, ready for merge

@andrijapanicsb
Copy link
Contributor

andrijapanicsb commented May 5, 2020

Documentation PR in progress - also please review whoever and LGTM if you are happy with it:

apache/cloudstack-documentation#112

Preview build at: https://acs-www.shapeblue.com/docs/WIP-PROOFING/pr112/upgrading/upgrade/upgrade-4.13.html#time-zone-requirements

@DaanHoogland
Copy link
Contributor

one remark about the use of articles in the english language in the doc change, @andrijapanicsb :p . I'm fine with merging this.

@DaanHoogland
Copy link
Contributor

however both jenkins and travis failed misserably.

@DaanHoogland
Copy link
Contributor

let's try again

@DaanHoogland DaanHoogland reopened this May 6, 2020
@DaanHoogland
Copy link
Contributor

@blueorangutan package

@Doni7722
Copy link

Doni7722 commented May 6, 2020

I can confirm and had same results as @andrijapanicsb had. Usage DB entries and Cloud DB entries all in UTC but CMK gives me CEST restults back. However the GUI gives me still UTC (inside the Events) but that's may caused by my environment.

@andrijapanicsb
Copy link
Contributor

Jenkins and Travis blues brothers look happy now, 2 x LGTMs, manual and regression testing OK (k8s tests are not related).

Merging.

@andrijapanicsb andrijapanicsb merged commit 381039a into apache:master May 6, 2020
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.

6 participants