Skip to content

Fix Importing Az in Parallel #18483

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

Merged

Conversation

isra-fel
Copy link
Member

@isra-fel isra-fel commented Jun 13, 2022

Description

Fix #18321

This PR resolves the file IO conflict when importing Az.Accounts in parallel PowerShell processes.

Root cause:
Config manager is initialized during importing Az.Accounts. It tries to create the config file if it does not exist. It also tries to write that file if there are old configs need to migrate. At the meantime, it reads the file to build config values.
All these file IO operations may conflict with each other. Causing Az.Accounts fail to be imported.

Fix:

  1. Add a mutex to protect the initialization of the configs. Other processes will need to wait until the previous one finishes initializing.
  2. Add a safe mode that we can fall back to when configs fail to initialize. In safe mode, it does not resolve configs from file system / environment variables. There are only default values.

Design:
This PR introduced a sibling to ConfigManager as SafeConfigManager, which do not load configs from file system nor environment variables, so it's safe from unexpected exceptions.
After that the name ConfigManager seemed inappropriate so I renamed it into DefaultConfigManager and introduced an abstract base class ConfigManager for both to inherit from.
Now AzureSessionInitializer will first try to create a DefaultConfigManager and if it fails, use SafeConfigManager instead.

P.S. we might want to add this to smoke test.

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
      • {Please put the link here}
    • the markdown help files have been regenerated using the commands listed here

@isra-fel isra-fel force-pushed the 18321-az-module-cannot-be-imported-in-parallel branch from df48745 to 87d1543 Compare June 15, 2022 06:24
@isra-fel isra-fel marked this pull request as ready for review June 15, 2022 06:24
@@ -34,11 +33,10 @@ namespace Microsoft.Azure.Commands.Common.Authentication.Config
internal class ConfigInitializer
{
internal IDataStore DataStore { get; set; } = new DiskDataStore();
private static readonly object _fsLock = new object();
Copy link
Member Author

Choose a reason for hiding this comment

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

lock is no longer needed because the initialization is guarded by a mutex

@jose-bueno-microsoft
Copy link

@isra-fel is there an ETA on this release? I have a few customers asking on this fix release. Many thanks!

@isra-fel isra-fel added this to the July 2022 (2022-07-05) milestone Jun 17, 2022
@isra-fel
Copy link
Member Author

isra-fel commented Jun 17, 2022

Hi @jose-bueno-microsoft we are getting this reviewed and tested by different teams. Currently the ETA would be July 5th.
Update 6/20: The engineering build has been verified by Functions team.

Comment on lines +49 to +53
var dataStore = settings?.DataStore;
if (dataStore == null)
{
dataStore = new MockDataStore();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why previous code didn't work? var dataStore = settings?.DataStore ?? new MockDataStore();

@BethanyZhou BethanyZhou merged commit 41b50c9 into Azure:main Jun 28, 2022
@isra-fel isra-fel deleted the 18321-az-module-cannot-be-imported-in-parallel branch July 1, 2022 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Az module cannot be imported in parallel
4 participants