Skip to content

Conversation

@2uasimojo
Copy link
Member

@2uasimojo 2uasimojo commented Oct 5, 2021

In #1434, we added the paradigm of keeping some number of ClusterPool
clusters in Running state, so they don't have to suffer Resuming time
when claimed. However, that work was narrowly focussed on conforming to
the ClusterPool.Spec.RunningCount.

This commit expands the concept to try to make sure we're always running
enough clusters to satisfy claims. In particular, when we are servicing
a number of claims in excess of the pool's capacity, we create that
excess number of clusters... but we previously didn't account for that
number when calculating how many should be Running.

Now we do.

To explain via a corner case: If your pool has a zero Size and you
create a claim, we will create a cluster and immediately assign it. But
because RunningCount is zero, prior to this commit, we would create the
cluster with PowerState=Hibernating, and then kick it over to
Running when we assigned the claim. Now we'll create it with
PowerState=Running in the first place.

HIVE-1651

In openshift#1434, we added the paradigm of keeping some number of ClusterPool
clusters in Running state, so they don't have to suffer Resuming time
when claimed. However, that work was narrowly focussed on conforming to
the ClusterPool.Spec.RunningCount.

This commit expands the concept to try to make sure we're always running
enough clusters to satisfy claims. In particular, when we are servicing
a number of claims in excess of the pool's capacity, we create that
excess number of clusters... but we previously didn't account for that
number when calculating how many should be Running.

Now we do.

To explain via a corner case: If your pool has a zero Size and you
create a claim, we will create a cluster and immediately assign it. But
because RunningCount is zero, prior to this commit, we would create the
cluster with `PowerState=Hibernating`, and then kick it over to
`Running` when we assigned the claim. Now we'll create it with
`PowerState=Running` in the first place.

HIVE-1651
@2uasimojo
Copy link
Member Author

/assign @joelddiaz
/cc @suhanime

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2021
@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #1567 (45664c2) into master (c770a0a) will increase coverage by 0.00%.
The diff coverage is 80.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1567   +/-   ##
=======================================
  Coverage   41.86%   41.87%           
=======================================
  Files         336      336           
  Lines       30884    30887    +3     
=======================================
+ Hits        12931    12934    +3     
  Misses      16864    16864           
  Partials     1089     1089           
Impacted Files Coverage Δ
...g/controller/clusterpool/clusterpool_controller.go 56.52% <80.00%> (+0.21%) ⬆️

@2uasimojo
Copy link
Member Author

/test e2e

Copy link
Contributor

@joelddiaz joelddiaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 6, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2uasimojo, joelddiaz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [2uasimojo,joelddiaz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 291cb11 into openshift:master Oct 6, 2021
@2uasimojo 2uasimojo deleted the HIVE-1651/excessCount branch October 6, 2021 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants