feat: implement direct controller for MonitoringDashboard #1863

justinsb · 2024-05-21T16:28:36Z

feat: implement direct controller for MonitoringDashboard
chore: update monitoringdashboard golden output

justinsb · 2024-05-22T14:22:29Z

I think we should merge this after we have more tests (at least #1806) - then I can also make the ref-test real

justinsb · 2024-06-06T11:26:55Z

...ure/testdata/basic/monitoring/v1beta1/monitoringdashboard/monitoringdashboardbasic/_http.log

        "widgets": [
          {
            "title": "Widget 1",
            "xyChart": {
              "dataSets": [
                {
-                  "plotType": "LINE",
+                  "plotType": 1,


Kind of annoying, but the official client uses integer enum encoding, messing up our diffs. I can try the proto form and see if it is any better.

I missed this. Please ignore my other comment.

justinsb · 2024-06-06T11:27:37Z

...ure/testdata/basic/monitoring/v1beta1/monitoringdashboard/monitoringdashboardbasic/_http.log

 Content-Type: application/json
-User-Agent: kcc/controller-manager DeclarativeClientLib/0.0.1


I think the diff is confused here because we don't do a GET before DELETE. Maybe we should just do the GET!

I added a semi-superfluous GET to delete, the diff now looks much better!

yuwenma · 2024-06-07T06:12:47Z

pkg/controller/direct/monitoring/monitoringdashboard_controller.go

+
+func newDashboardModel(config *controller.Config) directbase.Model {
+	ctx := context.TODO()
+	gcpClient, err := newGCPClient(ctx, config)


or we can move this to AdapterForObject so as we can use the passed in context.Context parameter.
The AdapterForObject will be initialized in each Reconcile loop.

My one concern is that building the client may be expensive: if we have to open a new TCP connection, or if we have to get the token again. But I don't want to block this PR on it, so I'll move it!

Ah, I guess we're calling the expensive call every time anyway :-)

scripts/github-actions/tests-e2e-direct.sh

...ture/testdata/basic/monitoring/v1beta1/monitoringdashboard/monitoringdashboardrefs/_http.log

yuwenma · 2024-06-07T06:18:31Z

...ingdashboard/monitoringdashboardbasic/_generated_object_monitoringdashboardbasic.golden.yaml

@@ -3,7 +3,6 @@ kind: MonitoringDashboard
 metadata:
  annotations:
    cnrm.cloud.google.com/management-conflict-prevention-policy: none
-    cnrm.cloud.google.com/state-into-spec: merge


yuwenma · 2024-06-07T06:23:02Z

pkg/controller/direct/references/resourceid.go

+)
+
+func GetResourceID(u *unstructured.Unstructured) (string, error) {
+	resourceID, _, err := unstructured.NestedString(u.Object, "spec", "resourceID")


I recalled to see a team discussion about the camel case for projectId rather than projectID. Shall this be resourceId or resourceID?
(Not a blocker to this PR, just want to align the names here since this is a shareable function under direct/references)

So ... go says "ID". KRM style guide says "ID". Google APIs use "Id" (in JSON). Most KCC fields use Id, but I guess here we use resourceID.

If the KCC field was called resourceId, I don't know whether I would follow go conventions or try to match the name of the field.

In this case though, we're reading a field, and I think that field's name is resourceID. That's correct against KRM style guide, but possibly inconsistent with most other GCP-generated fields.

I think it's your decision @yuwenma , and it's going to be interesting!

(Our best bet might be to stop adding fields that end in Id at all, maybe it's another marker that something is actually a ref ... or we could just drop ID arguing that it is the "type" of the field, and we don't say "fooString" so why do we say "fooID" or "fooURL" ? )

yuwenma · 2024-06-07T06:28:14Z

pkg/controller/direct/monitoring/refs.go

+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func normalizeResourceName(ctx context.Context, reader client.Reader, src client.Object, ref *v1alpha1.ResourceRef) (*v1alpha1.ResourceRef, error) {


Do you want to move this to controller/direct/reference/ or something that can be sharable? I feel like this is a good piece to reuse and continue improving as we building up more resources.

Yes, but my "rule of thumb" is to move it when we have two copies. For this particular one, we only have one copy (this is actually very specialized - there's a field called "resourceName" in MonitoringDashboard. Note that this method does call into shared functions for most of its logic!

Actually I guess this is resolving a resource of arbitrary kind, so is more reusable than I first thought. Still, it's our first ... let's move it when we have two or three instances.

yuwenma · 2024-06-07T06:33:30Z

pkg/controller/direct/monitoring/utils.go

+func setStatus(u *unstructured.Unstructured, typedStatus any) error {
+	status, err := runtime.DefaultUnstructuredConverter.ToUnstructured(typedStatus)
+	if err != nil {
+		return fmt.Errorf("error converting status to unstructured: %w", err)
+	}
+
+	// Use existing values for conditions/observedGeneration; they are managed in k8s not the GCP API
+	old, _, _ := unstructured.NestedMap(u.Object, "status")
+	if old != nil {
+		status["conditions"] = old["conditions"]
+		status["observedGeneration"] = old["observedGeneration"]
+	}
+
+	u.Object["status"] = status
+
+	return nil
+}
+


qq: do you think we shall move this to the direct_controllerReconcile?

We should move it, along with most/all of the functions in utils. I just don't think we know where to move it to yet!

yuwenma · 2024-06-13T00:40:29Z

/lgtm
/approve

I think the presubmit should be fixed in #1979. Could you rebase this PR and re-write the golden logs? It looks great!

google-oss-prow · 2024-06-13T00:40:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yuwenma

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

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

yuwenma · 2024-06-18T03:02:47Z

/lgtm

google-oss-prow bot added the do-not-merge/work-in-progress label May 21, 2024

justinsb force-pushed the monitoring_dashboard_controller branch from 165c67e to bde11ea Compare May 22, 2024 14:22

justinsb force-pushed the monitoring_dashboard_controller branch from bde11ea to b8be512 Compare May 22, 2024 14:25

justinsb force-pushed the monitoring_dashboard_controller branch 6 times, most recently from a5fc35d to 25883aa Compare June 6, 2024 11:15

justinsb commented Jun 6, 2024

View reviewed changes

justinsb changed the title ~~WIP: feat: implement direct controller for MonitoringDashboard~~ feat: implement direct controller for MonitoringDashboard Jun 6, 2024

google-oss-prow bot removed the do-not-merge/work-in-progress label Jun 6, 2024

justinsb force-pushed the monitoring_dashboard_controller branch from 5b333d9 to 9ac8098 Compare June 6, 2024 15:33

yuwenma reviewed Jun 7, 2024

View reviewed changes

justinsb force-pushed the monitoring_dashboard_controller branch 3 times, most recently from 3bdaf3f to a680eec Compare June 7, 2024 18:29

google-oss-prow bot assigned yuwenma Jun 13, 2024

google-oss-prow bot added the lgtm label Jun 13, 2024

google-oss-prow bot added the approved label Jun 13, 2024

justinsb force-pushed the monitoring_dashboard_controller branch from a680eec to 0f25e8c Compare June 17, 2024 19:36

google-oss-prow bot removed the lgtm label Jun 17, 2024

justinsb force-pushed the monitoring_dashboard_controller branch from 0f25e8c to 6d3b114 Compare June 17, 2024 22:35

justinsb added 2 commits June 17, 2024 21:59

feat: implement direct controller for MonitoringDashboard

66b5173

tests: Add monitoringdashboard to tests-e2e-direct

a275ba9

tests: update golden test data for MonitoringDashboard direct controller

81db68b

justinsb force-pushed the monitoring_dashboard_controller branch from 6d3b114 to 81db68b Compare June 18, 2024 02:00

google-oss-prow bot added the lgtm label Jun 18, 2024

yuwenma added this to the 1.119 milestone Jun 18, 2024

google-oss-prow bot merged commit 27c6cbf into GoogleCloudPlatform:master Jun 18, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement direct controller for MonitoringDashboard #1863

feat: implement direct controller for MonitoringDashboard #1863

justinsb commented May 21, 2024 •

edited

Loading

justinsb commented May 22, 2024

justinsb Jun 6, 2024

yuwenma Jun 7, 2024 •

edited

Loading

justinsb Jun 6, 2024

justinsb Jun 6, 2024

yuwenma Jun 7, 2024

justinsb Jun 7, 2024

justinsb Jun 7, 2024

yuwenma Jun 7, 2024

yuwenma Jun 7, 2024

justinsb Jun 7, 2024

yuwenma Jun 7, 2024 •

edited

Loading

justinsb Jun 7, 2024

justinsb Jun 7, 2024

yuwenma Jun 7, 2024

justinsb Jun 7, 2024

yuwenma commented Jun 13, 2024 •

edited

Loading

google-oss-prow bot commented Jun 13, 2024

yuwenma commented Jun 18, 2024

		Content-Type: application/json
		User-Agent: kcc/controller-manager DeclarativeClientLib/0.0.1

feat: implement direct controller for MonitoringDashboard #1863

feat: implement direct controller for MonitoringDashboard #1863

Conversation

justinsb commented May 21, 2024 • edited Loading

justinsb commented May 22, 2024

Choose a reason for hiding this comment

yuwenma Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwenma Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwenma commented Jun 13, 2024 • edited Loading

google-oss-prow bot commented Jun 13, 2024

yuwenma commented Jun 18, 2024

justinsb commented May 21, 2024 •

edited

Loading

yuwenma Jun 7, 2024 •

edited

Loading

yuwenma Jun 7, 2024 •

edited

Loading

yuwenma commented Jun 13, 2024 •

edited

Loading