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.

Sign up for GitHub

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

Jump to bottom

bindata/assets/alerts/api-usage: Include removed_release in APIRemovedInNext*ReleaseInUse labels #1367

Merged
merged 1 commit into from
Oct 17, 2022
Merged

bindata/assets/alerts/api-usage: Include removed_release in APIRemovedInNext*ReleaseInUse labels #1367

merged 1 commit into from
Oct 17, 2022

Conversation

wking
Copy link
Member

@wking wking commented Jul 11, 2022

There may be version skew around updates, where an alert gets bumped to consider 1.26 "the next release" while other cluster components are still running 1.25, or vice versa. This change makes it a bit easier to understand those skew issues by explicitly including in the alert description (and Telemetry-uploaded alert labels) the removed_release value we're complaining about.

@openshift-ci openshift-ci bot requested review from deads2k and mfojtik July 11, 2022 19:04
@wking wking force-pushed the include-version-in-RemovedInNext-alerts branch from 14cd0e2 to 584c549 Compare July 11, 2022 19:05
@wking
Copy link
Member Author

wking commented Jul 12, 2022

In a cluster-bot cluster from launch 4.12.0-0.nightly-2022-07-11-054352,openshift/cluster-kube-apiserver-operator#1367, with a PDB manifest:

$ cat pdb.yaml 
apiVersion: policy/v1beta1
kind: PodDisruptionBudget
metadata:
  name: prow-pods
  namespace: openshift
spec:
  maxUnavailable: 0
  selector:
    matchLabels:
      created-by-prow: "true"

Creating and deleting the PDB to drive up some API traffic on the deprecated version:

for i in $(seq 10); do
  oc apply -f pdb.yaml
  oc -n openshift delete poddisruptionbudget prow-pods
done

The firing alerts do include the new label and they render it in their description:

image

image

sanchezl
sanchezl approved these changes Sep 30, 2022
View reviewed changes
Copy link
Contributor

@sanchezl sanchezl left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2022
@sanchezl
Copy link
Contributor

/retest

@tkashem
Copy link
Contributor

tkashem commented Sep 30, 2022

/approve
Thanks

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 30, 2022
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 843db4f and 2 for PR HEAD 584c549 in total

@wking
Copy link
Member Author

wking commented Sep 30, 2022

/retest required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 30, 2022

@wking: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test e2e-aws-ovn
  • /test e2e-aws-serial
  • /test e2e-gcp-operator
  • /test e2e-gcp-operator-encryption
  • /test e2e-gcp-operator-encryption-perf
  • /test e2e-gcp-operator-encryption-rotation
  • /test e2e-upgrade
  • /test images
  • /test k8s-e2e-gcp
  • /test unit
  • /test verify
  • /test verify-deps

The following commands are available to trigger optional jobs:

  • /test e2e-aws-operator-disruptive-single-node
  • /test e2e-aws-single-node
  • /test e2e-gcp-operator-encryption-perf-single-node
  • /test e2e-gcp-operator-encryption-rotation-single-node
  • /test e2e-gcp-operator-encryption-single-node
  • /test e2e-gcp-operator-single-node
  • /test e2e-metal-single-node-live-iso
  • /test k8s-e2e-gcp-serial

Use /test all to run the following jobs that were automatically triggered:

  • pull-ci-openshift-cluster-kube-apiserver-operator-master-e2e-aws-operator-disruptive-single-node
  • pull-ci-openshift-cluster-kube-apiserver-operator-master-e2e-aws-ovn
  • pull-ci-openshift-cluster-kube-apiserver-operator-master-e2e-aws-serial
  • pull-ci-openshift-cluster-kube-apiserver-operator-master-e2e-aws-single-node
  • pull-ci-openshift-cluster-kube-apiserver-operator-master-e2e-gcp-operator
  • pull-ci-openshift-cluster-kube-apiserver-operator-master-e2e-gcp-operator-single-node
  • pull-ci-openshift-cluster-kube-apiserver-operator-master-e2e-upgrade
  • pull-ci-openshift-cluster-kube-apiserver-operator-master-images
  • pull-ci-openshift-cluster-kube-apiserver-operator-master-k8s-e2e-gcp
  • pull-ci-openshift-cluster-kube-apiserver-operator-master-k8s-e2e-gcp-serial
  • pull-ci-openshift-cluster-kube-apiserver-operator-master-unit
  • pull-ci-openshift-cluster-kube-apiserver-operator-master-verify
  • pull-ci-openshift-cluster-kube-apiserver-operator-master-verify-deps

In response to this:

/retest required

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wking
Copy link
Member Author

wking commented Oct 1, 2022

/retest-required

1 similar comment
@wking
Copy link
Member Author

wking commented Oct 1, 2022

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 9c36fe3 and 1 for PR HEAD 584c549 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 71cf3b4 and 0 for PR HEAD 584c549 in total

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2022
@wking wking force-pushed the include-version-in-RemovedInNext-alerts branch from 584c549 to b0f55b9 Compare October 10, 2022 18:33
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 10, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2022
@wking
Copy link
Member Author

wking commented Oct 10, 2022

rebased around #1382 with 584c549 -> b0f55b9

sanchezl
sanchezl reviewed Oct 11, 2022
View reviewed changes
Copy link
Contributor

@sanchezl sanchezl left a comment

Choose a reason for hiding this comment

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

The line breaks are causing issues with the regex used in the tests.

@wking wking force-pushed the include-version-in-RemovedInNext-alerts branch 2 times, most recently from b04153a to 5272c84 Compare October 11, 2022 15:04
sanchezl
sanchezl approved these changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@sanchezl sanchezl left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2022
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 71cf3b4 and 2 for PR HEAD 5272c84 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 477d55f and 1 for PR HEAD 5272c84 in total

@wking
Copy link
Member Author

wking commented Oct 12, 2022

/retest-required

3 similar comments
@wking
Copy link
Member Author

wking commented Oct 12, 2022

/retest-required

@wking
Copy link
Member Author

wking commented Oct 13, 2022

/retest-required

@wking
Copy link
Member Author

wking commented Oct 13, 2022

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 8806724 and 0 for PR HEAD 5272c84 in total

@openshift-ci-robot
Copy link

/hold

Revision 5272c84 was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2022
@wking
bindata/assets/alerts/api-usage: Include removed_release in APIRemove…
c5593cb 
…dInNext*ReleaseInUse labels

There may be version skew around updates, where an alert gets bumped
to consider 1.26 "the next release" while other cluster components are
still running 1.25, or vice versa.  This change makes it a bit easier
to understand those skew issues by explicitly including in the alert
description (and Telemetry-uploaded alert labels) the removed_release
value we're complaining about.
@wking wking force-pushed the include-version-in-RemovedInNext-alerts branch from 5272c84 to c5593cb Compare October 14, 2022 06:51
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 14, 2022

@wking: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws 584c549 link true /test e2e-aws
ci/prow/e2e-aws-operator-disruptive-single-node c5593cb link false /test e2e-aws-operator-disruptive-single-node
ci/prow/e2e-gcp-operator-single-node c5593cb link false /test e2e-gcp-operator-single-node

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@wking
Copy link
Member Author

wking commented Oct 14, 2022

Finally green enough 😅

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2022
@sanchezl
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sanchezl, tkashem, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit daf2176 into openshift:master Oct 17, 2022
@wking wking deleted the include-version-in-RemovedInNext-alerts branch October 20, 2022 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanchezl sanchezl sanchezl approved these changes

@deads2k deads2k Awaiting requested review from deads2k

@mfojtik mfojtik Awaiting requested review from mfojtik

Assignees

@sanchezl sanchezl

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wking @sanchezl @tkashem @openshift-ci-robot @openshift-merge-robot