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

roachtest: grafana link does not have cluster name #107894

Closed
annrpom opened this issue Jul 31, 2023 · 3 comments · Fixed by #107984
Closed

roachtest: grafana link does not have cluster name #107894

annrpom opened this issue Jul 31, 2023 · 3 comments · Fixed by #107984
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-testeng TestEng Team

Comments

@annrpom
Copy link
Contributor

annrpom commented Jul 31, 2023

Currently (as of 07/28/23), in a roachtest failure on 23.2 (like #107862), the generated Grafana link in the "Help" section redirects you to cockroachlabs.com - instead of testeng Grafana.

Looking at the issue linked above as an example, we see the following vanity link for Grafana: https://go.crdb.dev/p/roachfana//1690711427218/1690713270780

Instead, we should be seeing the following:
https://go.crdb.dev/p/roachfana/teamcity-11114747-1690696061-26-n5cpu4/1690711427218/1690713270780

The cluster name seems to be empty - clusterName in generateHelpCommand.

Jira issue: CRDB-30247

@annrpom annrpom added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Jul 31, 2023
@annrpom annrpom self-assigned this Jul 31, 2023
@annrpom annrpom added the T-testeng TestEng Team label Jul 31, 2023
@blathers-crl
Copy link

blathers-crl bot commented Jul 31, 2023

cc @cockroachdb/test-eng

annrpom added a commit to annrpom/cockroach that referenced this issue Aug 1, 2023
Previously, the testeng grafana link generated after a roachtest
failure directed the user to cockroachlabs.com due to a missing cluster
name from the link. This patch should fix this issue by getting the cluster
name from a `*githubIssues.cluster.name` instead of the `clusterName`
from roachtest/cluster.go.

Fixes: cockroachdb#107894

Release note (bug fix): The link to testeng grafana that is generated
on a roachtest failure should now properly direct to grafana.
@renatolabs
Copy link
Contributor

generateHelpCommand is called by (*githubIssues).createPostRequest:

HelpCommand: generateHelpCommand(clusterName, start, end),

The clusterName variable passed to it is the variable associated with the --cluster roachtest command line flag:

rootCmd.PersistentFlags().StringVarP(
&clusterName, "cluster", "c", "",
"Comma-separated list of names existing cluster to use for running tests. "+
"If fewer than --parallelism names are specified, then the parallelism "+
"is capped to the number of clusters specified. When a cluster does not exist "+
"yet, it is created according to the spec.")

That variable will only be set if the roachtest call was passed the --cluster argument, which is not the case in TeamCity nightly builds (in that case, clusters are created by the test runner as tests run).

Luckily, the githubIssues struct already includes a handle to the cluster used in the run, so we can change the generateHelpCommand call to pass g.cluster.Name() instead.

@annrpom do you have the time to make that change? Let me know if the above is not clear.

@annrpom
Copy link
Contributor Author

annrpom commented Aug 1, 2023

@renatolabs it is clear - thank you for the insight! my reasoning for why i thought clusterName was not being populated was ultimately incorrect (sob sob), but i do have a draft PR up: #107984

if/when you have the time, would you be able to address my comment/question about the guard i put for when a cluster creation fails (where i made the behavior == no grafana link) :>

thank you for taking your time to explain this to me/look over things!

craig bot pushed a commit that referenced this issue Aug 2, 2023
107075: asim: add random predefined cluster config selection r=kvoli a=wenyihu6

This patch takes the first step towards a randomized framework by enabling asim
testing to randomly select a cluster information configuration from a set of
predefined choices. These choices are hardcoded and represent common cluster
configurations.

TestRandomized now takes in `true` for randOptions.cluster to enable random
cluster config selection. This provides two modes for cluster generation:
1. Default: currently set to 3 nodes and 1 store per node
2. Random: randomly select a predefined cluster info

Part of: #106311

Release note: None

107967: kvserver: ignore lease validity when checking lease preferences r=erikgrinaker,andrewbaptist a=kvoli

In #107507, we began eagerly enqueuing into the replicate queue, when
acquiring a replica acquired a new lease which violated lease
preferences. Lease preferences were only considered violated when the
lease itself was valid. In #107507, we saw that it is uncommon, but
possible for an invalid lease to be acquired, violate lease preferences
and not be enqueued as a result. The end result was a leaseholder
violating the applied lease preferences which would not be resolved
until the next scanner cycle.

Update the eager enqueue check to only check that the replica is the
incoming leaseholder when applying the lease, and that the replica
violates the applied lease preferences.

Note the purgatory error introduced in #107507, still checks that the
lease is valid and owned by the store before proceeding. It is a
condition that the lease must be valid+owned by the store to have a
change planned, so whilst it is possible the lease becomes invalid
somewhere in-between planning, when the replica applies a valid lease,
it will still be enqueued so purgatory is unnecessary.

Fixes: #107862
Release note: None

107984: roachtest: bugfix testeng grafana link missing cluster name r=renatolabs a=annrpom

Previously, the testeng grafana link generated after a roachtest failure directed the user to cockroachlabs.com due to a missing cluster name from the link. This patch should fix this issue by getting the cluster name from a `*githubIssues.cluster.name` instead of the `clusterName` from roachtest/cluster.go.

Fixes: #107894

Release note (bug fix): The link to testeng grafana that is generated on a roachtest failure should now properly direct to grafana.

Co-authored-by: wenyihu6 <[email protected]>
Co-authored-by: Austen McClernon <[email protected]>
Co-authored-by: Annie Pompa <[email protected]>
@craig craig bot closed this as completed in abd1e0d Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-testeng TestEng Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants