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

BSDTimezone: distinguish UTC and Etc/UTC #41234

Merged
merged 2 commits into from
Jun 11, 2018
Merged

Conversation

tmshn
Copy link
Contributor

@tmshn tmshn commented Jun 7, 2018

SUMMARY

Improved idempotency of the timezone module on BSD.

It used not to distinguish between UTC and Etc/UTC, because both represents a same timezone.
This doesn't matter on normal usage, but cause weird unwilling behavior on a specific configuration.

I fixed this to distinguish those timezones and not to determine the timezone depending on user's input, but just read only from environment

In addition to that, now the order of scanning /usr/share/zoneinfo when /etc/localtime is copied rather than symlined is deterministic, and always returns same value.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

timezone module

ANSIBLE VERSION
ansible 2.7.0.dev0 (devel 63d993e07f) last updated 2018/06/08 17:39:54 (GMT +000)
  config file = None
  configured module search path = [u'/home/tmshn/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/tmshn/ansible/lib/ansible
  executable location = /home/tmshn/ansible/bin/ansible
  python version = 2.7.13 (default, Jul 13 2017, 01:17:52) [GCC 4.2.1 Compatible FreeBSD Clang 3.8.0 (tags/RELEASE_380/final 262564)]
ADDITIONAL INFORMATION

I wrote the original code in #36715.

At that time, I was thinking that we can regard two timezones as same if their content in /usr/share/zoneinfo/ are same, because wherever the file is coming, only the content of the /etc/localtime decides how a system works.

Under this thought, I implemented to determine the timezone depending on a user's input. If the specified timezone can be regarded same as current one, we no need to change anything, and actually we wouldn't.

However, this meddlesome mix-up brings somewhat weird behavior with such configuration:

- name: 1. Set timezone to Etc/UTC
  timezone:
    name: Etc/UTC
# => after:  {'name': 'Etc/UTC'}

- name: 2. Set timezone to UTC
  timezone:
    name: UTC
# => before: {'name': 'UTC'}
# => after:  {'name': 'UTC'}

- name: 3. Set timezone to Etc/UTC again
  timezone:
    name: Etc/UTC
# => before: {'name': 'Etc/UTC'}
# => after:  {'name': 'Etc/UTC'}

Even though you set the timezone to Etc/UTC on the first step, and it actually reports "after" timezone is Etc/UTC, you'll see the "before" timezone is reported as UTC rather than Etc/UTC on the next step. The "before" value doesn't match the "after" value of the previous step.

Now I know this is my bad on designing the logic.

The problem was originally addressed at #40739 (comment), by pointing out a failure of the integration test:

TASK [timezone : ensure timezone reported as changed in checkmode] *********************************************************************************************************************************************
fatal: [testhost]: FAILED! => {
    "assertion": "timezone_set_checkmode.diff.before.name == 'Etc/UTC'",
    "changed": false,
    "evaluated_to": false
}

#40855 tried to fix this by modifying "strategy 2". However, it works only on an environment where /usr/share/zoneinfo/Etc/UTC is symlinked from /usr/share/zoneinfo/UTC

In the environment I tested, those files are physically separated and thus sudo ansible-test integration --allow-destructive timezone -v failed with the above error.

Now, with the patch of this pr, all clear.
I also added some test cases to make things clear.

@ansibot
Copy link
Contributor

ansibot commented Jun 7, 2018

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.7 This issue/PR affects Ansible v2.7 feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. labels Jun 7, 2018
@tmshn tmshn force-pushed the fix-bsd-timezone branch 2 times, most recently from 57f0734 to d62f394 Compare June 7, 2018 09:09
@ansibot ansibot added the test This PR relates to tests. label Jun 7, 2018
@tmshn tmshn force-pushed the fix-bsd-timezone branch 2 times, most recently from f5f5275 to 5e9296c Compare June 7, 2018 10:15
@ansibot ansibot added the bug This issue/PR relates to a bug. label Jun 7, 2018
@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Jun 7, 2018
@tmshn tmshn changed the title [WIP] BSDTimezone: distinguish UTC between Etc/UTC BSDTimezone: distinguish UTC and Etc/UTC Jun 8, 2018
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. community_review In order to be merged, this PR must follow the community review workflow. labels Jun 8, 2018
@tmshn
Copy link
Contributor Author

tmshn commented Jun 8, 2018

Adding tests reveals same problem exists in non-systemd linux... maybe I'll fix this in another pr.
That is due to a bug of dpkg-reconfigure, it rewrites /etc/timezone.

Maybe #34390 has something to do with the problem?

The problem looks hard to fix, so I decided to skip the new tests on non-systemd managed debian/ubuntu.

@tmshn tmshn changed the title BSDTimezone: distinguish UTC and Etc/UTC [WIP] BSDTimezone: distinguish UTC and Etc/UTC Jun 8, 2018
@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Jun 8, 2018
@ansibot
Copy link
Contributor

ansibot commented Jun 8, 2018

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/system/timezone.py:79:0: ImportError: cannot import name OrderedDict

click here for bot help

@tmshn tmshn force-pushed the fix-bsd-timezone branch 4 times, most recently from d5fb452 to 0606c80 Compare June 9, 2018 06:33
@tmshn tmshn changed the title [WIP] BSDTimezone: distinguish UTC and Etc/UTC BSDTimezone: distinguish UTC and Etc/UTC Jun 9, 2018
@tmshn
Copy link
Contributor Author

tmshn commented Jun 9, 2018

ready_for_review

@tmshn
Copy link
Contributor Author

tmshn commented Jun 9, 2018

cc @maxamillion

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 9, 2018
@maxamillion
Copy link
Contributor

rebuild_merge

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. community_review In order to be merged, this PR must follow the community review workflow. and removed community_review In order to be merged, this PR must follow the community review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 11, 2018
@ansibot ansibot merged commit bcd6b5c into ansible:devel Jun 11, 2018
@tmshn tmshn deleted the fix-bsd-timezone branch June 12, 2018 11:26
jacum pushed a commit to jacum/ansible that referenced this pull request Jun 26, 2018
* Fixed BSDTimezone to distinguish UTC and Etc/UTC

* Added test for timezone to disinguish UTC vs Etc/UTC
@dagwieers dagwieers added the bsd BSD community label Jan 18, 2019
@ansible ansible locked and limited conversation to collaborators Jun 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 bsd BSD community bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants