Skip to content

feat(placement): switch MC/DNS discovery to mc-registration secrets + injected domains - #88

Merged
openshift-merge-bot[bot] merged 3 commits into
openshift-online:mainfrom
patjlm:placement-mc-registration
Aug 12, 2026
Merged

openshift-merge-bot[bot] merged 3 commits into
openshift-online:mainfrom
patjlm:placement-mc-registration

Conversation

@patjlm

@patjlm patjlm commented Aug 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Switches gecko-placement MC discovery from the Maestro consumers API +
argocd-cluster DNS lookup to per-MC mc-registration-* Secret Manager
secrets and an injected --hc-dns-domains flag.

  • DynamicSelector discovers eligible MCs by listing mc-registration-* Secret
    Manager secrets (label mc-registration=true), reading each secret's JSON
    payload ({"projectId": "...", "mode": "active|maintenance"}), and filtering
    to mode == "active". Individual broken secrets are logged and skipped —
    placement continues as long as at least one healthy MC exists. Eligible MC
    results are cached (30s TTL) to reduce Secret Manager RPCs under burst
    reconciliations.
  • Removes gecko-placement's dependency on the Maestro consumer API.
    (Note: maestro-consumer-name label and Maestro consumers API are still used
    by hyperfleet-placement-adapter for the CLS/legacy cluster flow.)
  • HC DNS zone domains are no longer read from the argocd-cluster secret.
    They're injected at construction time via a --hc-dns-domains
    comma-separated flag (Helm value hcDnsDomains), since domains are a
    region-level resource shared across all MCs, not per-MC.
  • Re-introduced minimal Selector interface to keep reconciler testable
    independently of Secret Manager wiring.
  • cmd/placement: dropped --candidates / --base-domains /
    --maestro-http-addr flags, added --hc-dns-domains, --secretmanager-project
    is now required.
  • Helm chart (gecko-placement): dropped candidates / baseDomains /
    maestro values, added hcDnsDomains; validateValues now requires both
    secretManagerProject and hcDnsDomains.

Related

  • Infra-side prerequisite: gcp-hcp-infra#1174
    — creates the per-MC mc-registration-* secrets, conditions the
    gecko-placement GSA's IAM to those secrets only, and wires
    hcDnsDomains into the ArgoCD Application template.

Testing

  • go build/vet/test ./... in controllers module — all pass
  • helm lint / helm template with required values — renders correctly,
    fails fast without secretManagerProject / hcDnsDomains

… injected domains

- DynamicSelector: discover eligible MCs via mc-registration-* SM secrets
  (label mc-registration=true, JSON {projectId,mode}), filter mode=active.
  Drop Maestro consumer API dependency entirely.
- DynamicSelector: DNS domains now injected at construction (Helm value ->
  --hc-dns-domains flag) instead of reading the argocd-cluster secret,
  since domains are a region-level resource shared across MCs.
- Remove Candidate struct, Selector interface, RoundRobinSelector -
  DynamicSelector is now the sole selector implementation.
- cmd: drop --candidates/--base-domains/--maestro-http-addr flags, add
  --hc-dns-domains, require --secretmanager-project.
- helm: drop candidates/baseDomains/maestro values, add hcDnsDomains,
  require both secretManagerProject and hcDnsDomains in validateValues.

Implements docs/mc-registration-plan.md and
docs/gecko-placement-dns-zones-plan.md.
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 12, 2026
@openshift-ci

openshift-ci Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci

openshift-ci Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patjlm

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

The pull request process is described 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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2026
patjlm added 2 commits August 12, 2026 12:43
F1: eligibleMCs skips broken secrets (log+continue) instead of failing all placement
F2: reconciler test asserts Status().Update() called on happy path
F3: 30s TTL cache on eligible MC list to reduce SM RPCs under burst
F4: render --hc-dns-domains unconditionally in Helm deployment
F6: re-add Selector interface, decouple Reconciler from DynamicSelector
F7: remove unreachable len(domains)==0 guard and its test
F8: add TestParseDomains for cmd package
F9: SM label filter uses = (equality) instead of : (containment)
@patjlm
patjlm marked this pull request as ready for review August 12, 2026 11:11
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 12, 2026
@patjlm

patjlm commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@patjlm

patjlm commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

/test ci/prow/test

@openshift-ci

openshift-ci Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

@patjlm: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test test

Use /test all to run all jobs.

Details

In response to this:

/test ci/prow/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

if v := envOr("MAESTRO_HTTP_ADDR", ""); v != "" && !cmd.Flags().Changed("maestro-http-addr") {
maestroHTTPAddr = v
if smProject == "" {
return fmt.Errorf("--secretmanager-project is required (or $SECRETMANAGER_PROJECT)")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@cristianoveiga i realize this may make more complex the local runs of this controller. let me know if you want me to reinstate the mc list flag.. but then we also need to add other params like maintenance...

alternatively, we go the externalsecret route. and on local deployment we can create our own secret?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the idea of having the candidates list (either via a flag or a configmap/secret), but I think we can deal with that separately (in a future PR)

@cristianoveiga

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2026
@openshift-merge-bot
openshift-merge-bot Bot merged commit 5925229 into openshift-online:main Aug 12, 2026
8 checks passed
cristianoveiga added a commit to cristianoveiga/gecko that referenced this pull request Aug 12, 2026
MC name and GCP project ID are interchangeable per team confirmation,
matching the mc-registration Secret Manager secret convention from PR openshift-online#88.
Removes the mcProjects map and --mc-projects flag; targetCluster is used
directly as the GCP project ID when creating Firestore clients.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cristianoveiga added a commit to cristianoveiga/gecko that referenced this pull request Aug 12, 2026
MC name and GCP project ID are interchangeable per team confirmation,
matching the mc-registration Secret Manager secret convention from PR openshift-online#88.
Removes the mcProjects map and --mc-projects flag; targetCluster is used
directly as the GCP project ID when creating Firestore clients.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants