-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Roachtest azure nightly #111704
Roachtest azure nightly #111704
Conversation
24cdbbf
to
715fee8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I was looking for a TC run and found one :) https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_Nightlies_RoachtestNightlyAzureBazel
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @smg260 and @srosenberg)
build/teamcity/util/roachtest_util.sh
line 82 at r2 (raw file):
azure) if [ -z "${FILTER}" ]; then # Soon to go away with Radu's tag changes.
Super Nit: A TODO tag could make this more visible, with a PR/issue ref. But sounds like it's really soon so might not matter to keep track.
pkg/roachprod/vm/azure/auth.go
line 55 at r2 (raw file):
p.mu.Unlock() } else { err = errors.Wrap(err, "could got get Azure auth token")
Nit: not part of this PR, but this comment has a typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @smg260 and @srosenberg)
pkg/roachprod/vm/azure/azure.go
line 56 at r2 (raw file):
const cliErr = "please install the Azure CLI utilities " + "(https://docs.microsoft.com/en-us/cli/azure/install-azure-cli)" const authErr = "please use `az login` to login to Azure"
Nit: This error could be confusing if the env vars are set but wrong, I think?
715fee8
to
17478f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan and @srosenberg)
build/teamcity/util/roachtest_util.sh
line 82 at r2 (raw file):
Previously, herkolategan (Herko Lategan) wrote…
Super Nit: A TODO tag could make this more visible, with a PR/issue ref. But sounds like it's really soon so might not matter to keep track.
It's consistent with the other branches for AWS and GCE, so its just a comment to be taken at face value. It will go away at the same time as the others.
pkg/roachprod/vm/azure/azure.go
line 56 at r2 (raw file):
Previously, herkolategan (Herko Lategan) wrote…
Nit: This error could be confusing if the env vars are set but wrong, I think?
Thanks. Made a more generic message.
Epic: CC-25185 Release note: none
This PR ensures that Azure SDK is granted access in either 1 of 2 ways. 1. If the required environment variables are present, then, `NewAuthorizerFromEnvironment` is used. 2. Else, `az`, the CLI tool must be installed and authenticated and `NewAuthorizerFromCLI` is used. The former is used by TeamCity, the latter is likely to be used my developers.
Previously, the subscription id associated with all the SDK requests defaulted to using the first one when listing all of them. This does not allow us to query against a particular one. This PR introduces code to check for an environment variable of name `AZURE_SUBSCRIPTION_ID` which, if present is used. Otherwise, we fall back to selecting the first one. Additionally, we just memoize the simple string representation of the subscriptionId instead of the struct. Epic: CC-25185 Release note: none
Epic: CC-25185 Release note: none
roachtest: apt update before go install Update default location to `eastus`, since there is more quota there right now. Also, `apt-get update` before installing go (cdc) Epic: CC-25185 Release note: none
17478f9
to
e7dd3ec
Compare
if len(page.Values()) == 0 { | ||
err = errors.New("did not find Azure subscription") | ||
return sub, err | ||
// Fallback to retrieving the first subscription |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know what order the subscriptions come back in from sc.List(ctx)
below? Can we guarantee that the first subscription will be a subscription where a roachprod cluster should be allowed to run in?
I'm asking these questions because previously there were few subscriptions in Azure and few that people had access to. Now, there are many more subscriptions and people will generally have access to multiple. Also, soon the revenue and product divisions will be running their roachprod clusters in different subscriptions from engineering.
How about an approach like the following instead?
- If the user specifies an exact subscription to use, use that subscription. If that subscription can't be found, error out (to ensure the cluster doesn't get created in the wrong subscription).
- If the user doesn't set a subscription, try to use a standard set of adhoc subscriptions and use the first one that is found to be accessible to the user. If non exist / are accessible for the user, then it errors. The initial list of subscriptions would be these in this order:
e2e-adhoc
,Microsoft Azure Sponsorship
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jlinder and @srosenberg)
pkg/roachprod/vm/azure/azure.go
line 1517 at r4 (raw file):
Previously, jlinder (James H. Linder) wrote…
Do we know what order the subscriptions come back in from
sc.List(ctx)
below? Can we guarantee that the first subscription will be a subscription where a roachprod cluster should be allowed to run in?I'm asking these questions because previously there were few subscriptions in Azure and few that people had access to. Now, there are many more subscriptions and people will generally have access to multiple. Also, soon the revenue and product divisions will be running their roachprod clusters in different subscriptions from engineering.
How about an approach like the following instead?
- If the user specifies an exact subscription to use, use that subscription. If that subscription can't be found, error out (to ensure the cluster doesn't get created in the wrong subscription).
- If the user doesn't set a subscription, try to use a standard set of adhoc subscriptions and use the first one that is found to be accessible to the user. If non exist / are accessible for the user, then it errors. The initial list of subscriptions would be these in this order:
e2e-adhoc
,Microsoft Azure Sponsorship
.
Can't guarantee order, as they could be different per person, nor that what is returned is usable by roachprod.
I don't like returning the first one blindly either, but it is just preserving the existing behaviour. I'd be ok to remove that.
The first bullet is covered because the code does look for a subscription in the environment, and in the case that its invalid, API calls error out.
The second bullet can be implemented but
- it ties roachprod to CRL's infrastructure, unless we store in config somewhere
- just because we can authenticate against a subscription, does not mean we will have the required permissions to do anything there, so "accessible to user" is prob not enough
- Minor but ideally we'd use the subscription ID over the display name.
If the concern here mainly for users of roachprod (i.e. not TeamCity), I could add a higher priority fallback which invokes az
in an attempt to get the default subscription ID, only if CLI auth is used? This can be set when installing the CLI and would be a reasonable fallback.
I talked to James and we created this issue (because we can do this later and it shouldn't block the PR). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @smg260 and @srosenberg)
pkg/roachprod/vm/azure/azure.go
line 1517 at r4 (raw file):
Previously, smg260 (Miral Gadani) wrote…
Can't guarantee order, as they could be different per person, nor that what is returned is usable by roachprod.
I don't like returning the first one blindly either, but it is just preserving the existing behaviour. I'd be ok to remove that.
The first bullet is covered because the code does look for a subscription in the environment, and in the case that its invalid, API calls error out.
The second bullet can be implemented but
- it ties roachprod to CRL's infrastructure, unless we store in config somewhere
- just because we can authenticate against a subscription, does not mean we will have the required permissions to do anything there, so "accessible to user" is prob not enough
- Minor but ideally we'd use the subscription ID over the display name.
If the concern here mainly for users of roachprod (i.e. not TeamCity), I could add a higher priority fallback which invokes
az
in an attempt to get the default subscription ID, only if CLI auth is used? This can be set when installing the CLI and would be a reasonable fallback.
👍 re: the first bullet being covered.
On the second bullet:
Agreed with Ahmad that this doesn't need to be fixed in this PR. I added a few more thoughts to the issue he linked.
TFTR! bors r=healthy-pod,herkolategan |
Build succeeded: |
These are a series of commits to enable roachtests to run on Azure in TeamCity.
apt-get update
before installing go for a cdc test (failed on azure)A follow up PR will enable an initial set of roachtests to run.
Epic: CC-25185
Release note: none