Bug 1807471: add always-on controller for image-registry-certificates #507

dmage · 2020-03-31T11:57:55Z

No description provided.

dmage · 2020-04-01T12:03:04Z

NodeControllerDegraded: The master nodes not ready: node \"ip-10-0-133-125.us-west-2.compute.internal\" not ready since 2020-03-31 17:31:49 +0000 UTC because NodeStatusUnknown (Kubelet stopped posting node status.)\nEtcdMembersDegraded: ip-10-0-133-125.us-west-2.compute.internal members are unhealthy,  members are unknown

/retest

dmage · 2020-04-01T13:08:03Z

/test e2e-aws-operator

openshift-ci-robot · 2020-04-02T12:13:01Z

@dmage: This pull request references Bugzilla bug 1807471, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1807471: add always-on controller for image-registry-certificates

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.

dmage · 2020-04-03T11:38:22Z

/retest

dmage · 2020-04-03T19:29:51Z

/retest

dmage · 2020-04-07T12:34:38Z

/assign @adambkaplan @ricardomaraschini

ricardomaraschini

Looking good.

ricardomaraschini · 2020-04-07T12:59:20Z

pkg/operator/controller.go

+		kubeInformerFactory.Core().V1().ConfigMaps(),
+		kubeInformerFactory.Core().V1().Services(),


Can't we use a single parameter here of type v1.Interface?

I try to use the upstream approach for controllers. I think their code is cleaner and we have a lot to learn from them.

https://github.com/kubernetes/kubernetes/blob/3758426884e3c82cbd99c72e8015f4396f21fde2/cmd/kube-controller-manager/app/apps.go#L40

In this case v1.Interface instead of two informers will be much more opaque dependency. I don't want to read this code few month later and think:

Do it really wants to use all corev1 types?! I don't think so, but what kind of objects does this controller really need? Meh, I need to read its code...

ricardomaraschini · 2020-04-07T13:02:31Z

pkg/operator/imageregistrycertificates.go

+}
+
+func (c *ImageRegistryCertificatesController) sync() error {
+	g := resource.NewGeneratorCAConfig(c.configMapLister, c.imageConfigLister, c.openshiftConfigLister, c.serviceLister, c.coreClient)


Could this be done when creating the object on NewImageRegistryCertificatesController() ?

Generators are lightweight objects (i.e. their constructors are trivial) and in this case it should be created on stack, so it doesn't matter. Eventually I'd expect us to get rid of these generators and inline them into controllers.

ricardomaraschini · 2020-04-07T13:10:13Z

test/framework/imageregistry.go

+		te.Fatalf("unable to get the current state of daemonset/node-ca: %s", err)
+	}
+
+	policy := metav1.DeletePropagationBackground


Isn't it possible to use DeletePropagationForeground here so we don't need to wait with WaitUntilFinalized()?

DeletePropagationForeground doesn't wait the object is finalized anyway, it's an instruction for GC. For daemonsets deleting its children can be slow. I'd prefer it to stay as a background job.

ricardomaraschini · 2020-04-08T09:08:51Z

test/e2e/graceful_shutdown_test.go

@@ -27,8 +34,6 @@ func TestNodeCAGracefulShutdown(t *testing.T) {
 		t.Fatalf("unable to list pods: %v", err)
 	}



I was looking at the error being reported by CI on this test and it seems like we could not find any pod to attach to(that would be very strange). Would you mind incorporating this slight change to make things clear?

Suggested change

if len(pods.Items) == 0 {

t.Fatalf("no node-ca pods found")

}

dmage · 2020-04-08T13:08:46Z

@ricardomaraschini ptal again

ricardomaraschini

/lgtm

openshift-ci-robot · 2020-04-08T13:43:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmage, ricardomaraschini

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~~ [dmage]

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

openshift-ci-robot · 2020-04-08T13:46:44Z

@dmage: All pull requests linked via external trackers have merged: openshift/cluster-image-registry-operator#507. Bugzilla bug 1807471 has been moved to the MODIFIED state.

In response to this:

Bug 1807471: add always-on controller for image-registry-certificates

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.

adambkaplan · 2020-04-09T17:21:05Z

/cherry-pick release-4.4

openshift-cherrypick-robot · 2020-04-09T17:21:51Z

@adambkaplan: #507 failed to apply on top of branch "release-4.4":

Applying: Add always-on controller for image-registry-certificates
Using index info to reconstruct a base tree...
A	pkg/defaults/defaults.go
M	pkg/operator/controller.go
A	pkg/operator/nodecadaemon.go
M	pkg/resource/caconfig.go
M	pkg/resource/generator.go
A	test/e2e/nodecadaemon_test.go
M	test/e2e/recreate_deployment_test.go
M	test/framework/daemonset.go
M	test/framework/imageregistry.go
Falling back to patching base and 3-way merge...
Auto-merging test/framework/imageregistry.go
CONFLICT (content): Merge conflict in test/framework/imageregistry.go
Auto-merging test/framework/daemonset.go
Auto-merging test/e2e/recreate_deployment_test.go
CONFLICT (content): Merge conflict in test/e2e/recreate_deployment_test.go
CONFLICT (modify/delete): test/e2e/nodecadaemon_test.go deleted in HEAD and modified in Add always-on controller for image-registry-certificates. Version Add always-on controller for image-registry-certificates of test/e2e/nodecadaemon_test.go left in tree.
Auto-merging pkg/resource/generator.go
CONFLICT (content): Merge conflict in pkg/resource/generator.go
Auto-merging pkg/resource/caconfig.go
CONFLICT (content): Merge conflict in pkg/resource/caconfig.go
CONFLICT (modify/delete): pkg/operator/nodecadaemon.go deleted in HEAD and modified in Add always-on controller for image-registry-certificates. Version Add always-on controller for image-registry-certificates of pkg/operator/nodecadaemon.go left in tree.
Auto-merging pkg/operator/controller.go
CONFLICT (content): Merge conflict in pkg/operator/controller.go
Auto-merging defaults/defaults.go
CONFLICT (content): Merge conflict in defaults/defaults.go
error: Failed to merge in the changes.
Patch failed at 0002 Add always-on controller for image-registry-certificates

In response to this:

/cherry-pick release-4.4

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.

openshift-ci-robot requested review from coreydaley and gabemontero March 31, 2020 11:58

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

dmage force-pushed the nodecadaemon branch from ad721a6 to a14eaf9 Compare March 31, 2020 16:15

dmage force-pushed the nodecadaemon branch from a14eaf9 to fc595bd Compare April 2, 2020 12:04

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2020

dmage force-pushed the nodecadaemon branch from fc595bd to 79253b7 Compare April 2, 2020 12:11

dmage changed the title ~~WIP~~ Add always-on controller for image-registry-certificates Apr 2, 2020

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2020

dmage changed the title ~~Add always-on controller for image-registry-certificates~~ Bug 1807471: add always-on controller for image-registry-certificates Apr 2, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 2, 2020

Report about missing resources only when verbose logging is on

ce3ad1c

dmage force-pushed the nodecadaemon branch from 79253b7 to 360ed54 Compare April 3, 2020 11:40

dmage force-pushed the nodecadaemon branch from cf6e7b2 to 5c78c5f Compare April 6, 2020 09:42

dmage added 2 commits April 6, 2020 15:30

Add always-on controller for image-registry-certificates

281b81e

Yet another fix for TestOperandProxyConfiguration

fcf910c

dmage force-pushed the nodecadaemon branch from 5c78c5f to 4240254 Compare April 6, 2020 13:31

openshift-ci-robot assigned adambkaplan and ricardomaraschini Apr 7, 2020

ricardomaraschini reviewed Apr 7, 2020

View reviewed changes

ricardomaraschini reviewed Apr 8, 2020

View reviewed changes

dmage added 3 commits April 8, 2020 12:18

Fix TestNodeCAGracefulShutdown

aa0542f

Don't use MustNewClientset in tests

3ae488a

Improve test error reporting

7de07c8

dmage force-pushed the nodecadaemon branch from 989cf66 to 7de07c8 Compare April 8, 2020 10:19

ricardomaraschini approved these changes Apr 8, 2020

View reviewed changes

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

openshift-merge-robot merged commit 062f6f9 into openshift:master Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1807471: add always-on controller for image-registry-certificates #507

Bug 1807471: add always-on controller for image-registry-certificates #507

dmage commented Mar 31, 2020

dmage commented Apr 1, 2020

dmage commented Apr 1, 2020

openshift-ci-robot commented Apr 2, 2020

dmage commented Apr 3, 2020

dmage commented Apr 3, 2020

dmage commented Apr 7, 2020

ricardomaraschini left a comment

ricardomaraschini Apr 7, 2020

dmage Apr 7, 2020

ricardomaraschini Apr 7, 2020

dmage Apr 7, 2020

ricardomaraschini Apr 7, 2020

dmage Apr 7, 2020

ricardomaraschini Apr 8, 2020

dmage commented Apr 8, 2020

ricardomaraschini left a comment

openshift-ci-robot commented Apr 8, 2020

openshift-ci-robot commented Apr 8, 2020

adambkaplan commented Apr 9, 2020

openshift-cherrypick-robot commented Apr 9, 2020

		kubeInformerFactory.Core().V1().ConfigMaps(),
		kubeInformerFactory.Core().V1().Services(),

		@@ -27,8 +34,6 @@ func TestNodeCAGracefulShutdown(t *testing.T) {
		t.Fatalf("unable to list pods: %v", err)
		}

+	if len(pods.Items) == 0 {
+		t.Fatalf("no node-ca pods found")
+	}

Bug 1807471: add always-on controller for image-registry-certificates #507

Bug 1807471: add always-on controller for image-registry-certificates #507

Conversation

dmage commented Mar 31, 2020

dmage commented Apr 1, 2020

dmage commented Apr 1, 2020

openshift-ci-robot commented Apr 2, 2020

dmage commented Apr 3, 2020

dmage commented Apr 3, 2020

dmage commented Apr 7, 2020

ricardomaraschini left a comment

Choose a reason for hiding this comment

ricardomaraschini Apr 7, 2020

Choose a reason for hiding this comment

dmage Apr 7, 2020

Choose a reason for hiding this comment

ricardomaraschini Apr 7, 2020

Choose a reason for hiding this comment

dmage Apr 7, 2020

Choose a reason for hiding this comment

ricardomaraschini Apr 7, 2020

Choose a reason for hiding this comment

dmage Apr 7, 2020

Choose a reason for hiding this comment

ricardomaraschini Apr 8, 2020

Choose a reason for hiding this comment

dmage commented Apr 8, 2020

ricardomaraschini left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 8, 2020

openshift-ci-robot commented Apr 8, 2020

adambkaplan commented Apr 9, 2020

openshift-cherrypick-robot commented Apr 9, 2020