ROSAENG-60886: Migrate from EKS Auto Mode to self-managed Karpenter - #698
Open
theautoroboto wants to merge 5 commits into
Open
ROSAENG-60886: Migrate from EKS Auto Mode to self-managed Karpenter#698theautoroboto wants to merge 5 commits into
theautoroboto wants to merge 5 commits into
Conversation
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces EKS Auto Mode with self-managed OSS Karpenter. It adds bootstrap capacity, IAM and interruption handling, Argo CD wiring, AWS Load Balancer Controller integration, updated bootstrap logic, monitoring changes, tooling updates, and documentation. ChangesOSS Karpenter migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
/test on-demand-e2e |
theautoroboto
marked this pull request as draft
July 23, 2026 16:41
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
Author
|
/test on-demand-e2e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
terraform/modules/eks-cluster/iam.tf (1)
262-289: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRemove the unused instance-profile write permissions.
EC2NodeClass.spec.instanceProfileis used in both nodeclass templates, and this policy already assumes the profile is pre-created; keepiam:GetInstanceProfile,iam:ListInstanceProfiles, andiam:PassRole, but dropiam:CreateInstanceProfile,iam:TagInstanceProfile,iam:AddRoleToInstanceProfile,iam:DeleteInstanceProfile, andiam:RemoveRoleFromInstanceProfile.terraform/modules/eks-cluster/iam.tf:262-289🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@terraform/modules/eks-cluster/iam.tf` around lines 262 - 289, Remove the IAMInstanceProfileCreate and IAMInstanceProfileModify statements from the policy, eliminating CreateInstanceProfile, TagInstanceProfile, AddRoleToInstanceProfile, DeleteInstanceProfile, and RemoveRoleFromInstanceProfile permissions. Preserve the existing iam:GetInstanceProfile, iam:ListInstanceProfiles, and iam:PassRole permissions.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@argocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yaml`:
- Around line 7-8: Update the amiSelectorTerms configuration in the NodeClass to
use the bottlerocket-fips@latest alias instead of bottlerocket@latest,
preserving the existing AMI selector structure.
In `@argocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yaml`:
- Around line 1-18: Update the EC2NodeClass resource named fips so
amiSelectorTerms selects the approved FIPS Bottlerocket AMI instead of the
standard bottlerocket@latest alias. Preserve the existing FIPS pool contract and
leave the cluster, subnet, security group, and metadata settings unchanged.
In `@argocd/config/shared/argocd/values.yaml`:
- Around line 172-185: Update the redis-ha configuration block in values.yaml to
explicitly set redis-ha.enabled to true, and verify its HA replica settings
remain aligned with the intended Argo CD HA configuration while preserving the
existing tolerations.
In `@argocd/config/shared/storageclass/templates/gp3.yaml`:
- Line 7: Replace the existing gp3 StorageClass rather than mutating its
immutable provisioner: add a new StorageClass with a distinct name using
ebs.csi.aws.com, update management and regional consumers/defaulting to
reference it, and retain the old class until the new driver is available before
retiring it.
In `@docs/design/fips-eks-compute.md`:
- Around line 77-79: Revise the compliance statement in the Bottlerocket
workload description to scope it to node-level FIPS readiness and FIPS-validated
cryptographic modules. Remove the implication that this alone satisfies FedRAMP
High/Moderate requirements for the cluster or customer-bearing workloads, unless
the statement explicitly references the broader required control set.
In `@docs/design/karpenter-node-provisioning.md`:
- Around line 27-32: Update the rationale in
docs/design/karpenter-node-provisioning.md:27-32 to remove the claim that Pod
Identity requires a separate admission webhook, retaining only the bootstrap and
compatibility reasons for choosing IRSA. Also update
docs/design/zoa-trusted-actions.md:816 to state that IRSA remains supported and
compare Pod Identity as an option that primarily avoids per-cluster OIDC
provider management; do not describe IRSA as deprecated.
In `@docs/design/rate-limiting-architecture.md`:
- Line 242: Update the Provisioned Concurrency note so each struck-through value
uses its own matched Markdown delimiters: `~~200 req/s~~` for traffic and
`~~$30-45/month~~` for cost. Keep the surrounding explanation unchanged.
In `@scripts/validate-rc-aws.sh`:
- Around line 135-151: Update the node-group validation flow around the
ng_desired and ng_ready queries to run these detail queries only when the node
group is confirmed ACTIVE, matching the guarded structure in validate-mc-aws.sh.
Preserve safe fallback handling for AWS or jq failures so missing or unavailable
node groups do not abort the script or leave ng_ready empty before the numeric
comparison.
In `@terraform/modules/aws-load-balancer-controller/README.md`:
- Around line 14-16: Update the upstream recommended policy URL in the README
IAM role description to reference version v2.17.1, matching the versions used by
iam.tf and Chart.yaml.
In `@terraform/modules/ecs-bootstrap/README.md`:
- Line 77: Update the karpenter_version entry in the Inputs table to document
the module’s actual default of "1.13.0", matching the default defined by the
karpenter_version variable.
In `@terraform/modules/eks-cluster/README.md`:
- Line 102: Update the ami_kms_key_arn documentation row in the module README so
kms:CreateGrant is described as granted only to the Karpenter controller role,
while preserving the existing kms:Decrypt role description.
In `@terraform/modules/eks-cluster/variables.tf`:
- Around line 84-88: Update the ami_kms_key_arn variable description to
accurately state that setting it adds the implemented KMS permissions to the
Karpenter controller role only, specifically kms:CreateGrant and
kms:DescribeKey, and remove claims about kms:Decrypt or node-role permissions.
---
Nitpick comments:
In `@terraform/modules/eks-cluster/iam.tf`:
- Around line 262-289: Remove the IAMInstanceProfileCreate and
IAMInstanceProfileModify statements from the policy, eliminating
CreateInstanceProfile, TagInstanceProfile, AddRoleToInstanceProfile,
DeleteInstanceProfile, and RemoveRoleFromInstanceProfile permissions. Preserve
the existing iam:GetInstanceProfile, iam:ListInstanceProfiles, and iam:PassRole
permissions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0775ea04-2597-48b3-ad3f-5c3825efa1a2
📒 Files selected for processing (60)
Makefileargocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/management-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/management-cluster/eks-nodepool/values.yamlargocd/config/management-cluster/hypershift/templates/05-job.yamlargocd/config/management-cluster/monitoring/values.yamlargocd/config/regional-cluster/aws-load-balancer-controller/Chart.yamlargocd/config/regional-cluster/aws-load-balancer-controller/values.yamlargocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/regional-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/regional-cluster/eks-nodepool/values.yamlargocd/config/regional-cluster/loki/templates/targetgroupbinding.yamlargocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yamlargocd/config/regional-cluster/thanos/templates/targetgroupbinding.yamlargocd/config/shared/argocd/values.yamlargocd/config/shared/storageclass/templates/gp3.yamlci/ephemeral-provider/__init__.pyconfig/templates/argocd-bootstrap/applicationset.yaml.j2deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldocs/README.mddocs/design/fips-eks-compute.mddocs/design/fully-private-eks-bootstrap.mddocs/design/gitops-cluster-configuration.mddocs/design/karpenter-node-provisioning.mddocs/design/logging-platform.mddocs/design/rate-limiting-architecture.mddocs/design/thanos-metrics-infrastructure.mddocs/design/zoa-trusted-actions.mdscripts/buildspec/provision-infra-mc.shscripts/buildspec/register.shscripts/validate-mc-aws.shscripts/validate-mc-k8s.shscripts/validate-rc-aws.shscripts/validate-rc-k8s.shterraform/config/management-cluster/main.tfterraform/config/pipeline-management-cluster/main.tfterraform/config/pipeline-regional-cluster/main.tfterraform/config/regional-cluster/imports.shterraform/config/regional-cluster/main.tfterraform/modules/aws-load-balancer-controller/README.mdterraform/modules/aws-load-balancer-controller/iam.tfterraform/modules/aws-load-balancer-controller/main.tfterraform/modules/aws-load-balancer-controller/outputs.tfterraform/modules/aws-load-balancer-controller/variables.tfterraform/modules/aws-load-balancer-controller/versions.tfterraform/modules/ecs-bootstrap/README.mdterraform/modules/ecs-bootstrap/main.tfterraform/modules/ecs-bootstrap/variables.tfterraform/modules/eks-cluster/README.mdterraform/modules/eks-cluster/data.tfterraform/modules/eks-cluster/iam.tfterraform/modules/eks-cluster/locals.tfterraform/modules/eks-cluster/main.tfterraform/modules/eks-cluster/outputs.tfterraform/modules/eks-cluster/variables.tfterraform/modules/eks-cluster/versions.tfterraform/modules/hyperfleet-infrastructure/amazonmq.tf
theautoroboto
force-pushed
the
remove-auto-mode-v2
branch
from
July 23, 2026 16:55
98def0f to
23b650e
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
Author
|
/test on-demand-e2e |
2 similar comments
Contributor
Author
|
/test on-demand-e2e |
Contributor
Author
|
/test on-demand-e2e |
theautoroboto
marked this pull request as ready for review
July 23, 2026 19:51
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
/test on-demand-e2e |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/zoa-trusted-actions.md`:
- Line 825: Update the IRSA rationale in the trusted-actions design section to
remove the claim that IRSA is deprecated. State instead that the platform
standardizes on EKS Pod Identity for new workload service accounts and avoids
per-cluster OIDC provider management, while preserving the existing comparison
context.
In `@scripts/validate-mc-k8s.sh`:
- Around line 138-184: Qualify all HyperShift NodePool kubectl resource
references in the NodePool validation block: update the list command, per-object
get commands, and process-substitution command to use
nodepools.hypershift.openshift.io, while preserving the existing counting and
readiness checks.
In `@terraform/modules/eks-cluster/iam.tf`:
- Around line 395-409: Add an aws:SourceArn condition to the AllowEventBridge
statement in aws_sqs_queue_policy.karpenter_interruption, restricting
sqs:SendMessage to the Karpenter EventBridge rule ARN(s); optionally also
constrain aws:SourceAccount. Reuse the existing Karpenter EventBridge rule
resource symbols and preserve the queue policy’s enablement and permissions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ece5d3d8-316a-4df0-98b5-a9949422062f
📒 Files selected for processing (57)
Makefileargocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/management-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/management-cluster/eks-nodepool/values.yamlargocd/config/management-cluster/hypershift/templates/05-job.yamlargocd/config/management-cluster/monitoring/values.yamlargocd/config/regional-cluster/aws-load-balancer-controller/Chart.yamlargocd/config/regional-cluster/aws-load-balancer-controller/values.yamlargocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/regional-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/regional-cluster/eks-nodepool/values.yamlargocd/config/regional-cluster/loki/templates/targetgroupbinding.yamlargocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yamlargocd/config/regional-cluster/thanos/templates/targetgroupbinding.yamlargocd/config/shared/argocd/values.yamlargocd/config/shared/storageclass/templates/gp3.yamlci/ephemeral-provider/__init__.pyconfig/templates/argocd-bootstrap/applicationset.yaml.j2deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldocs/README.mddocs/design/fips-eks-compute.mddocs/design/fully-private-eks-bootstrap.mddocs/design/karpenter-node-provisioning.mddocs/design/logging-platform.mddocs/design/thanos-metrics-infrastructure.mddocs/design/zoa-trusted-actions.mdscripts/buildspec/provision-infra-mc.shscripts/buildspec/register.shscripts/validate-mc-aws.shscripts/validate-mc-k8s.shscripts/validate-rc-aws.shscripts/validate-rc-k8s.shterraform/config/management-cluster/main.tfterraform/config/pipeline-management-cluster/main.tfterraform/config/pipeline-regional-cluster/main.tfterraform/config/regional-cluster/imports.shterraform/config/regional-cluster/main.tfterraform/modules/aws-load-balancer-controller/README.mdterraform/modules/aws-load-balancer-controller/iam.tfterraform/modules/aws-load-balancer-controller/main.tfterraform/modules/aws-load-balancer-controller/outputs.tfterraform/modules/aws-load-balancer-controller/variables.tfterraform/modules/aws-load-balancer-controller/versions.tfterraform/modules/ecs-bootstrap/README.mdterraform/modules/ecs-bootstrap/main.tfterraform/modules/ecs-bootstrap/variables.tfterraform/modules/eks-cluster/README.mdterraform/modules/eks-cluster/data.tfterraform/modules/eks-cluster/iam.tfterraform/modules/eks-cluster/locals.tfterraform/modules/eks-cluster/main.tfterraform/modules/eks-cluster/outputs.tfterraform/modules/eks-cluster/variables.tfterraform/modules/eks-cluster/versions.tf
🚧 Files skipped from review as they are similar to previous changes (46)
- argocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yaml
- terraform/modules/aws-load-balancer-controller/versions.tf
- argocd/config/regional-cluster/eks-nodepool/values.yaml
- argocd/config/regional-cluster/eks-nodepool/templates/10-nodepool.yaml
- argocd/config/management-cluster/eks-nodepool/values.yaml
- argocd/config/management-cluster/eks-nodepool/templates/10-nodepool.yaml
- ci/ephemeral-provider/init.py
- terraform/config/pipeline-regional-cluster/main.tf
- terraform/modules/eks-cluster/locals.tf
- terraform/config/pipeline-management-cluster/main.tf
- terraform/config/management-cluster/main.tf
- Makefile
- config/templates/argocd-bootstrap/applicationset.yaml.j2
- terraform/modules/aws-load-balancer-controller/main.tf
- argocd/config/regional-cluster/aws-load-balancer-controller/values.yaml
- docs/design/logging-platform.md
- deploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
- deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
- argocd/config/shared/storageclass/templates/gp3.yaml
- terraform/modules/aws-load-balancer-controller/variables.tf
- docs/README.md
- terraform/modules/aws-load-balancer-controller/outputs.tf
- docs/design/thanos-metrics-infrastructure.md
- terraform/modules/eks-cluster/data.tf
- terraform/modules/aws-load-balancer-controller/README.md
- argocd/config/management-cluster/monitoring/values.yaml
- deploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yaml
- terraform/modules/eks-cluster/versions.tf
- argocd/config/regional-cluster/aws-load-balancer-controller/Chart.yaml
- terraform/modules/ecs-bootstrap/variables.tf
- deploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yaml
- argocd/config/regional-cluster/thanos/templates/targetgroupbinding.yaml
- terraform/modules/eks-cluster/variables.tf
- terraform/config/regional-cluster/main.tf
- argocd/config/shared/argocd/values.yaml
- argocd/config/regional-cluster/loki/templates/targetgroupbinding.yaml
- terraform/modules/eks-cluster/outputs.tf
- scripts/validate-mc-aws.sh
- terraform/modules/eks-cluster/main.tf
- terraform/modules/aws-load-balancer-controller/iam.tf
- docs/design/fips-eks-compute.md
- terraform/modules/ecs-bootstrap/main.tf
- scripts/validate-rc-aws.sh
- docs/design/karpenter-node-provisioning.md
- argocd/config/management-cluster/hypershift/templates/05-job.yaml
- terraform/modules/eks-cluster/README.md
theautoroboto
changed the title
openshift-ci-robot
added
the
jira/valid-reference
Collaborator
|
@theautoroboto: This pull request references ROSAENG-60886 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
Contributor
Author
|
/test on-demand-e2e |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/ephemeral-provider/orchestrator.py`:
- Around line 233-244: Update the docstring describing the override-directory
handling to reflect the logic around load_and_merge: region YAML files in
config/ephemeral are replaced, while defaults.yaml is retained and merged with
the override to preserve existing environment settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 966663f8-4dcf-4747-9d6a-905bda0cf917
📒 Files selected for processing (1)
ci/ephemeral-provider/orchestrator.py
Contributor
Author
|
/test on-demand-e2e |
openshift-ci
Bot
added
the
needs-rebase
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
argocd/config/shared/argocd/templates/sre-targetgroupbinding.yaml (1)
2-2: 📐 Maintainability & Code Quality | 🔵 TrivialValidate the shared rendered configuration.
Run Helm lint and validate rendered output for both management and regional rendering contexts before merge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@argocd/config/shared/argocd/templates/sre-targetgroupbinding.yaml` at line 2, Validate the shared rendered configuration for sre-targetgroupbinding by running Helm lint and checking the rendered output in both management and regional rendering contexts before merging.Source: Coding guidelines
argocd/config/regional-cluster/grafana/templates/sre-targetgroupbinding.yaml (1)
2-2: 📐 Maintainability & Code Quality | 🔵 TrivialRun Helm lint and validate rendered configuration for every changed chart.
argocd/config/regional-cluster/grafana/templates/sre-targetgroupbinding.yaml#L2-L2: lint and render the Grafana chart.argocd/config/regional-cluster/loki/templates/targetgroupbinding.yaml#L2-L2: lint and render the Loki chart.argocd/config/regional-cluster/loki/templates/targetgroupbinding.yaml#L20-L20: validate the query-frontend rendering.argocd/config/regional-cluster/loki/templates/targetgroupbinding.yaml#L38-L38: validate the SRE query-frontend rendering.argocd/config/regional-cluster/monitoring/templates/sre-targetgroupbinding.yaml#L2-L2: lint and render the monitoring chart.argocd/config/shared/argocd/templates/sre-targetgroupbinding.yaml#L2-L2: validate both management and regional renderings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@argocd/config/regional-cluster/grafana/templates/sre-targetgroupbinding.yaml` at line 2, Run Helm lint and validate rendered output for every affected chart: Grafana at argocd/config/regional-cluster/grafana/templates/sre-targetgroupbinding.yaml:2-2; Loki at argocd/config/regional-cluster/loki/templates/targetgroupbinding.yaml:2-2, including query-frontend at :20-20 and SRE query-frontend at :38-38; monitoring at argocd/config/regional-cluster/monitoring/templates/sre-targetgroupbinding.yaml:2-2; and both management and regional renderings for argocd/config/shared/argocd/templates/sre-targetgroupbinding.yaml:2-2. Fix any lint or rendering errors exposed by these validations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@argocd/config/regional-cluster/grafana/templates/sre-targetgroupbinding.yaml`:
- Line 2: Run Helm lint and validate rendered output for every affected chart:
Grafana at
argocd/config/regional-cluster/grafana/templates/sre-targetgroupbinding.yaml:2-2;
Loki at
argocd/config/regional-cluster/loki/templates/targetgroupbinding.yaml:2-2,
including query-frontend at :20-20 and SRE query-frontend at :38-38; monitoring
at
argocd/config/regional-cluster/monitoring/templates/sre-targetgroupbinding.yaml:2-2;
and both management and regional renderings for
argocd/config/shared/argocd/templates/sre-targetgroupbinding.yaml:2-2. Fix any
lint or rendering errors exposed by these validations.
In `@argocd/config/shared/argocd/templates/sre-targetgroupbinding.yaml`:
- Line 2: Validate the shared rendered configuration for sre-targetgroupbinding
by running Helm lint and checking the rendered output in both management and
regional rendering contexts before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 09a325f2-c261-44a8-a248-39f9ce5e3f17
📒 Files selected for processing (5)
argocd/config/regional-cluster/grafana/templates/sre-targetgroupbinding.yamlargocd/config/regional-cluster/loki/templates/targetgroupbinding.yamlargocd/config/regional-cluster/monitoring/templates/sre-targetgroupbinding.yamlargocd/config/regional-cluster/thanos/templates/targetgroupbinding.yamlargocd/config/shared/argocd/templates/sre-targetgroupbinding.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- argocd/config/regional-cluster/thanos/templates/targetgroupbinding.yaml
openshift-ci
Bot
removed
the
needs-rebase
Contributor
Author
|
/test on-demand-e2e |
4 similar comments
Contributor
Author
|
/test on-demand-e2e |
Contributor
Author
|
/test on-demand-e2e |
Contributor
Author
|
/test on-demand-e2e |
Contributor
Author
|
/test on-demand-e2e |
openshift-ci
Bot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
/test on-demand-e2e |
1 similar comment
Contributor
Author
|
/test on-demand-e2e |
theautoroboto
force-pushed
the
remove-auto-mode-v2
branch
from
August 10, 2026 17:10
e14cdb9 to
b322fa4
Compare
Contributor
Author
|
/test on-demand-e2e |
2 similar comments
Contributor
Author
|
/test on-demand-e2e |
Contributor
Author
|
/test on-demand-e2e |
theautoroboto
force-pushed
the
remove-auto-mode-v2
branch
from
August 10, 2026 23:56
8900f40 to
62e32a7
Compare
Contributor
Author
|
/test on-demand-e2e |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
docs/design/karpenter-node-provisioning.md (1)
41-47:⚠️ Potential issue | 🟠 MajorKeep the IAM role and instance-profile contracts consistent.
iam:PassRoletargets the node IAM role ARN.EC2NodeClass.spec.instanceProfilereferences the pre-created instance profile name. The documentation must not treat these as the same resource.
docs/design/karpenter-node-provisioning.md#L41-L47: showiam:PassRoletargeting the node IAM role and render the instance profile separately.docs/design/karpenter-node-provisioning.md#L66-L72: describespec.instanceProfileas the exported instance-profile name, not an ARN or role.docs/design/fips-eks-compute.md#L120-L121: state that the instance profile wraps the node role and is referenced by the NodeClass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/karpenter-node-provisioning.md` around lines 41 - 47, Correct the IAM resource distinctions across the documented sites: in docs/design/karpenter-node-provisioning.md lines 41-47, show iam:PassRole targeting the node IAM role ARN and render the instance profile as a separate resource; in lines 66-72, describe EC2NodeClass.spec.instanceProfile as the exported instance-profile name, not an ARN or role; and in docs/design/fips-eks-compute.md lines 120-121, state that the instance profile wraps the node role and is referenced by the NodeClass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/fips-eks-compute.md`:
- Around line 100-101: Update the IAM inventory in the design document to list
three roles separately: the IRSA-backed Karpenter controller role,
karpenter-node-role, and the lightweight karpenter-bootstrap node-group role.
Replace the statement that only two roles are required while preserving each
role’s intended usage.
- Around line 19-22: Reword the FIPS compliance statements in the document,
including the sections around the RHEL node description and the additionally
referenced sections, to scope the claim to node-level FIPS readiness only. State
that FIPS-enabled RHEL nodes support validated cryptographic modules for node
compute, while clarifying that FedRAMP High/Moderate authorization and validated
cryptography across cluster and workload operations require additional controls.
- Around line 96-97: Update the workload description near the Karpenter
controller statement to stop classifying OSS Karpenter as an AWS-managed addon.
Replace that wording with “platform system components,” or explicitly document
the ownership of Karpenter, CoreDNS, and metrics-server while preserving the
distinction that they are not customer-bearing workloads.
In `@docs/sop/karpenter-lifecycle.md`:
- Around line 112-114: Correct the final comparison row in the Karpenter
lifecycle table: rename “Compute node taint” to describe the bootstrap node
group, and replace the OSS Karpenter value with the exact canonical node-group
name from the table at line 40, including the “<cluster-id>-” prefix.
In `@terraform/modules/ecs-bootstrap/main.tf`:
- Around line 294-318: Gate the hypershift health-wait block around the existing
CLUSTER_TYPE check with an explicit E2E/CI-only flag, ensuring ordinary
management-cluster bootstraps skip it. Define or reuse the flag in the bootstrap
environment and set it only from the E2E workflow; preserve the existing timeout
and error handling when the gate is enabled.
- Around line 297-299: Update the HyperShift readiness loop around the kubectl
application status query to retrieve and validate both sync and health statuses,
requiring .status.sync.status to equal "Synced" and .status.health.status to
equal "Healthy" before proceeding. Preserve the existing retry behavior and
error handling.
- Around line 297-313: The HyperShift polling and diagnostic kubectl calls lack
finite request timeouts. Update every kubectl request in the HyperShift wait
loop—including health, status, message, and timeout diagnostic calls—to include
a finite --request-timeout value, while preserving the existing deadline and
error-handling behavior.
In `@terraform/modules/ecs-bootstrap/variables.tf`:
- Around line 85-89: Require a non-empty value for karpenter_controller_role_arn
by removing its empty default or adding variable validation that rejects empty
strings. Keep the existing string type and description, ensuring module
configuration cannot pass an empty ARN to the Karpenter IRSA annotation.
In `@terraform/modules/eks-cluster/README.md`:
- Line 134: Update the Karpenter lifecycle statement in the module README to
describe concurrent synchronization with retries until the required CRDs exist,
rather than promising sync-wave ordering; alternatively, implement the
documented ordering in the bootstrap configuration before retaining that claim.
---
Duplicate comments:
In `@docs/design/karpenter-node-provisioning.md`:
- Around line 41-47: Correct the IAM resource distinctions across the documented
sites: in docs/design/karpenter-node-provisioning.md lines 41-47, show
iam:PassRole targeting the node IAM role ARN and render the instance profile as
a separate resource; in lines 66-72, describe EC2NodeClass.spec.instanceProfile
as the exported instance-profile name, not an ARN or role; and in
docs/design/fips-eks-compute.md lines 120-121, state that the instance profile
wraps the node role and is referenced by the NodeClass.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4459a18f-0b6a-4b8e-80d0-b3d565592f2d
📒 Files selected for processing (46)
.spec/002-spec-to-pr-agent/requirements.mdargocd/config/management-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/management-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/management-cluster/hypershift/templates/05-job.yamlargocd/config/management-cluster/karpenterargocd/config/management-cluster/kube-applier/templates/servicemonitor.yamlargocd/config/management-cluster/monitoring/values.yamlargocd/config/regional-cluster/alerting-rules/templates/hcp-installation.yamlargocd/config/regional-cluster/alerting-rules/templates/hcp-sla.yamlargocd/config/regional-cluster/alerting-rules/templates/hcp-state.yamlargocd/config/regional-cluster/alerting-rules/templates/ratelimit.yamlargocd/config/regional-cluster/alerting-rules/templates/remote-write-health.yamlargocd/config/regional-cluster/eks-nodepool/templates/00-nodeclass.yamlargocd/config/regional-cluster/eks-nodepool/templates/10-nodepool.yamlargocd/config/regional-cluster/grafana/templates/sre-targetgroupbinding.yamlargocd/config/regional-cluster/karpenterargocd/config/regional-cluster/loki/templates/targetgroupbinding.yamlargocd/config/regional-cluster/monitoring/templates/sre-targetgroupbinding.yamlargocd/config/regional-cluster/platform-api/templates/servicemonitor.yamlargocd/config/regional-cluster/platform-api/templates/targetgroupbinding.yamlargocd/config/regional-cluster/thanos/templates/targetgroupbinding.yamlargocd/config/shared/argocd/templates/sre-targetgroupbinding.yamlargocd/config/shared/argocd/values.yamlargocd/config/shared/karpenter/Chart.yamlargocd/config/shared/karpenter/templates/.gitkeepargocd/config/shared/karpenter/values.yamlconfig/templates/argocd-bootstrap/applicationset.yaml.j2deploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldocs/README.mddocs/design/fips-eks-compute.mddocs/design/karpenter-node-provisioning.mddocs/design/zoa-trusted-actions.mddocs/sop/karpenter-lifecycle.mdscripts/test_render.pyterraform/config/management-cluster/main.tfterraform/config/regional-cluster/main.tfterraform/modules/aws-load-balancer-controller/README.mdterraform/modules/aws-load-balancer-controller/versions.tfterraform/modules/ecs-bootstrap/README.mdterraform/modules/ecs-bootstrap/main.tfterraform/modules/ecs-bootstrap/variables.tfterraform/modules/eks-cluster/README.mdterraform/modules/eks-cluster/main.tf
🚧 Files skipped from review as they are similar to previous changes (11)
- terraform/modules/aws-load-balancer-controller/versions.tf
- .spec/002-spec-to-pr-agent/requirements.md
- docs/design/zoa-trusted-actions.md
- deploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yaml
- terraform/modules/aws-load-balancer-controller/README.md
- argocd/config/shared/argocd/values.yaml
- terraform/modules/ecs-bootstrap/README.md
- scripts/test_render.py
- terraform/modules/eks-cluster/main.tf
- docs/README.md
- argocd/config/management-cluster/hypershift/templates/05-job.yaml
openshift-ci
Bot
added
needs-rebase
Contributor
Author
|
/test on-demand-e2e |
theautoroboto
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/bootstrap-argocd.sh`:
- Around line 154-155: Update the ECS override construction in the bootstrap
script to generate JSON with jq -n --arg for all override values, including
REDIS_ENDPOINT and WAIT_FOR_HYPERSHIFT_HEALTH, instead of interpolating strings
into hand-built JSON. Validate WAIT_FOR_HYPERSHIFT_HEALTH and reject any value
other than true or false before generating the payload.
- Around line 113-114: Update the E2E workflow to export
WAIT_FOR_HYPERSHIFT_HEALTH as lowercase "true", validate the variable in
scripts/bootstrap-argocd.sh so only "true" or "false" are accepted, and
JSON-encode the validated value before constructing the --overrides argument.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 40c1d2b4-efe6-45a0-833a-682b96ffe7c5
📒 Files selected for processing (2)
scripts/bootstrap-argocd.shterraform/modules/ecs-bootstrap/main.tf
🚧 Files skipped from review as they are similar to previous changes (1)
- terraform/modules/ecs-bootstrap/main.tf
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/bootstrap-argocd.sh (2)
120-184: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerify the serialized override size before calling ECS.
Amazon ECS limits
RunTaskoverrides to 8,192 characters, including JSON formatting. This payload serializes many ARN and URL values. Measure${#OVERRIDES_JSON}after generation. If the value can approach the limit, use compact JSON and fail before Line 193. (docs.aws.amazon.com)As per path instructions, review shell scripts for command injection vulnerabilities and proper error handling. Verify AWS CLI usage follows security best practices.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bootstrap-argocd.sh` around lines 120 - 184, Update the OVERRIDES_JSON generation and ECS invocation to measure the serialized payload length using ${`#OVERRIDES_JSON`}, generate compact JSON without unnecessary formatting, and fail clearly before the RunTask call when the size approaches or exceeds the 8,192-character limit. Preserve the existing override values and ensure jq and AWS CLI failures are checked rather than silently continuing, with all externally supplied values remaining safely passed through jq arguments.Source: Path instructions
153-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate
CLUSTER_TYPEentry.The payload adds
CLUSTER_TYPEat Line 154 and again at Line 161. Keep one entry per environment variable name. The values are equal now, but duplicate names create ambiguity for future changes. ECS documents this list as the container environment override payload. (docs.aws.amazon.com)Proposed fix
{name: "AWS_REGION", value: $aws_region}, {name: "REGION_DEPLOYMENT", value: $region_deployment}, - {name: "CLUSTER_TYPE", value: $cluster_type}, {name: "API_TARGET_GROUP_ARN", value: $api_target_group_arn},As per path instructions, review shell scripts for command injection vulnerabilities and proper error handling. Verify AWS CLI usage follows security best practices.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bootstrap-argocd.sh` around lines 153 - 161, Remove the second duplicate CLUSTER_TYPE entry from the container environment override payload, keeping the existing earlier entry and preserving all other environment variables unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/bootstrap-argocd.sh`:
- Around line 120-184: Update the OVERRIDES_JSON generation and ECS invocation
to measure the serialized payload length using ${`#OVERRIDES_JSON`}, generate
compact JSON without unnecessary formatting, and fail clearly before the RunTask
call when the size approaches or exceeds the 8,192-character limit. Preserve the
existing override values and ensure jq and AWS CLI failures are checked rather
than silently continuing, with all externally supplied values remaining safely
passed through jq arguments.
- Around line 153-161: Remove the second duplicate CLUSTER_TYPE entry from the
container environment override payload, keeping the existing earlier entry and
preserving all other environment variables unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1d14b9a9-bf99-4bf4-819c-9c6fd56e6973
📒 Files selected for processing (1)
scripts/bootstrap-argocd.sh
Contributor
Author
|
/test on-demand-e2e |
1 similar comment
Contributor
Author
|
/test on-demand-e2e |
3 tasks
Replaces AWS EKS Auto Mode with self-managed Karpenter for more granular control over node provisioning and scaling across Regional and Management Clusters. Key changes: - Remove Auto Mode enablement from EKS module - Deploy Karpenter controller via Helm with GitOps - Configure EC2NodeClass and NodePool resources, pinned to bottlerocket v1.64.0 instead of the floating @latest alias - Add AWS Load Balancer Controller for RC (Karpenter dependency), using the current elbv2.k8s.aws/v1beta1 TargetGroupBinding API, pinned to the same AWS provider version (~> 6.56.0) as the rest of the repo - Upgrade bootstrap node group from t3.medium to t3.large for ArgoCD HA - Applications sync concurrently with no sync-wave ordering, consistent with the project's eventual-consistency ArgoCD model (selfHeal + retry.limit=-1 with backoff). Cross-Application CRD races (eks-nodepool/karpenter, ServiceMonitor+PrometheusRule/monitoring, TargetGroupBinding/aws-load-balancer-controller) are handled via per-resource argocd.argoproj.io/sync-options annotations, matching the existing cert-manager ClusterIssuer pattern, rather than a global SkipDryRunOnMissingResource flag that would mask real bugs everywhere - Remove CriticalAddonsOnly tolerations from the monitoring stack (prometheusOperator, kube-state-metrics, prometheus) — monitoring has no chicken-and-egg bootstrap dependency like ArgoCD/Karpenter do, so it can wait for Karpenter to provision a regular workload node instead of competing for fixed bootstrap-node capacity - Update Helm download with retry logic and SHA-256 checksum verification - Improve HyperShift install error handling (capture output, check stderr) - Restore hyperfleet values.yaml to upstream defaults - Remove stale hyperfleetApi/Sentinel/Adapter valuesObject keys left over from the pre-consolidation chart layout - Restore a response_templates block on the API Gateway default_4xx response that was silently dropped in this branch's rebase history, reverting a real upstream Terraform state-drift fix - Restore force_destroy variable on the regional-oidc module and revert an incorrect platform-monitoring test workaround, both reverted to match upstream/main Hardening added during review: - Gate the post-bootstrap HyperShift Synced+Healthy wait behind an explicit WAIT_FOR_HYPERSHIFT_HEALTH flag so ordinary bootstraps (not just the E2E workflow) skip it, and add --request-timeout to every kubectl call in that loop so a hung connection can't block past the loop's own deadline - Require a non-empty karpenter_controller_role_arn via variable validation instead of allowing a silent empty default - Build the ECS RunTask --overrides payload with jq -n --arg for every value instead of interpolating shell variables into hand-built JSON, removed a duplicated CLUSTER_TYPE entry, and fail clearly before the RunTask call if the serialized payload nears ECS's 8192-character limit - Correct documentation that had fallen out of sync with the implementation: sync-wave ordering claims, IAM role inventory, FIPS/ FedRAMP compliance scope, and stale version references Terminology: "OSS Karpenter" renamed to "self-managed Karpenter" throughout docs, comments, and Terraform descriptions — EKS Auto Mode also runs Karpenter internally, so "OSS" didn't capture the actual distinction; "self-managed" states it directly (ArgoCD-managed lifecycle vs AWS-managed under Auto Mode). Known gaps: - karpenter's own chart-managed ServiceMonitor (from the upstream oci://public.ecr.aws/karpenter chart) has no annotations passthrough for SkipDryRunOnMissingResource, so it still relies on retry/selfHeal alone to recover from a missing monitoring CRD on first bootstrap. - Disabling Prometheus Operator's admissionWebhooks (needed to avoid a PreSync hook Job race during ArgoCD's own bootstrap self-sync — a ttlSecondsAfterFinished-based workaround was tried first and found insufficient) also disables admission-time promql validation for PrometheusRule; malformed rules are applied instead of rejected. - WAIT_FOR_HYPERSHIFT_HEALTH is defined and validated but nothing in this repo sets it to "true" yet — the external CI job definition that chains provisioning immediately into E2E tests needs to export it. Infrastructure validation: - Tested in ephemeral environment with full RC + MC provisioning - Verified node provisioning metrics in CloudWatch - Confirmed Karpenter scales nodes based on pending pods Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
theautoroboto
force-pushed
the
remove-auto-mode-v2
branch
from
August 11, 2026 20:06
4f0a78e to
2ffe502
Compare
…gional-cluster/ argocd/config/management-cluster/karpenter and .../regional-cluster/karpenter were symlinks to shared/karpenter with no consumer: find (used by make helm-lint) doesn't follow symlinked directories by default, so they were never actually linted via these paths, and no script or template references them. The ApplicationSet already discovers shared/karpenter directly for both cluster types. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
/test on-demand-e2e |
…ing gap ArgoCD HA (server, application-controller, repo-server, applicationset, dex, notifications, plus a redis-ha subchart with 3 redis + 3 haproxy pods carrying zero resource requests anywhere in the dependency chain) and the Karpenter controller run on a fixed 2-node group. Explicit resource requests alone already total ~1.75 vCPU / ~3.2 GiB across the two nodes, against ~1.9 vCPU / ~7 GiB allocatable per t3.large (2 vCPU / 8 GiB raw) node, before counting the zero-request redis-ha/haproxy pods or system DaemonSets. Bump to m7i.xlarge (4 vCPU / 16 GiB) for real headroom, and sync every doc/comment reference to the old instance type. Also add the CriticalAddonsOnly toleration to the AWS Load Balancer Controller, which had none — a verified, independent bug that left it unable to schedule until Karpenter provisions untainted nodes. Investigated and explicitly declined a proposal to replace CriticalAddonsOnly tolerations with a PriorityClass on ArgoCD/Karpenter instead: the "toleration sprawl" premise doesn't hold (prometheusOperator, kube-state-metrics, and prometheus carry no such toleration today), and the claim that this would let us drop the monitoring admissionWebhooks/tls workaround doesn't hold either — that workaround exists because ArgoCD's own controller restarts during its own GitOps self-adoption, a reconciliation-timing race that PriorityClass-based preemption has no mechanism to prevent. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ctl path The destroy provisioner tried to reach the cluster API (update-kubeconfig, kubectl delete nodepools) before falling back to tag-based EC2 termination. The CodeBuild project that runs terraform destroy has no VPC connectivity to this fully-private cluster's API, so that path never actually succeeds in practice -- it silently fell through the 2>/dev/null fallback to the same tag-based termination every time, masking a real reachability gap behind what looked like an intentional "cluster already deleted" skip. Since the whole cluster is being destroyed there's no workload to protect by draining gracefully first, so remove the kubectl-dependent phase entirely rather than building a VPC-connected execution path for a step that provides no benefit. Terminating by tag has no dependency on kubeconfig, cluster reachability, or kubectl being installed. Also wrap the instance-terminated wait in an explicit 300s timeout instead of relying on the AWS CLI waiter's implicit one, and document that LBC only reconciles TargetGroupBinding in this architecture (confirmed no Ingress or LBC-provisioned ALB/NLB usage anywhere in argocd/config/), so there's no orphaned load balancer to worry about here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
typeid
reviewed
Aug 12, 2026
| - **list**: Display all tracked environments with status | ||
| - **e2e**: Run end-to-end tests against an environment | ||
| - **dump-env**: Dump environment state (Kubernetes logs and DB state) from clusters | ||
| - **dump-env**: Gather Kubernetes logs and DB state from clusters |
Collaborator
There was a problem hiding this comment.
chore: (I unfortunately missed this in my last review) This is a revert to a previous state not properly picked up by the rebase.
Suggested change
| - **dump-env**: Gather Kubernetes logs and DB state from clusters | |
| - **dump-env**: Dump environment state (Kubernetes logs and DB state) from clusters |
Comment on lines
+77
to
+95
| # Prometheus Operator CRDs must exist before hypershift install applies | ||
| # ServiceMonitor/PrometheusRule. The monitoring chart may still be syncing. | ||
| _CRD_DEADLINE=$((SECONDS + 1800)) | ||
| echo "Waiting for Prometheus Operator CRDs..." | ||
| until _crd_ready servicemonitors.monitoring.coreos.com && \ | ||
| _crd_ready prometheusrules.monitoring.coreos.com; do | ||
| if [ $SECONDS -ge $_CRD_DEADLINE ]; then | ||
| echo "ERROR: Prometheus Operator CRDs not available after 30 minutes" >&2 | ||
| echo "coreos.com CRDs present:" >&2 | ||
| curl -sf --cacert "$_CA" -H "Authorization: Bearer $_TOKEN" \ | ||
| "$_API/apis/apiextensions.k8s.io/v1/customresourcedefinitions" \ | ||
| 2>/dev/null | grep -o '"name":"[^"]*coreos[^"]*"' \ | ||
| || echo " (none or curl failed)" >&2 | ||
| exit 1 | ||
| fi | ||
| echo " Waiting for Prometheus Operator CRDs ($(( _CRD_DEADLINE - SECONDS ))s remaining)..." | ||
| sleep 15 | ||
| done | ||
| echo "=== Prometheus Operator CRDs present — proceeding with hypershift install ===" |
Collaborator
There was a problem hiding this comment.
thought: If you look at our e2es over the past 10 days, you will see they are very stable and should not require this. This PR has a lot of things added just because in some specific case during development something was not working as expected, e.g. here you got stuck on the monitoring CRD not syncing, this is unusual and I don't think it should happen anymore with this iteration of this PR.
In fact, we always ran this installation without the CRDs being there (that's the case on integration) and it won't fail.
Comment on lines
+110
to
+115
| {{- if .Values.hypershift.externalDns.domain }} | ||
| --external-dns-provider aws \ | ||
| --external-dns-domain-filter {{ .Values.hypershift.externalDns.domain }} \ | ||
| --external-dns-secret external-dns \ | ||
| --external-dns-image {{ .Values.hypershift.externalDns.image }} \ | ||
| {{- end }} |
Collaborator
There was a problem hiding this comment.
Question: Why do we need the optionality here? We would never provision this without externaldns.
Comment on lines
+187
to
+190
| tolerations: | ||
| - key: CriticalAddonsOnly | ||
| operator: Exists | ||
| effect: NoSchedule |
Collaborator
There was a problem hiding this comment.
Question: Same as abov,e why CriticalAddonsOnly? This can wait for karpenter / argocd to do its job.
Comment on lines
+166
to
+181
| # Upstream v0.21.0 needs discovery.k8s.io and networking.k8s.io API groups | ||
| # that HyperShift's generated ClusterRole doesn't include. | ||
| # Use JSON Patch (RFC 6902) to add rules without replacing HyperShift's existing rules. | ||
| _patch_http=$(curl -s --cacert "$_CA" -H "Authorization: Bearer $_TOKEN" \ | ||
| -H "Content-Type: application/json-patch+json" -X PATCH \ | ||
| "${_API}/apis/rbac.authorization.k8s.io/v1/clusterroles/external-dns" \ | ||
| -d '[ | ||
| {"op":"add","path":"/rules/-","value":{"apiGroups":["","discovery.k8s.io"],"resources":["services","endpoints","pods","nodes","endpointslices"],"verbs":["get","watch","list"]}}, | ||
| {"op":"add","path":"/rules/-","value":{"apiGroups":["extensions","networking.k8s.io"],"resources":["ingresses","ingressroutes","ingressroutetcps","ingressrouteudps"],"verbs":["get","list","watch"]}}, | ||
| {"op":"add","path":"/rules/-","value":{"apiGroups":["route.openshift.io"],"resources":["routes"],"verbs":["get","list","watch"]}} | ||
| ]' \ | ||
| -o /dev/null -w "%{http_code}" 2>/dev/null || echo "000") | ||
| echo " external-dns clusterrole patch: HTTP ${_patch_http}" | ||
| if [ "$_patch_http" != "200" ] && [ "$_patch_http" != "201" ]; then | ||
| echo "WARNING: external-dns clusterrole patch returned HTTP ${_patch_http} — skipping" >&2 | ||
| fi |
Collaborator
There was a problem hiding this comment.
issue: This whole file is a mix of reverting behavior from the main branch and introducing new behavior. Please disect this to ensure we don't re-add old behavior and only add changes we need.
Comment on lines
+290
to
+323
| # CI: E2E test runner starts immediately after bootstrap exits, so | ||
| # HyperShift must be fully installed before work agents apply | ||
| # HostedCluster manifests. This wait is a CI accommodation — bootstrap | ||
| # has no production requirement to block on application-level health. | ||
| # WAIT_FOR_HYPERSHIFT_HEALTH must be set to "true" by the E2E workflow | ||
| # invocation (e.g. via bootstrap-argocd.sh); ordinary bootstraps leave | ||
| # it unset and skip this wait entirely. | ||
| if [ "$${CLUSTER_TYPE:-}" = "management-cluster" ] && [ "$${WAIT_FOR_HYPERSHIFT_HEALTH:-false}" = "true" ]; then | ||
| echo "=== Waiting for hypershift Application to be Synced and Healthy (up to 30m) ===" | ||
| _HS_DEADLINE=$((SECONDS + 1800)) | ||
| until _HS_STATE=$(kubectl get application hypershift -n argocd --request-timeout=10s \ | ||
| -o jsonpath='{.status.sync.status}|{.status.health.status}' 2>/tmp/hs-err) \ | ||
| && [ "$${_HS_STATE}" = "Synced|Healthy" ]; do | ||
| if grep -qiE "unable to connect|connection refused|i/o timeout|no such host" /tmp/hs-err 2>/dev/null; then | ||
| echo "ERROR: kubectl cannot reach the API server — cannot wait for hypershift:" >&2 | ||
| cat /tmp/hs-err >&2 | ||
| exit 1 | ||
| fi | ||
| if [ $SECONDS -ge $_HS_DEADLINE ]; then | ||
| echo "ERROR: hypershift Application not Synced and Healthy after 30 minutes" >&2 | ||
| kubectl get application hypershift -n argocd --request-timeout=10s -o yaml 2>/dev/null || true | ||
| exit 1 | ||
| fi | ||
| _HS_SYNC=$(kubectl get application hypershift -n argocd --request-timeout=10s \ | ||
| -o jsonpath='{.status.sync.status}' 2>/dev/null || echo "NotFound") | ||
| _HS_HEALTH=$(kubectl get application hypershift -n argocd --request-timeout=10s \ | ||
| -o jsonpath='{.status.health.status}' 2>/dev/null || echo "NotFound") | ||
| _HS_MSG=$(kubectl get application hypershift -n argocd --request-timeout=10s \ | ||
| -o jsonpath='{.status.health.message}' 2>/dev/null || true) | ||
| echo " hypershift sync: $${_HS_SYNC}, health: $${_HS_HEALTH} ($(( _HS_DEADLINE - SECONDS ))s remaining)$${_HS_MSG:+ — $${_HS_MSG}}" | ||
| sleep 15 | ||
| done | ||
| echo "=== hypershift is Synced and Healthy ===" | ||
| fi |
Collaborator
There was a problem hiding this comment.
Suggestion: A health check for "all applications are synced" would make more sense - why specifically hypershift?
Comment on lines
+71
to
+80
| # Disabled: the kube-webhook-certgen PreSync hook Jobs stall monitoring | ||
| # sync during bootstrap when ArgoCD controllers restart and miss the hook | ||
| # completion event (ArgoCD #21055). | ||
| enabled: false | ||
| tls: | ||
| # Disabled alongside admissionWebhooks: the PrometheusOperator Deployment | ||
| # mounts a tls-secret volume gated on tls.enabled (not admissionWebhooks.enabled). | ||
| # Without this flag the pod fails to start because the certgen Job that | ||
| # creates the secret was disabled above. | ||
| enabled: false |
Collaborator
There was a problem hiding this comment.
issue: Similar to above, I think we don't need to drop those anymore. It should run just fine like it does on integration (passing e2e for 10 days).
| # Fields managed by controllers post-deployment that should not cause OutOfSync: | ||
| # LBC rotates its own webhook TLS cert into caBundle and aws-load-balancer-tls. | ||
| # LBC populates TargetGroupBinding status after reconciliation. | ||
| ignoreDifferences: |
Collaborator
There was a problem hiding this comment.
TargetGroupBinding .status:
A desync on .status doesn't make much sense to me, as it would mean any object is desynced.. status always changes it's not a desired state, it's a current state / health signal.
MutatingWebhookConfiguration/ValidatingWebhookConfiguration caBundle
We didn't have those in before and arogcd shows in sync, why is this needed now?
aws-load-balancer-tls
We are not even creating this, I think? It should not even be in argo's owned objects so not cause drift.
| aws_kms_key.eks_secrets | ||
| ] | ||
|
|
||
| # Terminate Karpenter-provisioned EC2 instances before the cluster is deleted. |
Collaborator
There was a problem hiding this comment.
We are still trying to delete nodepools:
else
# Stop Karpenter from creating new nodes by deleting the NodePool
kubectl delete nodepools --all --timeout=30s 2>/dev/null || true
sleep 10
fi
| max_size = 2 | ||
| } | ||
|
|
||
| taint { |
Collaborator
There was a problem hiding this comment.
As discussed, that assessment is wrong.
1 task
Adds CriticalAddonsOnly tolerations required for self-managed Karpenter bootstrap architecture: - Job pod tolerations: allows hypershift-install Job to schedule on karpenter-bootstrap node group (m7i.xlarge, CriticalAddonsOnly taint) - external-dns deployment toleration patch: ensures external-dns (created by hypershift install CLI) can schedule on bootstrap nodes Also removes direct Karpenter version references from documentation: - docs/design/fips-eks-compute.md: remove "v1" - docs/design/karpenter-node-provisioning.md: remove "v1 (1.14.0)" - docs/sop/karpenter-lifecycle.md: use placeholder version in example Without these tolerations, both the hypershift-install Job and external-dns pods would be stuck in Pending state during cluster bootstrap, as Karpenter has not yet provisioned any workload nodes. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Author
|
/test unit-tests |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit