Add PagerDutyIntegration CRD by grdryn · Pull Request #90 · openshift/pagerduty-operator

grdryn · 2020-06-02T12:51:13Z

This change adds a PagerDutyIntegration CRD & replaces the
ClusterDeployment and SyncSet controllers with a single
PagerDutyIntegration controller. This will allow more than one
PagerDuty service to be created per cluster, so that more than one SRE
team can receive alerts for different concerns (e.g. layered products
on top of OSD).

Watches

Since there's only one controller, and there's a many-to-many
relationship between PagerDutyIntegrations and ClusterDeployments,
here's how reconciliation will be triggered:

Create/Update/Delete of a PagerDutyIntegration will trigger a
reconcile request for that PagerDutyIntegration.
Create/Update/Delete of a ClusterDeployment will trigger a reconcile
request for each PagerDutyIntegration that matches that
ClusterDeployment (using the spec.clusterDeploymentSelector label
selector).
Create/Update/Delete of a ConfigMap/Secret/SyncSet owned by a
ClusterDeployment will trigger a reconcile request for each
PagerDutyIntegration that matches that ClusterDeployment.

Migration / upgrade logic

This change also includes temporary migration logic (which can
optionally be removed easily in a subsequent release) to decide based
on an annotation whether the CR should be used for migration of
resources from the pre-PagerDutyIntegration CRD way of working. This
pd.openshift.io/legacy annotation will inform the operator that
there may be clusters that were configured with a pagerduty service
already, and that it should update the names of those resources to the
new format. Once all of the clusters have been migrated, it will
remove the annotation.

jewzaam · 2020-06-05T11:40:06Z

PR check needs a fix: #97

grdryn · 2020-06-23T13:03:42Z

/cc @georgettica

RiRa12621 · 2020-06-30T05:53:31Z

/assign @RiRa12621

RiRa12621 · 2020-07-01T06:25:14Z

/hold
For time to sync on a plan going forward

deploy/crds/pagerduty.openshift.io_pagerdutyintegrations_crd.yaml

georgettica

I don't really know what you added me but I tried to give this a pass. maybe some of the stuff will help

pkg/controller/pagerdutyintegration/clusterdeployment_created.go

pkg/controller/pagerdutyintegration/mappers.go

pkg/controller/pagerdutyintegration/pagerdutyintegration_controller.go

RiRa12621 · 2020-07-01T13:06:20Z

/hold cancel

RiRa12621

Seems sane
see the comments + what @georgettica said

README.md

deploy/crds/pagerduty_v1alpha1_pagerdutyintegration_cr.yaml

go.mod

pkg/apis/pagerduty/v1alpha1/pagerdutyintegration_types.go

README.md

grdryn · 2020-07-02T15:58:08Z

@RiRa12621 @georgettica Thanks a lot for the reviews! 🙂

I believe I've addressed all of the comments in one way or another, if you'd like to take a look again.

RiRa12621

/lgtm
/approve

RiRa12621 · 2020-07-03T13:26:00Z

/assign @jewzaam for approval

openshift-ci-robot · 2020-07-03T13:26:02Z

@RiRa12621: GitHub didn't allow me to assign the following users: for, approval.

Note that only openshift members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/assign @jewzaam for approval

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.

grdryn · 2020-07-03T14:48:53Z

@RiRa12621 @jewzaam Thanks for the sign-off! :D Note that someone/something will need to create a PagerDutyIntegration CR for the details that were previously used in the old flow, and that CR should have the pd.openshift.io/legacy annotation to lift the existing resources for existing clusters into the new CR-based flow.

RiRa12621 · 2020-07-03T14:59:13Z

it's gonna go to int and stage anyways first, so I'd assume we notice all kinds of "problems" like this before it hits prod

This is just the default generated types and CRD. A future commit will flesh them out with the properties that we need in this project.

Command used: operator-sdk add controller \ --kind=PagerDutyIntegration \ --api-version=pagerduty.openshift.io/v1alpha1

This doesn't yet watch everything, and it doesn't handle migration of existing configured clusters to the new way with the PagerDutyIntegration CRD.

Since a reconciliation request for this controller is the name and namespace of a PagerDutyIntegration instance, and there might be multiple instances tied to e.g. a ClusterDeployment instance on which an event occurs; we want to trigger a reconciliation request for each PagerDutyIntegration that may be affected.

Before this change there were several places where the names of SyncSet/ConfigMap/Secret resources were decided, and since those are more dynamic in this patch set, it makes sense to try to tidy it up a bit.

Since this patch set changes the way this operator works to reconcile a new custom resource, we need to be able to get existing ClusterDeployments with their PagerDuty services into this new format. Rather than adding some sort of additional field to the spec that would only ever be useful once for one CR, this looks for the following annotation on CRDs: "pd.openshift.io/legacy" with a non-empty value. This allows us to set this annotation on the OSD PagerDutyIntegration CR, so that the operator knows that this is the one to use for migration. Once it has migrated all of the resources, it will delete the annotation from the PagerDutyIntegration CR, so that it won't attempt to migrate on every reconciliation.

This takes the old tests from the ClusterDeployment controller that was here before, and makes them work with the PagerDutyIntegration controller. NOTE: Some of the tests have been commented out until some dependencies get updated, as the fake k8s client can't handle list options to a client.List call correctly.

This was a requested change from code review: https://github.com/openshift/pagerduty-operator/pull/90/files#r448159838

grdryn · 2020-07-15T11:20:45Z

This has been rebased now, as the changes to upgrade the operator-sdk version have been merged to master in #100

RiRa12621

/lgtm
/assign @jharrington22 for approval since @jewzaam is on PTO

openshift-ci-robot · 2020-07-15T12:11:56Z

@RiRa12621: GitHub didn't allow me to assign the following users: since, is, on, PTO, for, approval.

Note that only openshift members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/lgtm
/assign @jharrington22 for approval since @jewzaam is on PTO

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.

jewzaam · 2020-07-20T12:18:44Z

/approve

openshift-ci-robot · 2020-07-20T12:19:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grdryn, jewzaam, RiRa12621

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [jewzaam]

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

…ionCRD" This reverts commit 84a3b77, reversing changes made to 291012e.

…ionCRD" This reverts commit 84a3b77, reversing changes made to 291012e. PagerDutyIntegration CRD is not deployed with the operator. Operator cannot start.

Revert "Merge pull request #90 from grdryn/PagerDutyIntegrationCRD"

openshift-ci-robot requested review from blrm and jewzaam June 2, 2020 12:51

grdryn force-pushed the PagerDutyIntegrationCRD branch 2 times, most recently from 350efb0 to cc03b66 Compare June 5, 2020 11:00

grdryn force-pushed the PagerDutyIntegrationCRD branch from cc03b66 to 0fa18ef Compare June 8, 2020 14:57

grdryn mentioned this pull request Jun 8, 2020

Fix yaml validation in PR check #97

Merged

grdryn force-pushed the PagerDutyIntegrationCRD branch from 0fa18ef to 1ee1d5e Compare June 15, 2020 18:22

openshift-ci-robot requested a review from georgettica June 23, 2020 13:03

openshift-ci-robot assigned RiRa12621 Jun 30, 2020

RiRa12621 added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 30, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 1, 2020

georgettica reviewed Jul 1, 2020

View reviewed changes

deploy/crds/pagerduty.openshift.io_pagerdutyintegrations_crd.yaml Show resolved Hide resolved

georgettica reviewed Jul 1, 2020

View reviewed changes

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 1, 2020

RiRa12621 reviewed Jul 2, 2020

View reviewed changes

georgettica reviewed Jul 2, 2020

View reviewed changes

README.md Show resolved Hide resolved

RiRa12621 approved these changes Jul 3, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2020

openshift-ci-robot assigned jewzaam Jul 3, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2020

grdryn added 14 commits July 15, 2020 11:58

Generate PagerDutyIntegration using operator-sdk

0e7dfe7

This is just the default generated types and CRD. A future commit will flesh them out with the properties that we need in this project.

Add appropriate fields to CRD Spec

1021315

Scaffold PagerDutyIntegration controller

a63e6f8

Command used: operator-sdk add controller \ --kind=PagerDutyIntegration \ --api-version=pagerduty.openshift.io/v1alpha1

Replace CD/SS controllers w/ PagerDutyIntegration

dbcaca6

This doesn't yet watch everything, and it doesn't handle migration of existing configured clusters to the new way with the PagerDutyIntegration CRD.

Make naming of resources more understandable

3baf71c

Before this change there were several places where the names of SyncSet/ConfigMap/Secret resources were decided, and since those are more dynamic in this patch set, it makes sense to try to tidy it up a bit.

Don't return reconcile.Result from create/delete funcs

1f19a94

Update README with details of new changes

794df56

Add test for migration code

251d438

Address initial code review comments

b2bd0d9

Use go:generate directive to generate correct CRD

e0df263

Exit reconcile early after setting finalizer

20f2b76

This was a requested change from code review: https://github.com/openshift/pagerduty-operator/pull/90/files#r448159838

grdryn force-pushed the PagerDutyIntegrationCRD branch from a2c5f09 to 20f2b76 Compare July 15, 2020 11:08

RiRa12621 approved these changes Jul 15, 2020

View reviewed changes

openshift-ci-robot assigned jharrington22 Jul 15, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2020

openshift-merge-robot merged commit 84a3b77 into openshift:master Jul 20, 2020

jewzaam added a commit to jewzaam/pagerduty-operator that referenced this pull request Jul 20, 2020

Revert "Merge pull request openshift#90 from grdryn/PagerDutyIntegrat…

77c06d5

…ionCRD" This reverts commit 84a3b77, reversing changes made to 291012e.

openshift-merge-robot added a commit that referenced this pull request Jul 20, 2020

Merge pull request #101 from jewzaam/revert-pr-90

41d6c9b

Revert "Merge pull request #90 from grdryn/PagerDutyIntegrationCRD"

grdryn deleted the PagerDutyIntegrationCRD branch July 21, 2020 10:48

grdryn mentioned this pull request Jul 21, 2020

Unrevert PagerDutyIntegration & get generate-operator-bundle.py to add CRDs to CSV #102

Merged

grdryn mentioned this pull request Aug 19, 2020

Adding DeadMansSnitchIntegration CRD to deadmanssnitch operator openshift/deadmanssnitch-operator#70

Merged

Comments

Conversation

grdryn commented Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Watches

Migration / upgrade logic

Uh oh!

jewzaam commented Jun 5, 2020

Uh oh!

grdryn commented Jun 23, 2020

Uh oh!

RiRa12621 commented Jun 30, 2020

Uh oh!

RiRa12621 commented Jul 1, 2020

Uh oh!

Uh oh!

georgettica left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RiRa12621 commented Jul 1, 2020

Uh oh!

RiRa12621 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grdryn commented Jul 2, 2020

Uh oh!

RiRa12621 left a comment

Choose a reason for hiding this comment

Uh oh!

RiRa12621 commented Jul 3, 2020

Uh oh!

openshift-ci-robot commented Jul 3, 2020

Uh oh!

grdryn commented Jul 3, 2020

Uh oh!

RiRa12621 commented Jul 3, 2020

Uh oh!

grdryn commented Jul 15, 2020

Uh oh!

RiRa12621 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jul 15, 2020

Uh oh!

jewzaam commented Jul 20, 2020

Uh oh!

openshift-ci-robot commented Jul 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

grdryn commented Jun 2, 2020 •

edited

Loading