docs: Update workflow for resource migration with reconciler annotation #2860

jasonvigil · 2024-10-04T22:08:38Z

This is the first step in removing KCC_USE_DIRECT_RECONCILERS, and switching over to using the user-configurable annotation instead.

This lets us simultaneously test/review the behavior of legacy TF/DCL-based controllers and the new direct controllers.

google-oss-prow · 2024-10-04T22:08:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jasonvigil. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

acpana

small comments but overall looks great, thanks for making these changes 🙏🏼

acpana · 2024-10-07T17:48:41Z

docs/develop-resources/guides/4-add-controller.md

+Copy all existing test cases and add `-direct` suffix to the test names.
+


Let's offer an example maybe of what the final folder will look like here?

Good call, added an example

acpana · 2024-10-07T17:50:57Z

docs/develop-resources/guides/4-add-controller.md

+
+This will override the `cnrm.cloud.google.com/dcl2crd: "true"` or `cnrm.cloud.google.com/tf2crd: "true"` annotations in the CRD and enable the direct controller for the new test cases. The previously-existing test cases will continue to use the TF/DCL-based controller.
+
+Verify the new `-direct` test cases have equivalent behavior to the existing test cases (though not necessarily the exact same API interactions, if the direct controller achieves the same end result differently than the TF/DCL-based controller).


As an external contributor, how do I verify this 🙃 ? Would it be sufficient for now that the new tests pass?

Personally, I feel like it's impossible to expect external contributers to develop new controllers with only mock tests. They will need to verify behavior with real gcp, using hack/record-gcp.

How about suggesting users to use git mv when creating the -direct test cases. So that the PR git-diff shows whether it gives equivalent behavior or not.

acpana · 2024-10-07T17:51:10Z

docs/develop-resources/guides/4-add-controller.md

Suggested change

* The roundtrip fuzz tests shall cover all the fields in `spec `and `status.observedState `fields [example](https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/0bbac86ace6ab2f4051b574f026d5fe47fa05b75/pkg/controller/direct/redis/cluster/roundtrip_test.go#L92)

* The roundtrip fuzz tests covers all the fields in `spec `and `status.observedState `fields [example](https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/0bbac86ace6ab2f4051b574f026d5fe47fa05b75/pkg/controller/direct/redis/cluster/roundtrip_test.go#L92)

This seems totally unrelated, no? Created #2866.

What is this change about? I apply RFC2119 on the wordings for should and shall here.

This is the first step in removing KCC_USE_DIRECT_RECONCILERS, and switching over to using the user-configurable annotation instead. This lets us simultaneously test/review the behavior of legacy TF/DCL-based controllers and the new direct controllers.

yuwenma · 2024-10-08T14:06:28Z

docs/develop-resources/guides/5-releases.md

-### For DCL-based Beta resource
-
-* Remove the `cnrm.cloud.google.com/dcl2crd: "true"` label from the CRD will turn on SciFi controller.
+* Remove all of the duplicate `-direct` mockgcp test cases, and re-record mockgcp interactions (now using direct controller instead of legacy TF/DCL).


I think we want to make it clear that the transient -direct test cases are only required for Beta TF/DCL migration

yuwenma · 2024-10-08T14:07:40Z

docs/develop-resources/guides/5-releases.md


-### For TF-based Beta resource
+* Remove the `cnrm.cloud.google.com/dcl2crd: "true"` or `cnrm.cloud.google.com/tf2crd: "true"` go tag from the CRD struct, and run `dev/tasks/generate-crds` to use Direct as the permanent controller.


I like the wording changes, can we also keep the example? Otherwise it could be hard for contributors to figure out what the CRD struct or API refers to.

yuwenma · 2024-10-08T14:09:29Z

docs/develop-resources/guides/5-releases.md

@@ -1,14 +1,10 @@
 # 5. Release

-## 5.1 Turn on your Direct controller (TF/DCL Beta Only)
+## 5.1 Make Direct controller the default (for migration from TF/DCL)


You may want to keep the Beta only because Alpha TF/DCL migration does not require changing the tf2crd or dcl2crd labels.

yuwenma · 2024-10-08T14:11:36Z

docs/develop-resources/scenarios/migrate-tf-resource-alpha.md

 ### PR Reviews

 * We require the roundtrip fuzz tests to cover all the fields in `spec` and `status.observedState` fields [example](https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/0bbac86ace6ab2f4051b574f026d5fe47fa05b75/pkg/controller/direct/redis/cluster/roundtrip_test.go#L92) (For mapper)
-* We require removing the `cnrm.cloud.google.com/dcl2crd: "true"` or `cnrm.cloud.google.com/tf2crd: "true"` tags from the API and the presubmit test passes.
+* We require duplicating all existing test cases (by copying and adding `-direct` suffix to test names), and enabling the direct controller in the new test cases (by setting the `alpha.cnrm.cloud.google.com/reconciler: direct` annotation).


Suggest giving more detailed guidance so we can verify this, like "using git mv to ..."

yuwenma · 2024-10-08T14:14:19Z

docs/develop-resources/guides/4-add-controller.md

+
+This will override the `cnrm.cloud.google.com/dcl2crd: "true"` or `cnrm.cloud.google.com/tf2crd: "true"` annotations in the CRD and enable the direct controller for the new test cases. The previously-existing test cases will continue to use the TF/DCL-based controller.
+
+Verify the new `-direct` test cases have equivalent behavior to the existing test cases (though not necessarily the exact same API interactions, if the direct controller achieves the same end result differently than the TF/DCL-based controller).


How about suggesting users to use git mv when creating the -direct test cases. So that the PR git-diff shows whether it gives equivalent behavior or not.

jasonvigil assigned yuwenma Oct 4, 2024

acpana reviewed Oct 7, 2024

View reviewed changes

jasonvigil mentioned this pull request Oct 7, 2024

fix: Direct controller exit criteria grammar #2866

Open

jasonvigil force-pushed the remove-KCC_USE_DIRECT_RECONCILERS branch from 9a860fd to 4c5d00b Compare October 7, 2024 18:06

jasonvigil requested a review from acpana October 7, 2024 18:06

yuwenma reviewed Oct 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: Update workflow for resource migration with reconciler annotation #2860

docs: Update workflow for resource migration with reconciler annotation #2860

jasonvigil commented Oct 4, 2024

google-oss-prow bot commented Oct 4, 2024

acpana left a comment

acpana Oct 7, 2024

jasonvigil Oct 7, 2024

acpana Oct 7, 2024

jasonvigil Oct 7, 2024

yuwenma Oct 8, 2024

acpana Oct 7, 2024

jasonvigil Oct 7, 2024

yuwenma Oct 8, 2024

yuwenma Oct 8, 2024

yuwenma Oct 8, 2024 •

edited

Loading

yuwenma Oct 8, 2024

yuwenma Oct 8, 2024

yuwenma Oct 8, 2024

		Copy all existing test cases and add `-direct` suffix to the test names.


		This will override the `cnrm.cloud.google.com/dcl2crd: "true"` or `cnrm.cloud.google.com/tf2crd: "true"` annotations in the CRD and enable the direct controller for the new test cases. The previously-existing test cases will continue to use the TF/DCL-based controller.

		Verify the new `-direct` test cases have equivalent behavior to the existing test cases (though not necessarily the exact same API interactions, if the direct controller achieves the same end result differently than the TF/DCL-based controller).

	* The roundtrip fuzz tests shall cover all the fields in `spec `and `status.observedState `fields [example](https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/0bbac86ace6ab2f4051b574f026d5fe47fa05b75/pkg/controller/direct/redis/cluster/roundtrip_test.go#L92)
	* The roundtrip fuzz tests covers all the fields in `spec `and `status.observedState `fields [example](https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/0bbac86ace6ab2f4051b574f026d5fe47fa05b75/pkg/controller/direct/redis/cluster/roundtrip_test.go#L92)


		### For TF-based Beta resource
		* Remove the `cnrm.cloud.google.com/dcl2crd: "true"` or `cnrm.cloud.google.com/tf2crd: "true"` go tag from the CRD struct, and run `dev/tasks/generate-crds` to use Direct as the permanent controller.

docs: Update workflow for resource migration with reconciler annotation #2860

Are you sure you want to change the base?

docs: Update workflow for resource migration with reconciler annotation #2860

Conversation

jasonvigil commented Oct 4, 2024

google-oss-prow bot commented Oct 4, 2024

acpana 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

yuwenma Oct 8, 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

yuwenma Oct 8, 2024 •

edited

Loading