OCPBUGS-14877: Validate that number hosts does not exceed replicas#7268
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@bfournie: This pull request references Jira Issue OCPBUGS-14877, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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 kubernetes/test-infra repository. |
zaneb
left a comment
zaneb
left a comment
There was a problem hiding this comment.
It's never an error to not have enough hosts in a role, only too many.
Unit tests are failing which may be related.
There was a problem hiding this comment.
The effect of this is that the user has to specify either all or none of the masters. Since each of the masters is theoretically optional, and this is a new check, I definitely don't think this is something we could backport.
For this to be an error, I think instead of the condition numMasters != 0 && numMasters < numRequiredMasters it should be:
numMasters < numRequiredMasters && (numRequiredMasters - numMasters) > ((numRequiredMasters + numRequiredWorkers) - (numMasters + numWorkers))
i.e. the number of masters without a host definition is greater than the number of remaining undefined hosts.
This simplifies to numMasters < numRequiredMasters && numWorkers > numRequiredWorkers. Since this is a strict subset of the check on line 380, there's no need to return an error here at all.
It would still be unusual for users to configure hosts for only some of the masters, so I think we should change this error back to a warning.
There was a problem hiding this comment.
Likewise, for this to be an error then instead of numWorkers != 0 && numWorkers < numRequiredWorkers) this condition should be something like
numWorkers < numRequiredWorkers && (numRequiredWorkers - numWorkers) > ((numRequiredMasters + numRequiredWorkers) - (numMasters + numWorkers) - (numRequiredMasters - numMasters))
i.e. the number of workers without a host definition is greater than the number of remaining undefined hosts excluding undefined hosts that should be masters. (This assumes that we don't have too many masters defined, which is already causes an error on line 371.)
It turns out that this can never be true, because it simplifies to numWorkers < numRequiredWorkers && (numRequiredWorkers - numWorkers) > (numRequiredWorkers - numWorkers) so there is no time we want to return an error here.
Let's change this one back to a warning also.
There was a problem hiding this comment.
Technically this can't be true when numMasters == 0, since numRequiredMasters can't be negative so it doesn't matter, but it's a bit confusing having this inside the if numMasters != 0 block.
There was a problem hiding this comment.
Same comment about nesting this inside if numWorkers != 0
Yeah we can't know if not enough hosts are defined that other hosts aren't being booted, I'll remove the error. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
A check was added in https://issues.redhat.com//browse/OCPBUGS-10342 to warn when the number of replicas exceeded the configured hosts, however this did not catch the case when the number of configured hosts exceeds the replicas, so it is added here. In addition this validation will now result in an error instead of a warning since this invalid configuration will cause installation failures.
|
@bfournie: This pull request references Jira Issue OCPBUGS-14877, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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 kubernetes/test-infra repository. |
|
@bfournie: 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/test-infra repository. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pawanpinjarkar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@bfournie: Jira Issue OCPBUGS-14877: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-14877 has been moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
6 participants
A check was added in https://issues.redhat.com//browse/OCPBUGS-10342 to warn when the number of replicas exceeded the configured hosts, however this did not catch the case when the number of configured hosts exceeds the replicas, so it is added here. In addition if too many hosts are defined compared to the replicas it will result in an error instead of a warning, since this invalid configuration will cause installation failures.