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

illumos: set default timezone info path #104533

Merged
merged 3 commits into from
Jul 15, 2024
Merged

Conversation

AustinWise
Copy link
Contributor

@AustinWise AustinWise commented Jul 7, 2024

Empirically, this is where these files live on OpenIndiana and SmartOS (global zone and Joyent-branded zones). This also the documented location for illumos and Solaris. (Thanks for the pointers to man pages Tarek!)

This makes this test pass:

dotnet xunit.console.dll System.Runtime.Tests.dll -method "Tests.System.TimeProviderTests.TestSystemProviderWithTimeZone"

If you don't have the fix in this PR, you can also make this test pass by setting the TZDIR environmental variable to /usr/share/lib/zoneinfo/ .

Contributes to #34944

This is where these files live on OpenIndiana and SmartOS.
@tarekgh
Copy link
Member

tarekgh commented Jul 10, 2024

I have confirmed that both Solaris and Illumos used the /usr/share/lib/zoneinfo/ for the time zone files.

https://www.illumos.org/man/5/timezone and https://support.oracle.com/knowledge/Sun%20Microsystems/1011541_1.html#aref_section23

So, the change looks good here.

I am wondering, would it make sense instead of ifdef that for different system to introduce a fallback path to the time zones folder? I mean let GetTimeZoneDirectory return the time zones path in addition to fallback path too. Then pass these two paths to the methods reading the time zones like ReadAllBytesFromSeekableNonZeroSizeFile?

I am suggesting that as I am not sure if we build the browser or WASM for Solaris or Illumos and use the embedded files which can point at wrong path at that time?

CC @lewing if he can advise regarding the WASM cases.

@am11 do you have any feedback about this PR?

@tarekgh tarekgh added this to the 9.0.0 milestone Jul 10, 2024
@tarekgh tarekgh self-assigned this Jul 10, 2024
@am11
Copy link
Member

am11 commented Jul 10, 2024

I wasn't able to run it myself as I'm currently on osx-arm64 (where x64 VMs are quite slow). I trust what @AustinWise has found. However, I would like to test it in Global Zone vs. non- Global Zone. Sometimes these system paths and properties vary in a virtualized zone vs. GZ. (not to be confused with timezones; zones in sunos are like containers in linux and jails in freebsd, virtualized sandbox environment)

I mean let GetTimeZoneDirectory return the time zones path in addition to fallback path too. Then pass these two paths to the methods reading the time zones like ReadAllBytesFromSeekableNonZeroSizeFile?

Sounds like a good idea. It can probably return two strings for now, something like:

-        private static string GetTimeZoneDirectory()
+        private static (string knownPath, string fallbackPath) GetTimeZoneDirectory()

@AustinWise
Copy link
Contributor Author

AustinWise commented Jul 14, 2024

Having a fallback system seems reasonable. I'll set this to a draft for now, and undraft when I have an implementation and testing strategy.

@AustinWise AustinWise marked this pull request as draft July 14, 2024 03:00
@AustinWise
Copy link
Contributor Author

I have confirmed that both Solaris and Illumos used the /usr/share/lib/zoneinfo/ for the time zone files.

Thanks for the links. I also found a version of the Solaris documentation that does not require a support subscription: https://docs.oracle.com/cd/E88353_01/html/E37852/timezone-5.html

I am suggesting that as I am not sure if we build the browser or WASM for Solaris or Illumos and use the embedded files which can point at wrong path at that time?

Looking at Directory.Build.props, it looks only one of TARGET_ILLUMOS, TARGET_SOLARIS, TARGET_WASI, and TARGET_BROWSER can be defined at one time. So this change should not effect the paths used on WASI and browser to lookup time zone info. Similarly, the embedded time zone info paths are hard-coded here. The embedded files themselves from the System.Runtime.TimeZoneData Nuget package. So this PR should not change the names of embedded files.

Considering the case of running a WASI app on an illumos system, this change should not have a negative effect. See UseEmbeddedTzDatabase in TimeZoneInfo.Unix.NonAndroid.cs. UseEmbeddedTzDatabase is true when TZDIR variable is NOT set. When UseEmbeddedTzDatabase is true and an embedded time zone file cannot be found, the code does not fall back to reading from the file system. When TZDIR is set, UseEmbeddedTzDatabase will be false and the value returned from GetTimeZoneDirectory() will be the value of TZDIR. Therefore, the fact that DefaultTimeZoneDirectory is incorrect for illumos should not effect a WASI app running on illumos.

Given all of the above, I'm not sure if change the code to probe multiple directories is worth it. There are already several fallbacks for probing different directories (example). Adding more fallbacks would further complicate the code and testing, for a scenario I'm not sure exists in the wild (non-standard timezone info path). And there is already the TZDIR workaround for people on non-standard systems.

Given the above, I'm going to un-draft this with the existing, simple implementation. Please let me know if I overlooked a case where the fallback would be valuable.

However, I would like to test it in Global Zone vs. non- Global Zone.

For what it's worth, I confirmed the location in the OpenIndiania global zone, the SmartOS global zone, and a Joyent-branded zone on SmartOS. I have not tried other distributions or zone brands.

@AustinWise AustinWise marked this pull request as ready for review July 14, 2024 17:40
@tarekgh
Copy link
Member

tarekgh commented Jul 14, 2024

@AustinWise thanks for your effort. The current change is ok. I am wondering if there is any other Linux based system using /usr/share/lib/zoneinfo/ and we are not aware of. The fallback can just make it work without any further changes. But I am ok to proceed with the current change for now and watch out if we see the issue again, we can consider the fallback at that time.

I'll wait for @am11 if he has any more comments and then we can merge the change.

@am11
Copy link
Member

am11 commented Jul 14, 2024

I have not tried other distributions or zone brands.

GZ is not recommended to be used for development purposes and using non-GZ zones is a common practice. In SmartOS, GZ doesn't even persists data and config by default and even if you alter the RO-ness, it has resource limitations. OmniOS made it simpler for the cloud but we are targeting generic SunOS. To get result from a neutral zone, you can create a base-64-lts zone, e.g. https://gist.github.com/am11/5f1702de2ce5474670da4e4604856891 (I pieced it together from an old stash).

@AustinWise
Copy link
Contributor Author

AustinWise commented Jul 15, 2024

Here are some different configurations to confirm the location of zone info in, with a checkmark indicating whether or not I have tested it:

  • SmartOS
    • Global zone (20240701T205528Z)
    • Joyent-branded zone (base-64-lts 23.04 , though in Joyent-brand zones /usr is a lofs mount from the global zone 20240701T205528Z)
  • OpenIndiana
    • global zone
    • ipkg-banded zone
    • nlipkg-branded zone
  • Omni OS
    • global zone
    • ipkg-brand zone
    • lipkg-brand zone
    • sparse-brand zone
  • tribblix
    • global zone
    • sparse zone
    • whole-root zone
    • The other three brand types (Alternate-root, Partial-root, and Alien-root) seem like less common configurations that are variations on the other two brand type.

Are there other distributions or zone brands I should try?

@am11
Copy link
Member

am11 commented Jul 15, 2024

Are there other distributions or zone brands I should try?

I think having tested on generic 64-bit base is fine. When linux distros running in a zone, we don't expect dotnet(1) to run cross OS (each OS has a separate ELF binary). Thanks for testing. Looks good to me!

ps - if you could keep a zone on OpenIndiana or SmartOS for future testing, that would be great to rule out any doubts.

@tarekgh tarekgh merged commit dcfd52b into dotnet:main Jul 15, 2024
144 of 146 checks passed
@AustinWise AustinWise deleted the austin/zoneinfo branch July 19, 2024 00:36
@AustinWise
Copy link
Contributor Author

I confirmed that this change is compatible with several other illumos distributions and zone brands. I updated the above table with the extra testing.

I am wondering if there is any other Linux based system using /usr/share/lib/zoneinfo/ and we are not aware of.

From what I can gleam from code search, AIX might also use this path. I don't see obvious evidence of a Linux distribution also using this path.

ps - if you could keep a zone on OpenIndiana or SmartOS for future testing, that would be great to rule out any doubts.

Will do, I'll add an OpenIndiana zone to my primary SmartOS zone testing environment.

@tarekgh
Copy link
Member

tarekgh commented Jul 20, 2024

Thanks @AustinWise, this is very helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.DateTime community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants