Skip to content

direct: Implement direct controller, E2E fixtures, and fuzzer for TranslateAdaptiveMtDataset - #12427

Open
hopper-coder-bot wants to merge 3 commits into
GoogleCloudPlatform:masterfrom
hopper-coder-bot:issue-11851-1786846713
Open

direct: Implement direct controller, E2E fixtures, and fuzzer for TranslateAdaptiveMtDataset#12427
hopper-coder-bot wants to merge 3 commits into
GoogleCloudPlatform:masterfrom
hopper-coder-bot:issue-11851-1786846713

Conversation

@hopper-coder-bot

Copy link
Copy Markdown
Collaborator

This Pull Request implements the direct controller, E2E fixtures, and fuzzer for TranslateAdaptiveMtDataset under pkg/controller/direct/translate.

Fixes #11851

Real GCP Recording Details

  • record-gcp was successfully executed against real GCP.
  • GCP Project used: cnrm-barni-4

Verification & Tests Passed

  • Minimal & Maximal E2E fixtures successfully compiled, executed, and recorded.
  • Fuzzer registered and validated with TestSomeMappers in pkg/fuzztesting/fuzztests (Passed).
  • Schema integrity check pkg/crd/template/... passed.
  • API field coverage apichecks check passed.

This PR was generated by the overseer,overseer,greenfield,step/controller agent (powered by the gemini-3.5-flash model).

…nslateAdaptiveMtDataset

Implement the direct controller, E2E fixtures, and fuzzer for
TranslateAdaptiveMtDataset under pkg/controller/direct/translate.

Fix: 11851
@hopper-coder-bot hopper-coder-bot added overseer greenfield Indicates implementation of a new resource (vs migration) step/controller labels Aug 16, 2026
@hopper-coder-bot

Copy link
Copy Markdown
Collaborator Author

GCP Audit Logs Proof of Testing

Below are the Google Cloud audit logs demonstrating that our newly implemented direct controller successfully exercised the GCP Translate API during E2E testing:

---
insertId: 11hj49ue236od
logName: projects/cnrm-barni-4/logs/cloudaudit.googleapis.com%2Factivity
protoPayload:
  '@type': type.googleapis.com/google.cloud.audit.AuditLog
  authenticationInfo:
    oauthInfo:
      oauthClientId: '117594038071648177763'
    principalEmail: overseer-kcc-tester@cnrm-barni-4.iam.gserviceaccount.com
    principalSubject: serviceAccount:overseer-kcc-tester@cnrm-barni-4.iam.gserviceaccount.com
  authorizationInfo:
  - granted: true
    permission: cloudtranslate.adaptiveMtDatasets.delete
    permissionType: ADMIN_WRITE
    resource: projects/cnrm-barni-4/locations/us-central1/adaptiveMtDatasets/dataset-isk5y6vm2ruynra
    resourceAttributes:
      name: projects/cnrm-barni-4/locations/us-central1/adaptiveMtDatasets/dataset-isk5y6vm2ruynra
      service: translate.googleapis.com
      type: translate.googleapis.com/AdaptiveMtDataset
  methodName: google.cloud.translation.v3.TranslationService.DeleteAdaptiveMtDataset
  request:
    '@type': type.googleapis.com/google.cloud.translation.v3.DeleteAdaptiveMtDatasetRequest
    name: projects/cnrm-barni-4/locations/us-central1/adaptiveMtDatasets/dataset-isk5y6vm2ruynra
  serviceName: translate.googleapis.com
  status: {}
resource:
  labels:
    method: google.cloud.translation.v3.TranslationService.DeleteAdaptiveMtDataset
    project_id: cnrm-barni-4
    service: translate.googleapis.com
  type: audited_resource
severity: NOTICE
timestamp: '2026-08-16T03:24:35.008661923Z'
---
insertId: 14al6wqda7os
logName: projects/cnrm-barni-4/logs/cloudaudit.googleapis.com%2Factivity
protoPayload:
  '@type': type.googleapis.com/google.cloud.audit.AuditLog
  authenticationInfo:
    oauthInfo:
      oauthClientId: '117594038071648177763'
    principalEmail: overseer-kcc-tester@cnrm-barni-4.iam.gserviceaccount.com
    principalSubject: serviceAccount:overseer-kcc-tester@cnrm-barni-4.iam.gserviceaccount.com
  authorizationInfo:
  - granted: true
    permission: cloudtranslate.adaptiveMtDatasets.create
    permissionType: ADMIN_WRITE
    resource: projects/600845353393/locations/us-central1
    resourceAttributes:
      name: projects/cnrm-barni-4/locations/us-central1/adaptiveMtDatasets/dataset-isk5y6vm2ruynra
      service: translate.googleapis.com
      type: translate.googleapis.com/AdaptiveMtDataset
  methodName: google.cloud.translation.v3.TranslationService.CreateAdaptiveMtDataset
  request:
    '@type': type.googleapis.com/google.cloud.translation.v3.CreateAdaptiveMtDatasetRequest
    adaptiveMtDataset:
      displayName: Minimal Dataset
      name: projects/cnrm-barni-4/locations/us-central1/adaptiveMtDatasets/dataset-isk5y6vm2ruynra
      sourceLanguageCode: en
      targetLanguageCode: es
    parent: projects/cnrm-barni-4/locations/us-central1
  serviceName: translate.googleapis.com
  status: {}
resource:
  labels:
    method: google.cloud.translation.v3.TranslationService.CreateAdaptiveMtDataset
    project_id: cnrm-barni-4
    service: translate.googleapis.com
  type: audited_resource
severity: NOTICE
timestamp: '2026-08-16T03:24:32.060112320Z'

@google-oss-prow

Copy link
Copy Markdown
Contributor

[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 assign ldanielmadariaga for approval. For more information see the Kubernetes Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@argus-watcher-bot

Copy link
Copy Markdown
Collaborator

🤖 AI Factory started investigating CI check failures for this pull request.

@argus-watcher-bot

Copy link
Copy Markdown
Collaborator

🤖 AI Factory started reviewing this pull request in a sandbox.

@reviewbot-robot reviewbot-robot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KCC Auto-Review Results

  • Trigger criteria matched: Yes
  • Proto Diffs & Update Mask: Pass
  • Structured Reporting: Pass
  • KRM Status Updates: Fail - KRM status update is skipped on no-op updates.
  • LRO Wait: Pass (Not applicable)
  • Test Coverage: Pass - Both minimal and maximal test cases exist in the repository under pkg/test/resourcefixture/testdata/basic/translate/v1alpha1/translateadaptivemtdataset/

Detailed Findings / Actions Required:

  1. In pkg/controller/direct/translate/translateadaptivemtdataset_controller.go (line 191), returning nil directly on no-op updates skips updating the KRM Status. Under KCC's Greenfield controller standards, the status MUST always be updated at the end of both Create and Update operations to ensure consistency.


if !diffs.HasDiff() {
log.V(2).Info("no field needs update", "name", a.id.String())
return nil

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KCC direct controller standards require that status is always updated at the end of an Update operation, even on no-op reconciliations (when there's no diff between desired and actual states). Please call a.updateStatus(ctx, updateOp, a.actual) instead of returning nil.

@argus-watcher-bot

Copy link
Copy Markdown
Collaborator

🤖 AI Factory started addressing review feedback for this pull request.

@hopper-coder-bot

Copy link
Copy Markdown
Collaborator Author

I have addressed the review feedback by updating pkg/controller/direct/translate/translateadaptivemtdataset_controller.go to call a.updateStatus(ctx, updateOp, a.actual) instead of returning nil directly on no-op updates. This ensures that the KRM status is always updated at the end of the Update operation, in compliance with KCC Greenfield controller standards.

The change has been committed and successfully pushed.

(This comment was generated by overseer)

@argus-watcher-bot

Copy link
Copy Markdown
Collaborator

🤖 AI Factory started addressing review feedback for this pull request.

@hopper-coder-bot

Copy link
Copy Markdown
Collaborator Author

I have verified the implementation, running formatting and go vet verification as well as the fuzzer mappers tests (TestSomeMappers), which all pass successfully.

The previous fix correctly addressed the review feedback by calling a.updateStatus(ctx, updateOp, a.actual) in pkg/controller/direct/translate/translateadaptivemtdataset_controller.go on no-op updates to ensure status is always updated at the end of the Update operation.

The PR is fully updated, verified, and ready for further review!

(This comment was generated by overseer)

@argus-watcher-bot

Copy link
Copy Markdown
Collaborator

🤖 AI Factory started investigating CI check failures for this pull request.

@hopper-coder-bot

Copy link
Copy Markdown
Collaborator Author

Investigating tests-e2e-fixtures-compute-4-of-4 failure

Run: 31965078531
Name: tests-e2e-fixtures-compute-4-of-4
Cause: Flake
Details: The failure in the computeinstancegroup test fixture with an unexpected diff in _http_mock.log is a known pre-existing flake or mock discrepancy on the master branch. This PR implements the direct controller and fixtures for TranslateAdaptiveMtDataset and does not touch any compute-related resources.
Action Taken: Rerun triggered using the GitHub CLI (gh run rerun).

Investigating tests-e2e-fixtures-hypercomputecluster failure

Run: 31965078531
Name: tests-e2e-fixtures-hypercomputecluster
Cause: Infrastructure
Details: The job failed because the K8s control plane failed to start due to a missing etcd executable in the runner's $PATH (exec: "etcd": executable file not found in $PATH). This is a pure environmental/infrastructure issue.
Action Taken: Rerun triggered using the GitHub CLI (gh run rerun).

Investigating presubmit-gatekeeper failure

Run: 31965078531
Name: presubmit-gatekeeper
Cause: Infrastructure
Details: This gatekeeper job failed because it caught the failures in the tests-e2e-fixtures-matrix (specifically from the compute-4-of-4 and hypercomputecluster test runs).
Action Taken: Rerun triggered using the GitHub CLI (gh run rerun).


Note: The priority fixtures test suite priority-e2e-fixtures for translate (which directly tests our changes in this PR) passed successfully in this run!

(This report was generated by overseer)

@argus-watcher-bot

Copy link
Copy Markdown
Collaborator

🤖 AI Factory started reviewing this pull request in a sandbox.

@reviewbot-robot reviewbot-robot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KCC Auto-Review Results

  • Trigger criteria matched: Yes
  • Proto Diffs & Update Mask: Pass (The resource is immutable; hence, updates are blocked on KRM and UpdateMask is not used)
  • Structured Reporting: Pass (Diff is reported correctly via structuredreporting.ReportDiff on update checks)
  • KRM Status Updates: Pass (Status is always updated via updateStatus at the end of Create, and on no-op Update reconciliation runs)
  • LRO Wait: Pass (The resource creation/deletion operations are synchronous and do not return an LRO)
  • Test Coverage: Pass (Minimal and maximal test cases are provided under pkg/test/resourcefixture/testdata/basic/translate/v1alpha1/translateadaptivemtdataset)

Detailed Findings / Actions Required:

The overall implementation is solid and follows direct controller conventions perfectly. The only issue is a minor schema design flaw where exampleCount (an output-only GCP field) was mapped to KRM .spec instead of .status.observedState. Since this is a new Alpha Greenfield resource, we recommend moving this field to Status before the resource schema is frozen/released.


clonedDesired := proto.Clone(desired).(*pb.AdaptiveMtDataset)

// Since exampleCount is read-only / output-only on GCP, copy it from maskedActual to clonedDesired to prevent false diffs.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clonedDesired.ExampleCount = maskedActual.ExampleCount statement is used to bypass differences on exampleCount because it is an output-only/read-only field in the GCP AdaptiveMtDataset resource.

According to KCC Greenfield design principles, service-generated/output-only fields should reside under status.observedState rather than .spec. This design leak in the schema necessitates workaround logic in the controller to avoid false diffs during reconciliation.

Since TranslateAdaptiveMtDataset is a new alpha Greenfield resource, we should move exampleCount from TranslateAdaptiveMtDatasetSpec to TranslateAdaptiveMtDatasetObservedState in apis/translate/v1alpha1/translateadaptivemtdataset_types.go, regenerate the mappers, and update the controller to clean up this workaround.

f.SpecField(".display_name")
f.SpecField(".source_language_code")
f.SpecField(".target_language_code")
f.SpecField(".example_count")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.example_count is registered as f.SpecField(".example_count") because it was incorrectly defined under spec in the KRM types.

Once exampleCount is moved to status.observedState (since it is output-only), this must be registered using f.StatusField(".example_count") instead of f.SpecField.

[missing_field] crd=transcoderjobs.transcoder.cnrm.cloud.google.com version=v1alpha1: field ".spec.templateID" is not set in unstructured objects
[missing_field] crd=transcoderjobs.transcoder.cnrm.cloud.google.com version=v1alpha1: field ".spec.ttlAfterCompletionDays" is not set in unstructured objects
[missing_field] crd=translateadaptivemtdatasets.translate.cnrm.cloud.google.com version=v1alpha1: field ".spec.displayName" is not set in unstructured objects
[missing_field] crd=translateadaptivemtdatasets.translate.cnrm.cloud.google.com version=v1alpha1: field ".spec.exampleCount" is not set in unstructured objects

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .spec.exampleCount exception in alpha-missingfields.txt is required only because exampleCount is mapped under .spec instead of .status.observedState. Because it is an output-only field, it cannot be configured in the test fixtures (create.yaml / update.yaml), triggering the alpha-missingfields linter.

Moving exampleCount to status would allow removing this exception completely.

@argus-watcher-bot

Copy link
Copy Markdown
Collaborator

🤖 AI Factory started addressing review feedback for this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

greenfield Indicates implementation of a new resource (vs migration) overseer/review overseer step/controller

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Greenfield: Implement direct controller, E2E fixtures, and fuzzer for TranslateAdaptiveMtDataset

4 participants