MGMT-20119: Create feature gate to allow installing TNA using the API #7523
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@giladravid16: This pull request references MGMT-20119 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 task to target the "4.19.0" version, but no target version was set. 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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
size/L
giladravid16
force-pushed
the
MGMT-20119
branch
from
111b84d to
c4ad2fe
Compare
giladravid16
force-pushed
the
MGMT-20119
branch
2 times, most recently
from
0597f75 to
3375cdd
Compare
openshift-ci
bot
added
size/XXL
giladravid16
force-pushed
the
MGMT-20119
branch
from
3375cdd to
7de16eb
Compare
|
@giladravid16: This pull request references MGMT-20119 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 task to target the "4.19.0" version, but no target version was set. 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. |
1 similar comment
|
@giladravid16: This pull request references MGMT-20119 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 task to target the "4.19.0" version, but no target version was set. 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. |
|
@giladravid16: This pull request references MGMT-20119 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 task to target the "4.19.0" version, but no target version was set. 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. |
1 similar comment
|
@giladravid16: This pull request references MGMT-20119 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 task to target the "4.19.0" version, but no target version was set. 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. |
giladravid16
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7523 +/- ##
==========================================
- Coverage 67.30% 67.26% -0.05%
==========================================
Files 334 335 +1
Lines 42293 42621 +328
==========================================
+ Hits 28465 28667 +202
- Misses 11256 11361 +105
- Partials 2572 2593 +21
🚀 New features to boost your workflow:
|
internal/bminventory/inventory.go
Outdated
| if highAvailabilityMode == models.ClusterCreateParamsHighAvailabilityModeFull { | ||
| var minMasterHostsNeededForInstallation int64 = common.MinMasterHostsNeededForInstallationInHaMode | ||
| minVersion := common.MinimumVersionForNonStandardHAOCPControlPlane | ||
| if !arbiterClustersNotSupported { |
There was a problem hiding this comment.
I'd prefer this get reversed in some way so we don't have a double negative here. It's harder to reason about.
internal/bminventory/inventory.go
Outdated
| err = errors.Errorf("Cannot set role arbiter to host %s in infra-env %s, it must be bound to a cluster with openshift version %s or newer", host.ID, host.InfraEnvID, common.MinimumVersionForArbiterClusters) | ||
| if cluster != nil && cluster.OpenshiftVersion != "" { | ||
| arbiterClustersNotSupported, err2 := common.BaseVersionLessThan(common.MinimumVersionForArbiterClusters, cluster.OpenshiftVersion) | ||
| if err2 != nil { | ||
| return err2 | ||
| } | ||
| if !arbiterClustersNotSupported { | ||
| err = nil | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic feels a bit backwards. Why create the error first just to nil it out if we hit the success case?
Can you refactor this?
giladravid16
force-pushed
the
MGMT-20119
branch
from
7de16eb to
91d052d
Compare
|
/retest-required |
internal/bminventory/inventory.go
Outdated
| var err error | ||
| if !b.TNAClustersSupport { | ||
| err = errors.Errorf("TNA clusters support is disabled, cannot set role arbiter to host %s in infra-env %s", host.ID, host.InfraEnvID) | ||
| } else { | ||
| if cluster != nil && cluster.OpenshiftVersion != "" { | ||
| arbiterClustersSupported, err2 := common.BaseVersionGreaterOrEqual(common.MinimumVersionForArbiterClusters, cluster.OpenshiftVersion) | ||
| if err2 != nil { | ||
| return err2 | ||
| } | ||
| if !arbiterClustersSupported { | ||
| err = errors.Errorf("Cannot set role arbiter to host %s in infra-env %s, it must be bound to a cluster with openshift version %s or newer", host.ID, host.InfraEnvID, common.MinimumVersionForArbiterClusters) | ||
| } | ||
| } else { | ||
| err = errors.Errorf("Cannot set role arbiter to host %s in infra-env %s, it must be bound to a cluster with openshift version %s or newer", host.ID, host.InfraEnvID, common.MinimumVersionForArbiterClusters) | ||
| } | ||
| } | ||
| if err != nil { | ||
| log.Error(err) | ||
| return common.NewApiError(http.StatusBadRequest, err) | ||
| } |
There was a problem hiding this comment.
| var err error | |
| if !b.TNAClustersSupport { | |
| err = errors.Errorf("TNA clusters support is disabled, cannot set role arbiter to host %s in infra-env %s", host.ID, host.InfraEnvID) | |
| } else { | |
| if cluster != nil && cluster.OpenshiftVersion != "" { | |
| arbiterClustersSupported, err2 := common.BaseVersionGreaterOrEqual(common.MinimumVersionForArbiterClusters, cluster.OpenshiftVersion) | |
| if err2 != nil { | |
| return err2 | |
| } | |
| if !arbiterClustersSupported { | |
| err = errors.Errorf("Cannot set role arbiter to host %s in infra-env %s, it must be bound to a cluster with openshift version %s or newer", host.ID, host.InfraEnvID, common.MinimumVersionForArbiterClusters) | |
| } | |
| } else { | |
| err = errors.Errorf("Cannot set role arbiter to host %s in infra-env %s, it must be bound to a cluster with openshift version %s or newer", host.ID, host.InfraEnvID, common.MinimumVersionForArbiterClusters) | |
| } | |
| } | |
| if err != nil { | |
| log.Error(err) | |
| return common.NewApiError(http.StatusBadRequest, err) | |
| } | |
| if !b.TNAClustersSupport { | |
| err := errors.Errorf("TNA clusters support is disabled, cannot set role arbiter to host %s in infra-env %s", host.ID, host.InfraEnvID) | |
| log.Error(err) | |
| return common.NewApiError(http.StatusBadRequest, err) | |
| } | |
| if cluster == nil || cluster.OpenshiftVersion == nil { | |
| err := errors.Errorf("Cannot set role arbiter to host %s in infra-env %s, it must be bound to a cluster with openshift version %s or newer", host.ID, host.InfraEnvID, common.MinimumVersionForArbiterClusters) | |
| log.Error(err) | |
| return common.NewApiError(http.StatusBadRequest, err) | |
| } | |
| arbiterClustersSupported, err := common.BaseVersionGreaterOrEqual(common.MinimumVersionForArbiterClusters, cluster.OpenshiftVersion) | |
| if err != nil { | |
| return err | |
| } | |
| if !arbiterClustersSupported { | |
| err := errors.Errorf("Cannot set role arbiter to host %s in infra-env %s, it must be bound to a cluster with openshift version %s or newer", host.ID, host.InfraEnvID, common.MinimumVersionForArbiterClusters) | |
| log.Error(err) | |
| return common.NewApiError(http.StatusBadRequest, err) | |
| } |
Not super urgent, but I generally prefer guards with early returns to a lot of "else" statements. I find something like this easier to read because you don't have to consider multiple conditions at the same time when thinking about why some code is running. Additionally you don't have to worry about error shadowing here.
Also if we want to deduplicate the logging I'd suggest logging the error from the caller rather than every error case in this function.
There was a problem hiding this comment.
Updated it like you suggested
giladravid16
force-pushed
the
MGMT-20119
branch
from
91d052d to
f9fc5dd
Compare
|
/retest |
internal/cluster/validator_test.go
Outdated
|
|
||
| status, message := validator.SufficientMastersCount(preprocessContext) | ||
| Expect(status).To(Equal(ValidationFailure)) | ||
| Expect(message).To(Equal("The cluster must have exactly 2 dedicated control plane nodes. Add or remove hosts, or change their roles configurations to meet the requirement.")) |
There was a problem hiding this comment.
Should this message be updated in some way? It seems like what it is complaining about is satisfied (it wan't 2 CP nodes and we have 2 CP nodes...)
There was a problem hiding this comment.
In general yes, but because we decided that users won't tell us in advance that they want a TNA cluster, an error message that tells the users they need to add at least 1 arbiter node won't be correct when using TNF.
Also the validator doesn't know if TNA is supported (or any other feature gate), it just assumes that if some cluster has a feature (like 5 CP nodes) then it's fine. But when we have different features gates for TNA and TNF, then we won't have a clear way of knowing which one it should check.
If TNF is something that we will force users to tell us in advance, then we can consider TNA clusters to be the default cluster topology with 2 CP nodes. Do you want to do that?
There was a problem hiding this comment.
I think we should ignore TNF in this case for now. Ideally we should update the message to state the actual options (2CP nodes with arbiter or 3 CP nodes).
If I had to guess I'd say we're going to know of a cluster is TNF because it will likely look like a provider integration or some additional fields/flag at cluster creation.
|
/retest |
giladravid16
force-pushed
the
MGMT-20119
branch
from
f9fc5dd to
f3181c5
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin, giladravid16 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 |
1 similar comment
|
/retest |
|
/retest-required |
1 similar comment
|
/retest-required |
|
@giladravid16: The following test 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. |
openshift-merge-bot
bot
merged commit 07e5c43
into
openshift:master
|
[ART PR BUILD NOTIFIER] Distgit: ose-agent-installer-api-server |
This is the first PR for adding support for TNA Clusters in assisted-service.
The purpose of this PR is to add the minimum required to install TNA clusters using the REST API.
The changes include:
To check this PR I used my build to install a few TNA clusters and verified that the clusters installed successfully.
List all the issues related to this PR
Closes MGMT-20119
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist