Add direct resource `cloudbuildworkerpool` #1920

yuwenma · 2024-05-31T17:42:56Z

Change description

Fixes #

Tests you have done

Run make ready-pr to ensure this PR is ready for review.
Perform necessary E2E testing for changed resources.

justinsb

Looks good!

apis/cloudbuild/v1beta1/conversion.go

apis/cloudbuild/v1beta1/groupversion_info.go

apis/cloudbuild/v1beta1/workerpool_types.go

pkg/controller/direct/directbase/directbase_controller.go

pkg/controller/direct/references/computenetworkref.go

pkg/controller/direct/references/projectref.go

pkg/controller/registration/registration_controller.go

apis/refs/v1beta1/networkref.go

pkg/controller/direct/references/projectref.go

pkg/controller/direct/references/computenetworkref.go

pkg/controller/direct/references/projectref.go

yuwenma · 2024-06-04T00:04:38Z

pkg/controller/direct/cloudbuild/workerpool_controller.go

+	return &v
+}
+
+func ToDateDashTime(ts *timestamppb.Timestamp) *string {


I am using RFC3339 (ln392).

oh, are you suggesting to rename to ToRFC3339Time? The reason I call it ToDateDashTime is because the kubebuilder tag +kubebuilder:validation:Format=date-time. If using a different format, the CRD validation will return something like invalid format, expect date-time. But I don't have a preference.

pkg/controller/direct/cloudbuild/workerpool_controller.go

config/tests/samples/create/harness.go

apis/cloudbuild/v1beta1/groupversion_info.go

justinsb · 2024-06-05T10:25:48Z

apis/cloudbuild/v1beta1/workerpool_types.go

+	/* The creation timestamp of the workerpool.*/
+	// +optional
+	// +kubebuilder:validation:Format=date-time
+	CreateTime *string `json:"createTime,omitempty"`


Move to ObservedState?

Done.
(LLM keeps the createTime and updateTime under status. I know we cannot easily change that). Do you know how many beta resources have createTime and updateTime under status.observedState? If none, I would lean towards put them under status.

apis/cloudbuild/v1beta1/workerpool_types.go

justinsb · 2024-06-05T10:27:26Z

apis/cloudbuild/v1beta1/workerpool_types.go

+	// +kubebuilder:validation:Format=date-time
+	UpdateTime *string `json:"updateTime,omitempty"`
+
+	ResourceID *string `json:"resourceID,omitempty"`


I like this idea, but ... does naming it resourceID imply that the format will be the same as spec.resourceID? Should we call it status.link or status.selfLink or status.ref or ??? (selfLink might have collisions and might also have implications, so probably not that, unless we want to use that format)

I want to find a clear way to show the machine status telling if CC has successfully acquired the resource, or has created the resource according to the given spec.resourceId or metadate name.

I like selfLink, it is reliable and most accurate. But I want to use the selfLink from the GCP server ( not configconnector to build one). But the cloudbuild workerpool response does not have a selfLink.

justinsb · 2024-06-05T10:28:05Z

apis/cloudbuild/v1beta1/workerpool_types.go

+}
+
+type CloudBuildWorkerPoolObservedState struct {
+	// +required


Marking it as required in status might be limiting - we can never change this. But if it's required in the proto schema, it can't be changed there either (although technically they can change it with a v2, and then we'd be forced to do a v2, but maybe we'd be forced to do a v2 anyway in that case, so ....)

Yes, that's what I'm thinking about (if the gcp server changes the field in v2, we can then change it in v2). In this specific resource, I found some interesting usage of the required/optional that I want to discuss:

the workerConfig is required by the cloudbuild server, which means if we validate this in the CRD level, we can avoid the GCP requests for immutable field changes (what I did here).

the networkConfig is optional, but if provided, its networkConfig.peeredNetwork field is required (and immutable). Originally I set the networkConfig to optional and networkConfig.peeredNetwork to be required. But it does not work very well with gcp update (because it is immutable). So It feels like forcing required just adds the complexity without clear benefits. So I choose to leave that field optional and expect users to figure out the right config via the GCP errors

I also put these fields in .status.observedState, so if users change the GCP immutable field in .spec.XXX and got GCP error, they know what's the right config is (in status.observedState.XXX). And of course, Export is another option.

justinsb · 2024-06-05T11:18:26Z

pkg/controller/direct/cloudbuild/workerpool_controller.go

+	req := &cloudbuildpb.DeleteWorkerPoolRequest{Name: a.fullyQualifiedName(), AllowMissing: true}
+	op, err := a.gcpClient.DeleteWorkerPool(ctx, req)
+	if err != nil {
+		// likely a server bug. worker_pool can be successfully deleted.


do you mean the cloud.google.com lib

Right. I've seen this once before with an LRO with an invalid response AnyPb (no "value"). We should have a complete log in artifacts/ ... if you attach / DM me the delete portion of the log I can see if it matches what I saw before (and hopefully better remember it!)

IIRC we could work around this by switching from the REST client to the proto client (which is a one liner - NewClient vs NewRESTClient). Previously we couldn't log proto requests/responses, but this is a new resource, and we can log them now, so maybe that's an easy option.

justinsb · 2024-06-05T11:20:29Z

pkg/controller/direct/cloudbuild/workerpool_controller.go

+	return &v
+}
+
+func ToDateDashTime(ts *timestamppb.Timestamp) *string {


Ah I see where the name comes from! Maybe ToOpenAPIDateTime would be better in that case? I think the bigger question is whether we should preserve the full timestamp as provided to us (e.g. convert the timestamp to a string) ... i.e. does it matter that we drop precision (the nanos)

justinsb · 2024-06-05T11:21:15Z

pkg/controller/direct/references/computenetworkref.go

+				Project:          tokens[1],
+				ComputeNetworkID: tokens[4]}, nil
+		}
+		return nil, fmt.Errorf("format of computenetwork external=%q was not known (use projects/<projectId>/global/networks/<networkid>)", ref.External)


This is great ... such an improvement over the current status quo where if you get it wrong, you get really weird errors!

pkg/controller/direct/references/computenetworkref.go

justinsb · 2024-06-05T11:23:47Z

pkg/controller/direct/references/computenetworkref.go

+		computenetworkID = computenetwork.GetName()
+	}
+
+	projectID := ""


We might want a TODO: extract GetProject helper function!

Done.
(A lot of weirdness around projectID and resourceID)

justinsb · 2024-06-05T11:26:00Z

Looks really good. I think we're talking about the right things at this point, i.e. the details of how a KRM <-> GCP bridge should work. I do think starting with alpha lets us iterate more rapidly on these, but we could also just commit to getting it done in a single release (or even not publish this specific CRD until we are ready, e.g. by deleting it from the crd generation step)

justinsb · 2024-06-07T15:38:45Z

Awesome stuff!

/approve
/lgtm

(Sadly there's a merge conflict...)

justinsb · 2024-06-07T21:43:33Z

Thanks @yuwenma

/approve
/lgtm

google-oss-prow · 2024-06-07T21:43:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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

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

jingyih · 2024-06-20T19:58:15Z

Hi @yuwenma, the prow test for this resource is constantly failing. Do you mind take a look?

To reproduce:

$ go test -v -tags=integration ./pkg/controller/dynamic/ -test.run TestCreateNoChangeUpdateDelete -run-tests cloudbuildworkerpool

yuwenma · 2024-06-20T20:00:53Z

Hi @yuwenma, the prow test for this resource is constantly failing. Do you mind take a look?

To reproduce:
$ go test -v -tags=integration ./pkg/controller/dynamic/ -test.run TestCreateNoChangeUpdateDelete -run-tests cloudbuildworkerpool

for sure

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

yuwenma changed the title ~~Add cloudbuildworkerpool resource~~ Add direct resource cloudbuildworkerpool Jun 1, 2024

yuwenma force-pushed the cloudbuild-workerpool branch from e063990 to b588d2a Compare June 2, 2024 20:43

yuwenma marked this pull request as ready for review June 2, 2024 20:44

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

yuwenma force-pushed the cloudbuild-workerpool branch from cd1d9f2 to 12a9cd2 Compare June 2, 2024 22:50

justinsb reviewed Jun 3, 2024

View reviewed changes

yuwenma force-pushed the cloudbuild-workerpool branch from 12a9cd2 to cab5dbd Compare June 3, 2024 22:55

yuwenma commented Jun 4, 2024

View reviewed changes

yuwenma force-pushed the cloudbuild-workerpool branch 3 times, most recently from 4521f1e to 2183a70 Compare June 4, 2024 06:03

yuwenma commented Jun 4, 2024

View reviewed changes

config/tests/samples/create/harness.go Show resolved Hide resolved

yuwenma force-pushed the cloudbuild-workerpool branch from 2183a70 to ff9774f Compare June 4, 2024 08:04

yuwenma mentioned this pull request Jun 4, 2024

tests: Don't ignore http log with direct reconcilers #1924

Merged

yuwenma force-pushed the cloudbuild-workerpool branch 8 times, most recently from 70624d0 to 25f5a18 Compare June 5, 2024 05:26

justinsb reviewed Jun 5, 2024

View reviewed changes

yuwenma force-pushed the cloudbuild-workerpool branch 5 times, most recently from adcaaa7 to e4d2f72 Compare June 7, 2024 11:17

google-oss-prow bot assigned justinsb Jun 7, 2024

google-oss-prow bot added lgtm approved and removed lgtm labels Jun 7, 2024

yuwenma force-pushed the cloudbuild-workerpool branch from 2481da5 to 02e2717 Compare June 7, 2024 19:42

yuwenma added 11 commits June 7, 2024 20:31

Register direct controller from crd annotation

eff651b

Add mockgcp for cloudbuild workerpool

3f241e4

Add SciFi for CloudBuild workerpool

ba504b0

autogenerated from 'make ready-pr'

f648b49

register direct controller to cc reconciler

dfb7d64

resolve merge conflict

6f266dc

resolve comments; update mock test

9d4d6f6

re-enable mock check on serviceusage

288b477

force llm projectRef.Kind. uhhhh

f730acd

skip cloudbuildworkerpool from tf servicemap check

3788b45

no parent; mockgcp with StartLRO

7db0d9a

yuwenma force-pushed the cloudbuild-workerpool branch from 02e2717 to 8e01714 Compare June 7, 2024 20:35

move cloudbuildworkerpool to alpha; other crd changes

85a3d1a

yuwenma force-pushed the cloudbuild-workerpool branch from 8e01714 to 85a3d1a Compare June 7, 2024 20:57

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

google-oss-prow bot merged commit afd378f into GoogleCloudPlatform:master Jun 7, 2024
14 checks passed

This was referenced Jun 16, 2024

chore: change CBWP required status field to optional #2018

Merged

chore: change CBWP 'Ip' to 'IP' #2019

Merged

feat: add ExternalReference (selfLink) with GCP update/deletion uniqueness guardrail #2020

Merged

yuwenma added this to the 1.119 milestone Jun 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add direct resource `cloudbuildworkerpool` #1920

Add direct resource `cloudbuildworkerpool` #1920

yuwenma commented May 31, 2024

justinsb left a comment

yuwenma Jun 4, 2024

justinsb Jun 5, 2024

yuwenma Jun 7, 2024

yuwenma Jun 7, 2024

justinsb Jun 5, 2024

yuwenma Jun 7, 2024

justinsb Jun 5, 2024

yuwenma Jun 7, 2024

justinsb Jun 5, 2024

justinsb Jun 5, 2024

justinsb Jun 5, 2024

justinsb Jun 5, 2024

yuwenma Jun 7, 2024

justinsb commented Jun 5, 2024

justinsb commented Jun 7, 2024 •

edited

Loading

justinsb commented Jun 7, 2024

google-oss-prow bot commented Jun 7, 2024

jingyih commented Jun 20, 2024

yuwenma commented Jun 20, 2024

Add direct resource cloudbuildworkerpool #1920

Add direct resource cloudbuildworkerpool #1920

Conversation

yuwenma commented May 31, 2024

Change description

Tests you have done

justinsb left a 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

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

justinsb commented Jun 5, 2024

justinsb commented Jun 7, 2024 • edited Loading

justinsb commented Jun 7, 2024

google-oss-prow bot commented Jun 7, 2024

jingyih commented Jun 20, 2024

yuwenma commented Jun 20, 2024

Add direct resource `cloudbuildworkerpool` #1920

Add direct resource `cloudbuildworkerpool` #1920

justinsb commented Jun 7, 2024 •

edited

Loading