Upgrade to controller-runtime 0.7.0 #1318

dgoodwin · 2021-03-26T17:14:08Z

A number of invasive changes required here not limited to:

Reconcile func now requires a context
Broad use of client.Object rather than runtime.Object
New isUnstructured boolean arg for RESTClientForGVK
Events now require Object not an ObjectKey (namespace/name)
Changes to EnqueueRequestsFromMapFunc

The highest risk areas here are:

dnszone and dnsendpoint controllers. We have a GenericEvent being emitted used to trigger reconciles which switched from an object key to full object. Plumbing this through was tricky and wreaked havoc on test assertions.
Use of context instead of stop functions. (not an area I'm super familiar with)

x-ref: https://issues.redhat.com/browse/HIVE-1452

dgoodwin · 2021-03-26T17:14:48Z

Tests are passing and verify is clear, I am doing manual testing of a manageDNS cluster at the moment and it has made it past DNS and launched the install, so looking good so far.

dgoodwin · 2021-03-26T17:56:19Z

/test e2e-azure
/test e2e-gcp

abhinavdahiya · 2021-03-26T18:14:06Z

apis/go.mod

 	k8s.io/api v0.20.0
 	k8s.io/apimachinery v0.20.0
-	sigs.k8s.io/controller-runtime v0.6.2
+	sigs.k8s.io/controller-runtime v0.7.0


should we bump this to 0.7.3 as the PR title suggests?

hmm.. or maybe rename the PR title as we are bumping to 0.7.0

Yup my mistake, wanted to stick with 0.7.0 to match agent controllers, but this is going to be unsustainable if we have to stay in lockstep. Maybe we can get their controllers into Hive.

abhinavdahiya · 2021-03-26T18:24:26Z

pkg/controller/clusterpool/clusterdeploymentexpectations.go

+		handler.EnqueueRequestsFromMapFunc(
+			func(a client.Object) []reconcile.Request {
+				cpKey := clusterPoolKey(a.(*hivev1.ClusterDeployment))
+				if cpKey == nil {
+					return nil
+				}
+				return []reconcile.Request{{NamespacedName: *cpKey}}
+			},
+		),
+		r,


struct is being initialized without Field names https://github.com/uber-go/guide/blob/master/style.md#use-field-names-to-initialize-structs

dgoodwin · 2021-03-30T13:03:14Z

Updated per review. Thanks!

dgoodwin · 2021-03-31T11:27:34Z

/test e2e-azure
/test e2e-gcp

joelddiaz

/lgtm

dgoodwin · 2021-03-31T13:28:39Z

/hold

dgoodwin · 2021-03-31T13:42:39Z

I didn't see changes to cover these changes in https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.7.0 @dgoodwin
Change default webhook port to 9443 (https://github.com/kubernetes-sigs/controller-runtime/pull/1076)

Impact: update your deployment configuration to use port 9443, or manually
configure the webhook port in manager Options.
I think we have webhooks and making sure the correct port was configured.

Looks like this is ok, we specify --secure-port 9443 in our Deployment and the Service forwards 443 to 9443. I can see the admission hook getting called if I try to make a bad change.

Remove logs from internal controller (https://github.com/kubernetes-sigs/controller-runtime/pull/1096)

Impact: automatic logs for "succesful reconcile" no longer exist
(it tended to duplicate manual logs that folks had in place).
dropping the logs means that we won't get the logs from errors that aren't explicity logged. we tried to capture those here #1263 but i think moving to this version will again drop those error logs.

Reading the discussion in there, I don't think there's any action for us to take. We don't want to log at V(5), we should be logging our own errors, and it sounds like that is the direction controller-runtime is going. Feels like we'll have to handle any situations where we missed logging as they arise.

EDIT: actually may have been restored: kubernetes-sigs/controller-runtime#1245

pkg/webhook/admission: upgrade v1beta1 admission types to v1 (https://github.com/kubernetes-sigs/controller-runtime/pull/1284)

Impact: construct v1 Go types instead of v1beta1. Both versions are
supported on the wire.

Does this affect Hive running on 3.11 cluster as maybe that version is not supported there??

abhinavdahiya · 2021-03-31T16:38:31Z

I didn't see changes to cover these changes in https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.7.0 @dgoodwin
Change default webhook port to 9443 (https://github.com/kubernetes-sigs/controller-runtime/pull/1076)

Impact: update your deployment configuration to use port 9443, or manually
configure the webhook port in manager Options.
I think we have webhooks and making sure the correct port was configured.
Looks like this is ok, we specify --secure-port 9443 in our Deployment and the Service forwards 443 to 9443. I can see the admission hook getting called if I try to make a bad change.
Remove logs from internal controller (https://github.com/kubernetes-sigs/controller-runtime/pull/1096)

Impact: automatic logs for "succesful reconcile" no longer exist
(it tended to duplicate manual logs that folks had in place).
dropping the logs means that we won't get the logs from errors that aren't explicity logged. we tried to capture those here #1263 but i think moving to this version will again drop those error logs.
Reading the discussion in there, I don't think there's any action for us to take. We don't want to log at V(5), we should be logging our own errors,

If controller-runtime return from Reconcile will not log the errors, we should log any error returned by Reconcile... that does not happen today. And the alternative that you have proposed of logging before returning error when we see a bug/requirement is not sustainable.

logging the error returned by Reconcile is important info that should not be left to each return. If the controller-runtime cannot be our backstop, we should enforce all Reconcile functions look like

func (r *Reconciler) Reconcile(_, _) (reconcile.Result, error) {
    // setup logger

    r, err := r.reconcile(_, _, logger)
    if err != nil {
        logger.WithError(err).Error("failed to reconcile")
    }
    return r, err
}

and it sounds like that is the direction controller-runtime is going. Feels like we'll have to handle any situations where we missed logging as they arise.

EDIT: actually may have been restored: kubernetes-sigs/controller-runtime#1245
pkg/webhook/admission: upgrade v1beta1 admission types to v1 (https://github.com/kubernetes-sigs/controller-runtime/pull/1284)

Impact: construct v1 Go types instead of v1beta1. Both versions are
supported on the wire.
Does this affect Hive running on 3.11 cluster as maybe that version is not supported there??

dgoodwin · 2021-03-31T17:03:22Z

I have confirmed that we still get reconciler errors logged by controller-runtime by modifying clusterdeployment_controller to just immediately return an error:

time="2021-03-31T17:00:40.379Z" level=error msg="Reconciler error" _name=clusterdeployment-controller error="new error!!!!!!!!" name=dgoodwin-gcp namespace=hive

Which looks like Alberto's commit restored what we were counting on.

Per slack I'm going to remove the hold, let this roll to stage/int so we can test it on one of the v3 clusters and make sure admission webhooks are working properly.

dgoodwin · 2021-03-31T17:03:45Z

/hold cancel

openshift-bot · 2021-03-31T21:50:50Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-31T22:16:50Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-31T22:29:47Z

/retest

Please review the full test history for this PR and help us cut down flakes.

suhanime · 2021-03-31T22:31:32Z

/hold for rebase

dgoodwin · 2021-04-01T14:12:27Z

/hold cancel

Updated with a merge.

joelddiaz

/lgtm

openshift-ci-robot · 2021-04-01T14:15:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgoodwin, joelddiaz

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:

~~OWNERS~~ [dgoodwin,joelddiaz]

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

openshift-bot · 2021-04-01T18:52:24Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-04-01T19:06:21Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-04-01T22:34:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-04-01T23:13:19Z

/retest

Please review the full test history for this PR and help us cut down flakes.

dgoodwin added 21 commits March 26, 2021 13:37

Upgrade apis to patched controller-runtime v0.7.0.

d8ae8b6

Upgrade hive to patched controller-runtime v0.7.0. (not compiling yet)

5f11a93

Fix creation of metrics DelegatingClient.

12b4983

Fix dnszone utils mapfunc.

77a6d30

Fix loggingeventhandler meta.

f8fcba4

Fix utils use of ObjectList.

1bb52de

Hopefully fix stopCh use in metrics controller.

de4e164

velerobackup controller working.

f20706c

remotemachineset controller working.

136433a

Fix remaining controller Reconcile funcs to take context.

b8d6f77

Fix most of remaining controller MapFuncs.

cb0dfd2

Fix clusterdeploymentexpectations.

aa71993

High risk dns endpoint fixes.

dfcd411

Migrate operator hive controller.

f9b0df0

Migrate installmanager, hive now building.

35ebb86

Unit tests compiling but there are dns problems.

9d625f5

Fix dnsendpoint controller tests with customized comparison.

43ab4d3

Fix remaining dns tests.

19f6331

Migrate new awsprivatelink controller.

4e9e597

Cleanup for dns controllers and tests.

f8bb4e2

Fix e2e suite.

5d6952c

openshift-ci-robot assigned abhinavdahiya and joelddiaz Mar 26, 2021

openshift-ci-robot requested review from joelddiaz and suhanime March 26, 2021 17:14

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2021

abhinavdahiya reviewed Mar 26, 2021

View reviewed changes

Init struct with field names.

360d9a8

joelddiaz reviewed Mar 31, 2021

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2021

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

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

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

dgoodwin added 2 commits April 1, 2021 09:15

Merge remote-tracking branch 'up/master' into controller-runtime-0.7.3

7453827

More missed filed from merge.

78df2dd

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2021

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

joelddiaz reviewed Apr 1, 2021

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2021

openshift-merge-robot merged commit 7a0d639 into openshift:master Apr 2, 2021

Upgrade to controller-runtime 0.7.0 #1318

Upgrade to controller-runtime 0.7.0 #1318

Uh oh!

Conversation

dgoodwin commented Mar 26, 2021

Uh oh!

dgoodwin commented Mar 26, 2021

Uh oh!

dgoodwin commented Mar 26, 2021

Uh oh!

abhinavdahiya Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

dgoodwin Mar 30, 2021

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

dgoodwin commented Mar 30, 2021

Uh oh!

dgoodwin commented Mar 31, 2021

Uh oh!

joelddiaz left a comment

Choose a reason for hiding this comment

Uh oh!

dgoodwin commented Mar 31, 2021

Uh oh!

dgoodwin commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abhinavdahiya commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgoodwin commented Mar 31, 2021

Uh oh!

dgoodwin commented Mar 31, 2021

Uh oh!

openshift-bot commented Mar 31, 2021

Uh oh!

openshift-bot commented Mar 31, 2021

Uh oh!

openshift-bot commented Mar 31, 2021

Uh oh!

suhanime commented Mar 31, 2021

Uh oh!

dgoodwin commented Apr 1, 2021

Uh oh!

joelddiaz left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Apr 1, 2021

Uh oh!

openshift-bot commented Apr 1, 2021

Uh oh!

openshift-bot commented Apr 1, 2021

Uh oh!

openshift-bot commented Apr 1, 2021

Uh oh!

openshift-bot commented Apr 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dgoodwin commented Mar 31, 2021 •

edited

Loading

abhinavdahiya commented Mar 31, 2021 •

edited

Loading