-
Notifications
You must be signed in to change notification settings - Fork 213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NO-ISSUE: Ensure that openshift_version
isn't empty
#6863
base: master
Are you sure you want to change the base?
NO-ISSUE: Ensure that openshift_version
isn't empty
#6863
Conversation
We found a situation where the `openshift_version` column of the clusters table is empty. We haven't been able to understand why, but it results in an error when running validations. For example, the dual-stack VIP validation produces a false negative: ``` 2024-09-11T05:39:04.055138029Z time="2024-09-11T05:39:04Z" level=error msg="Cluster 686c7849-8d68-4340-b81d-9add8c4261fe failed VIP validations" func="github.com/openshift/assisted-service/internal/bminventory.(*bareMetalInventory).validateAndUpdateClusterParams" file="/remote-source/assisted-service/app/internal/bminventory/inventory.go:1889" error="dual-stack VIPs are not supported in OpenShift " pkg=Inventory ``` Note how the error message doesn't contain the OpenShift version at the end. It should, as the code is like this: ```go return common.NewApiError(http.StatusBadRequest, errors.Errorf("%s %s", "dual-stack VIPs are not supported in OpenShift", cluster.OpenshiftVersion)) ``` So we are assuming, at least in that code, that the `openshift_version` column is not empty. This patch changes adds a database constraint to ensure that that `openshift_column` isn't null or empty. That doesn't address the root cause (because we don't know it) but at least it should catch situations where something tries to set that column empty. Related: https://issues.redhat.com/browse/OCPBUGS-42059 Related: https://github.com/openshift/assisted-service/blob/e59df80246f418b62048d7252e4d79bf5723c81e/internal/cluster/validations/validations.go#L256 Signed-off-by: Juan Hernandez <[email protected]>
@jhernand: This pull request explicitly references no jira issue. In 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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6863 +/- ##
==========================================
- Coverage 68.74% 62.38% -6.37%
==========================================
Files 249 249
Lines 37439 37439
==========================================
- Hits 25739 23355 -2384
- Misses 9402 11985 +2583
+ Partials 2298 2099 -199 |
@jhernand: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
We found a situation where the
openshift_version
column of the clusters table is empty. We haven't been able to understand why, but it results in an error when running validations. For example, the dual-stack VIP validation produces a false negative:Note how the error message doesn't contain the OpenShift version at the end. It should, as the code is like this:
So we are assuming, at least in that code, that the
openshift_version
column is not empty.This patch adds a database constraint to ensure that that
openshift_column
isn't null or empty. That doesn't address the root cause (because we don't know it) but at least it should catch situations where something tries to set that column empty.Related: https://issues.redhat.com/browse/OCPBUGS-42059
Related:
assisted-service/internal/cluster/validations/validations.go
Line 256 in e59df80
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs
, README, etc)Reviewers Checklist