WIP: CNTRLPLANE-3395: Lower maximum allowed etcd quota from 32 to 16GiB#2840
WIP: CNTRLPLANE-3395: Lower maximum allowed etcd quota from 32 to 16GiB#2840dusk125 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@dusk125: This pull request references CNTRLPLANE-3395 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 story 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. |
|
Hello @dusk125! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughThis change reduces the maximum allowed value for the 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
|
[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 |
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 `@operator/v1/types_etcd.go`:
- Around line 45-49: The BackendQuotaGiB validation in operator/v1/types_etcd.go
was lowered to a maximum of 16, which breaks tests and existing CRs; update the
test suite operator/v1/tests/etcds.operator.openshift.io/EtcdBackendQuota.yaml
by removing or changing the case "Should be able to create with 32
BackendQuotaGiB" (and any other cases using values >16) and update expected
validation error strings to reference max 16 instead of 32 for tests that assert
failure; after adjusting tests, ensure you either document/implement a CRD
versioning/conversion or migration strategy for existing Etcd resources that may
have BackendQuotaGiB ≥17 so cluster upgrades remain safe (verify compliance with
OpenShift CRD upgrade rules).
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e01d9449-15f8-4b64-908f-fb28441d8061
⛔ Files ignored due to path filters (6)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests/etcds.operator.openshift.io/EtcdBackendQuota.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (4)
operator/v1/types_etcd.gopayload-manifests/crds/0000_12_etcd_01_etcds-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_12_etcd_01_etcds-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_12_etcd_01_etcds-TechPreviewNoUpgrade.crd.yaml
|
@dusk125: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
Are we able to check to telemetry data to see what the field currently has set for this value? Has anyone tried increasing this value above 16 presently? |
This is in Tech Preview which we're currently trying to get to GA. Our internal testing has shown that, currently, 32GiB is too large and will very likely cause cluster instability. Also, using the logic that it's easier to increase the maximum later, than it is to ship this at 32 and later limit the maximum to 16 |
|
Ack, missed that this was TP, in which case all good Any reason this is still in WIP? |
No problem! Yeah we're still aligning that this is what we want to do, and leaving this unmerged allows us to test up to the 32GiB to validate cluster stability (and see if we can indeed offer up to 32) |
|
@JoelSpeed am I correct in assuming that increasing this maximum constraint in a minor or patch release of openshift is not a big deal? Meaning if we were to ship 16, and later we validate that 32 is stable, we could, without disruption, update this api constraint? |
Yes, we generally allow relaxing validation in cases like this |
No description provided.