Add namespaces managed by CNO to related objects field #199

rcarrillocruz · 2019-06-14T12:38:22Z

This allows must-gather tool to give information about CNO objects
for free.

danwinship · 2019-06-14T14:20:47Z

pkg/controller/statusmanager/status_manager.go

+	related := []configv1.ObjectReference{
+		{
+			Resource: "namespaces",
+			Name:     "openshift-sdn",


(Aside: is "namespaces" correct? I would have expected it to want the singular.)

This should be computed dynamically based on the actual Namespaces we created. The same way that ReconcileOperConfig figures out the set of active DaemonSets and Deployments and then tells the StatusManager about them, it should do the same with Namespaces. (Except it should add the controller's namespace to the list too.)

Plural seems fine, I found it in other spots of the code:

https://github.com/openshift/machine-api-operator/blob/6066765d8ff090fd081c7a20e59955d00004b0ad/pkg/operator/status.go#L165
https://github.com/openshift/cluster-samples-operator/blob/df3dbf4d8a119058f80067eca568c6f78ff8d725/pkg/operatorstatus/operatorstatus.go#L169
https://github.com/openshift/cluster-autoscaler-operator/blob/62768a6ba48010d12c4d86b8b06689a6970a7841/pkg/operator/operator.go#L110

Plural is correct, because this is a Resource, not a Kind. However, dan is right, this should be computed automatically.

danwinship · 2019-06-24T13:34:03Z

pkg/controller/statusmanager/status_manager.go

+		log.Printf("Failed to get ClusterOperator %q: %v", status.name, err)
+		return
+	}
+	co.Status.RelatedObjects = objects


This is just modifying the copy that you just fetched, which will be GC'ed as soon as this function returns.

You can just store the objects in a field on status here, and then set them on the object in Set

danwinship · 2019-07-08T17:06:11Z

@rcarrillocruz good except now you're never copying status.relatedObjects over to co.Status.RelatedObjects.

Also, please squash your commits rather than doing fixups. It's generally easier to deal with if the PR is always in a ready-to-be-merged form.

rcarrillocruz · 2019-07-10T19:19:16Z

/retest

danwinship · 2019-07-10T20:26:59Z

Doh. Sorry, I guess I wasn't clear. SetRelatedObjects should just set a field on the StatusManager object, like in the previous version, and then Set should copy that into the object being updated, like in the original version except with the dynamically-generated list (co.Status.RelatedObjects = status.relatedObjects)

squeed · 2019-07-11T16:53:53Z

pkg/controller/operconfig/operconfig_controller.go

 		}
+		relatedObjects = append(relatedObjects, configv1.ObjectReference{
+			Group:     obj.GetObjectKind().GroupVersionKind().Group,
+			Resource:  obj.GetKind(),


This isn't right - the object kind is "Namespace", whereas the API resource is "namespaces". You need to actually look up the conversion between the Kind and the Resource using the the RESTMapper.

It's a bit awkward, yes. It looks like a few other operators do it. See if they hard-code the mapping or do something cleverer.

rcarrillocruz · 2019-07-12T11:58:46Z

/retest

squeed · 2019-07-12T12:42:53Z

pkg/controller/operconfig/operconfig_controller.go

 	}
 	objs = append([]*uns.Unstructured{app}, objs...)

+	config, err := rest.InClusterConfig()


No need to create your own RestMapper - just get it from the manager.

squeed · 2019-07-12T12:43:31Z

pkg/controller/operconfig/operconfig_controller.go

 		}
+		restMapping, err := mapper.RESTMapping(obj.GroupVersionKind().GroupKind())
+		if err != nil {
+			log.Printf("Failed to get REST mapping:%v", err)


needs a continue here, and maybe a bit more useful error message.

squeed · 2019-07-12T12:45:35Z

Two small comments, then this should be good to go.

squeed · 2019-07-15T19:29:32Z

/approve

This adds all objects to the Related field. I think that's fine - thoughts, @danwinship?

This allows must-gather tool to give information about CNO objects for free.

danwinship · 2019-07-16T12:49:32Z

This adds all objects to the Related field. I think that's fine - thoughts, @danwinship?

That was the plan, wasn't it? Then we can use that to figure out what we need to clean up?

The API documentation is not detailed enough to either confirm or deny that this is OK.

squeed · 2019-07-16T14:25:04Z

/lgtm

openshift-ci-robot · 2019-07-16T14:25:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rcarrillocruz, squeed

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

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 added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 14, 2019

openshift-ci-robot requested review from danwinship and squeed June 14, 2019 12:39

danwinship requested changes Jun 14, 2019

View reviewed changes

danwinship reviewed Jun 24, 2019

View reviewed changes

rcarrillocruz force-pushed the add_namespaces_to_related_objects branch from f5678bd to 6d0e4bb Compare July 2, 2019 13:44

rcarrillocruz force-pushed the add_namespaces_to_related_objects branch from 6d0e4bb to ef1141d Compare July 10, 2019 17:32

rcarrillocruz force-pushed the add_namespaces_to_related_objects branch from ef1141d to 16dd37f Compare July 11, 2019 16:10

squeed reviewed Jul 11, 2019

View reviewed changes

rcarrillocruz force-pushed the add_namespaces_to_related_objects branch from 16dd37f to e306aa9 Compare July 12, 2019 10:38

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 12, 2019

squeed reviewed Jul 12, 2019

View reviewed changes

rcarrillocruz force-pushed the add_namespaces_to_related_objects branch 2 times, most recently from 1af0bae to efa160e Compare July 12, 2019 15:24

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

Add namespaces managed by CNO to related objects field

79ee4fc

This allows must-gather tool to give information about CNO objects for free.

rcarrillocruz force-pushed the add_namespaces_to_related_objects branch from efa160e to 79ee4fc Compare July 16, 2019 07:57

openshift-ci-robot assigned squeed Jul 16, 2019

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

openshift-merge-robot merged commit f86fe2f into openshift:master Jul 16, 2019

Add namespaces managed by CNO to related objects field #199

Add namespaces managed by CNO to related objects field #199

Uh oh!

Conversation

rcarrillocruz commented Jun 14, 2019

Uh oh!

danwinship Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

rcarrillocruz Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

squeed Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

danwinship Jun 24, 2019

Choose a reason for hiding this comment

Uh oh!

danwinship commented Jul 8, 2019

Uh oh!

rcarrillocruz commented Jul 10, 2019

Uh oh!

danwinship commented Jul 10, 2019

Uh oh!

squeed Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

rcarrillocruz commented Jul 12, 2019

Uh oh!

squeed Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

squeed Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

squeed commented Jul 12, 2019

Uh oh!

squeed commented Jul 15, 2019

Uh oh!

danwinship commented Jul 16, 2019

Uh oh!

squeed commented Jul 16, 2019

Uh oh!

openshift-ci-robot commented Jul 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants