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

Improve performance of az on ubuntu hosted runners #8441

Closed
wants to merge 22 commits into from

Conversation

jessehouwing
Copy link
Contributor

@jessehouwing jessehouwing commented Oct 2, 2023

Description

Similar to: #8294

The logs show that first invocation on Ubuntu also does a command index rebuild and installs the python keyring package.

This pull request follows the same improvements that were just added to the Windows Hosted Runner.

Expected performance improvement about 10s for calling az for the first time.

Related issue:

Check list

  • Related issue / work item is attached
  • Tests are written (if applicable)
  • Documentation is updated (if applicable)
  • Changes are tested and related VM images are successfully generated

@jessehouwing
Copy link
Contributor Author

jessehouwing commented Oct 2, 2023

@ilia-shipitsin FYI.

Attached logs show that Ubuntu has the same issue as windows, the performance impact is a lot lower though, but still, this should improve things further:

dev.azure.com_jessehouwing_6484ebc3-af16-4af9-aa66-6b3398db7214__apis_build_builds_4830_logs_6.txt

Changes are the same as on windows.

@jessehouwing jessehouwing marked this pull request as ready for review October 2, 2023 20:08
images/linux/scripts/installers/azure-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-devops-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-devops-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-devops-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-devops-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-devops-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-devops-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-devops-cli.sh Outdated Show resolved Hide resolved
@mikhailkoliada
Copy link
Contributor

@jessehouwing forgot to mention, please add the source $HELPER_SCRIPTS/etc-environment.sh line before the actual code, it will let you import the setEtcEnvironmentVariable function

@jessehouwing
Copy link
Contributor Author

@mikhailkoliada

@mikhailkoliada
Copy link
Contributor

/azp run ubuntu2004,ubuntu2204

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@mikhailkoliada
Copy link
Contributor

@jessehouwing it looks like az --version does not work as it is being bitten by the permission denied error on the /opt/az/config/ dir. I'd say we can call sudo chmod 777 -R /opt/az because later we do sudo chmod 777 -R /opt in anyway.

@jessehouwing
Copy link
Contributor Author

@jessehouwing it looks like az --version does not work as it is being bitten by the permission denied error on the /opt/az/config/ dir. I'd say we can call sudo chmod 777 -R /opt/az because later we do sudo chmod 777 -R /opt in anyway.

Hmz Funny... that worked on my machine...

Added the creation and setting permissions on that folder. Would it make sense to create and set permissions on that folder earlier in the process of creating the image, instead of setting the permissions trice?

Added to az-devops too.

@mikhailkoliada
Copy link
Contributor

/azp run ubuntu2004,ubuntu2204

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jessehouwing
Copy link
Contributor Author

jessehouwing commented Oct 5, 2023

That didn't do the trick. 😥

Would we need to add this too?

https://github.com/actions/runner-images/blob/main/images/linux/scripts/installers/Install-Toolset.ps1#L53

But then for /opt/az and /opt/az-devops?

@jessehouwing
Copy link
Contributor Author

Running a build to verify changes...

@jessehouwing
Copy link
Contributor Author

jessehouwing commented Oct 5, 2023

Build succeeded.
@mikhailkoliada can you /azp run?

@jessehouwing
Copy link
Contributor Author

/azp run ubuntu2004,ubuntu2204

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 8441 in repo actions/runner-images

@jessehouwing
Copy link
Contributor Author

I wonder if I can, I suppose it won't work.

@jessehouwing jessehouwing marked this pull request as draft October 18, 2023 20:09
@jessehouwing jessehouwing marked this pull request as ready for review October 18, 2023 20:09
Redirects config & cache folders out of user profile
Forces installation of keyring package through dummy login/logout
Force package load & compilation by calling `--help` on each sub module
@jord1e
Copy link

jord1e commented Nov 6, 2023

@jessehouwing is something blocking you on this that I can assist in?

@jessehouwing
Copy link
Contributor Author

@jessehouwing is something blocking you on this that I can assist in?

No clue what this is waiting for. As far as I can tell someone needs to run /azp run, test and merge.

@jord1e
Copy link

jord1e commented Nov 10, 2023

@mikhailkoliada could you trigger the pipeline/merge?
Thank you :)

images/linux/scripts/installers/azure-devops-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-devops-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/azure-devops-cli.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/post-deployment.sh Outdated Show resolved Hide resolved
images/linux/scripts/installers/post-deployment.sh Outdated Show resolved Hide resolved
@jessehouwing
Copy link
Contributor Author

@vpolikarpov-akvelon Fixed a couple and replied to remaining comments.

@jessehouwing
Copy link
Contributor Author

jessehouwing commented Nov 14, 2023

As requested:

@jessehouwing
Copy link
Contributor Author

@mikhailkoliada 👍

# AZURE_EXTENSION_DIR shell variable defines where modules are installed
# https://docs.microsoft.com/en-us/cli/azure/azure-cli-extensions-overview
export AZURE_EXTENSION_DIR=/opt/az/azcliextensions
setEtcEnvironmentVariable "AZURE_EXTENSION_DIR" "$AZURE_EXTENSION_DIR"
Copy link
Contributor Author

@jessehouwing jessehouwing Nov 23, 2023

Choose a reason for hiding this comment

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

This PR seems to have changed the convention away from the setEtcEnvironmentVariable call back to | tee -a /etc/environment. I'd be happy to change these calls either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikhailkoliada your thoughts? I'd like to get this closed with the pace at which you folks are updating files, I need to rebase this PR every other day at the moment ;)>

Comment on lines 9 to 10
export AZURE_EXTENSION_DIR=/opt/az/azcliextensions
echo "AZURE_EXTENSION_DIR=$AZURE_EXTENSION_DIR" | tee -a /etc/environment
Copy link
Contributor Author

@jessehouwing jessehouwing Nov 23, 2023

Choose a reason for hiding this comment

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

These lines aren't needed, but were introduced by the refactoring of the build scripts.

The environment variable is already set in install-azure-cli.ch and it makes more sense there, as it's a config setting of az-cli and not of az-devops.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikhailkoliada your thoughts? I'd like to get this closed with the pace at which you folks are updating files, I need to rebase this PR every other day at the moment ;)>

@jessehouwing
Copy link
Contributor Author

Ping @mikhailkoliada @jord1e @vpolikarpov-akvelon I really want this thing done. What can we do to move it off the list? I've rebased since the restructure, it looks like some styling things changed, hence my open questions above.

I'd like to resolve those ASAP and then close this, it is taking longer than I'd anticipated.

@mikhailkoliada
Copy link
Contributor

After observations I do not think we are ready to change this aspect.

  1. I am not sure that customers won't be broken if we change the configuration directory and make it the global one
  2. We do not observe hight demand in az optimisation from customers in general
  3. Even if (2) were true I do not think it should be fixed from az upstream side and not the runner-images side

@jord1e
Copy link

jord1e commented Dec 10, 2023

My initial az logins take 22+ seconds first run, which is insane.

  1. I am not sure that customers won't be broken if we change the configuration directory and make it the global one
    How would it break customers' setups? All configuration is handled via https://learn.microsoft.com/en-us/cli/azure/devops?view=azure-cli-latest#az-devops-configure for az devops, is it not? I get the concern but all configuration should be done via the CLI anyways.
  2. We do not observe hight demand in az optimisation from customers in general
    Because they don't know about these things in general as it would only be observed in CI environments where there is no command & update index caching
  3. Even if (2) were true I do not think it should be fixed from az upstream side and not the runner-images side
    How would az upstream be able to create an index and do an update check before the executable was every called?

And it is implemented on the windows runners already, why would implementing it on Ubuntu change anything summed up in these statements?
https://github.com/actions/runner-images/pull/8427/files

@jessehouwing
Copy link
Contributor Author

jessehouwing commented Dec 18, 2023

After observations I do not think we are ready to change this aspect.

  1. I am not sure that customers won't be broken if we change the configuration directory and make it the global one

This isn't as much of a change, as much as it is formalizing the default configuration. If not instructed through the environment variable, az will create a global configuration on first invocation and use that. If people do not want to use the default configuration, they should be overriding the environment variables to begin with, and that would still be possible.

  1. We do not observe high demand in az optimisation from customers in general

I do see high demand for faster runners and faster workflows. Many organizations spend a large amount of time trying to speed up, parallelize and optimize their workflows. That there is no demand specifically for az might be because people aren't aware a 15s gain is available. GitHub even added large runners to reduce the duration of workflows.

  1. Even if (2) were true I do not think it should be fixed from az upstream side and not the runner-images side

While I agree the az and az devops team could do more here, they're not building the tool to be primarily used on a hosted runner. az seems to me first and foremost a developer/administrator tool. The default setup works pretty well in those circumstances. The ephemeral hosted runner is a pretty specific environment and the installer isn't optimizing for that at the moment.

I think it would be possible on the ubuntu image to forego setting the environment variables, still have a reasonable speed improvement. The reason I set the environment variables on the ubuntu runner, is to make the setup aligned to the windows setup, where (due to the removal of the provisioning account) it was deemed preferred to set these environment variables.

Note that the current scripts already set a variable for the Azure DevOps extension installation folder.

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.

az behaves similarly on Ubuntu as it did on windows
4 participants