-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: surface cloud cluster spec info in artifacts #124243
roachtest: surface cloud cluster spec info in artifacts #124243
Conversation
4dfb0fe
to
5d7bc8c
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.
So it seems like we only surface this info if the test fails? I think that's fine in the context of using this to debug test failures, but just making sure that's the intention here.
if err != nil { | ||
return err | ||
} | ||
providerToVMs := bucketVMsByProvider(cachedCluster) |
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.
You could use vm.FanOut
instead if you wanted to. Might be slightly simpler. It collates vms by a provider and calls the function passed to it, basically what you're doing here.
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.
Will take this up in a follow up PR. Do you think we should use vm.FanOut
for GetPreemptedVMs
and GetHostErrorVMs
as well?
Yes that's the intention |
33a1564
to
5021905
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.
Another small nit but otherwise LGTM!
Previously, getting the spec of the VMs on which a roachtest ran was tricky to derive. This was inadequate because it's often necessary to understand the particulars of a roachtest run's environment. To address this, this patch creates a json file per VM describing its spec in. These files are stored in the Artifacts directory. Epic: none Fixes: cockroachdb#112707 Release note: None
5021905
to
0e3ae2c
Compare
bors r+ |
This is a follow up PR to cockroachdb#124243. It implements the surfacing of cluster specs for aws and refactors logic in `cluster.go` to make it cloud agnostic. Epic: none Release note: None
This is a follow up PR to cockroachdb#124243. It implements the surfacing of cluster specs for aws and refactors logic in `cluster.go` to make it cloud agnostic. Epic: none Release note: None
124443: roachtest: surface cloud cluster spec info in artifacts for aws r=vidit-bhat a=vidit-bhat This is a follow up PR to #124243. It implements the surfacing of cluster specs for `aws` and refactors logic in `cluster.go` to make it cloud agnostic. Epic: none Release note: None Co-authored-by: Vidit Bhat <[email protected]>
blathers backport 24.1 23.2 |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 0e3ae2c to blathers/backport-release-23.2-124243: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.2 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This is a follow up PR to #124243. It implements the surfacing of cluster specs for aws and refactors logic in `cluster.go` to make it cloud agnostic. Epic: none Release note: None
Previously, getting the spec of the VMs on which a roachtest ran was tricky to derive.
This was inadequate because it's often necessary to understand the particulars of a roachtest run's environment.
To address this, this patch creates a json file per VM describing its spec. These files are stored under
artifacts/vm_spec
.Epic: none
Fixes: #112707
Release note: None