ClusterPool RunningCount #1528

2uasimojo · 2021-09-03T22:37:38Z

Adds ClusterPool.Spec.RunningCount, the number of pool clusters to try to keep active at any time.

Note that when ClusterPool.Spec.HibernateAfter is set, we count from when it was claimed rather than when it was started so it doesn't hibernate early.

HIVE-1576

2uasimojo · 2021-09-03T22:38:17Z

WIP: Test

codecov · 2021-09-09T23:46:49Z

Codecov Report

Merging #1528 (60d73ef) into master (22c6da1) will increase coverage by 0.16%.
The diff coverage is 76.34%.

❗ Current head 60d73ef differs from pull request most recent head 52de679. Consider uploading reports for the commit 52de679 to get more accurate results

@@            Coverage Diff             @@
##           master    #1528      +/-   ##
==========================================
+ Coverage   41.50%   41.66%   +0.16%     
==========================================
  Files         336      336              
  Lines       30691    30690       -1     
==========================================
+ Hits        12737    12788      +51     
+ Misses      16867    16809      -58     
- Partials     1087     1093       +6

Impacted Files	Coverage Δ
...shift/hive/apis/hive/v1/clusterdeployment_types.go	`100.00% <ø> (ø)`
...m/openshift/hive/apis/hive/v1/clusterpool_types.go	`100.00% <ø> (ø)`
pkg/controller/clusterpool/collections.go	`75.82% <70.37%> (-0.79%)`	⬇️
...g/controller/clusterpool/clusterpool_controller.go	`56.31% <76.19%> (+1.47%)`	⬆️
...g/controller/hibernation/hibernation_controller.go	`59.58% <78.94%> (+0.13%)`	⬆️
pkg/test/clusterdeployment/clusterdeployment.go	`96.66% <100.00%> (+0.07%)`	⬆️
pkg/test/clusterpool/clusterpool.go	`97.53% <100.00%> (+0.09%)`	⬆️
pkg/operator/util/apply.go	`0.00% <0.00%> (-3.28%)`	⬇️
pkg/operator/hive/hive.go	`0.00% <0.00%> (ø)`
pkg/operator/assets/bindata.go	`0.00% <0.00%> (ø)`
... and 2 more

2uasimojo · 2021-09-10T19:58:07Z

Rebase picks up #1534 which should reduce e2e-pool flakes.
Still likely to run into #1535 though.

pkg/test/clusterdeployment/clusterdeployment.go

2uasimojo · 2021-09-13T17:52:38Z

Tested live. Works gooood. Still pending UT.

2uasimojo · 2021-09-13T22:20:12Z

UT done

/hold cancel

2uasimojo · 2021-09-14T14:14:58Z

/retest

2uasimojo · 2021-09-14T15:56:35Z

/assign @joelddiaz

2uasimojo · 2021-09-14T16:23:10Z

/test e2e-pool

pkg/controller/clusterpool/clusterpool_controller_test.go

pkg/controller/hibernation/hibernation_controller.go

apis/hive/v1/clusterpool_types.go

pkg/controller/clusterpool/collections.go

joelddiaz · 2021-09-17T14:19:23Z

One overarching thought I've had around this PR is adding the new field to the ClusterDeployment for tracking the claimed timestamp. I understand its purpose, but it does feel a bit like leaking details about ClusterPools down into the ClusterDeployment (which we've already done as I look through other part of the ClusterDeployment.Spec).

I suppose my question is: does everyone feel okay with this (I don't know what to call it: separation of concerns or leaky abstractions or something else)?

2uasimojo · 2021-09-20T15:22:57Z

One overarching thought I've had around this PR is adding the new field to the ClusterDeployment for tracking the claimed timestamp. I understand its purpose, but it does feel a bit like leaking details about ClusterPools down into the ClusterDeployment (which we've already done as I look through other part of the ClusterDeployment.Spec).

Would it be more comfortable if it was ClusterDeployment.Spec.ClusterPoolRef.ClaimedTimestamp instead? There are tradeoffs either way.

The timestamp seems like more of a status-y thing than a spec-y thing.
It feels a lot like InstalledTimestamp.
It's used by the hibernation controller right alongside ⬆️

...but it's set a the same time as ClusterPoolRef.ClaimName; and putting it in there would save us having to do a separate status update.

I'm happy either way, just lmk.

2uasimojo · 2021-09-20T15:49:31Z

/test e2e-pool

2uasimojo · 2021-09-20T21:06:07Z

/hold

Team met today and we decided to do this:

Would it be more comfortable if it was ClusterDeployment.Spec.ClusterPoolRef.ClaimedTimestamp instead?

Resolve a TODO whereby clusters added to satisfy capacity weren't being registered in the cdCollection tracking the state of all the pool clusters. Previously this didn't matter because there was no code after that point that cared; we're about to add some. Prep for HIVE-1576

2uasimojo · 2021-09-21T13:36:15Z

4acce17 rebase only. ClaimedTimestamp changes still forthcoming.

Adds ClusterPool.Spec.RunningCount, the number of pool clusters to try to keep active at any time. Note that when ClusterPool.Spec.HibernateAfter is set, we count from when it was claimed rather than when it was started so it doesn't hibernate early. Adds a ClusterDeployment.Spec.ClusterPoolref.ClaimedTimestamp to track this. HIVE-1576

2uasimojo · 2021-09-21T15:02:44Z

Would it be more comfortable if it was ClusterDeployment.Spec.ClusterPoolRef.ClaimedTimestamp instead?

This is done.

/hold cancel

joelddiaz

looks good to go
/lgtm

openshift-ci · 2021-09-21T18:11:57Z

[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 · 2021-09-21T18:32:43Z

/retest-required

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

openshift-bot · 2021-09-21T18:44:44Z

/retest-required

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

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 3, 2021

openshift-ci bot requested review from gregsheremeta and twiest September 3, 2021 22:38

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2021

2uasimojo force-pushed the HIVE-1576.code branch 2 times, most recently from 253e2c1 to 0aa8e48 Compare September 9, 2021 23:05

2uasimojo force-pushed the HIVE-1576.code branch from 0aa8e48 to 59b91dd Compare September 10, 2021 19:57

2uasimojo commented Sep 13, 2021

View reviewed changes

pkg/test/clusterdeployment/clusterdeployment.go Outdated Show resolved Hide resolved

2uasimojo force-pushed the HIVE-1576.code branch from 59b91dd to c9da459 Compare September 13, 2021 22:20

2uasimojo changed the title ~~WIP: ClusterPool RunningCount~~ ClusterPool RunningCount Sep 13, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2021

openshift-ci bot assigned joelddiaz Sep 14, 2021

2uasimojo force-pushed the HIVE-1576.code branch 2 times, most recently from 60d73ef to de8c987 Compare September 14, 2021 18:10

joelddiaz reviewed Sep 15, 2021

View reviewed changes

pkg/controller/clusterpool/clusterpool_controller_test.go Show resolved Hide resolved

pkg/controller/hibernation/hibernation_controller.go Show resolved Hide resolved

abhinavdahiya reviewed Sep 16, 2021

View reviewed changes

apis/hive/v1/clusterpool_types.go Outdated Show resolved Hide resolved

abhinavdahiya reviewed Sep 16, 2021

View reviewed changes

apis/hive/v1/clusterpool_types.go Show resolved Hide resolved

abhinavdahiya reviewed Sep 16, 2021

View reviewed changes

pkg/controller/clusterpool/collections.go Show resolved Hide resolved

2uasimojo force-pushed the HIVE-1576.code branch from de8c987 to ee27f2d Compare September 20, 2021 15:23

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2021

2uasimojo force-pushed the HIVE-1576.code branch from ee27f2d to 4acce17 Compare September 21, 2021 13:35

2uasimojo force-pushed the HIVE-1576.code branch from 4acce17 to 52de679 Compare September 21, 2021 15:01

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2021

joelddiaz reviewed Sep 21, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2021

openshift-merge-robot merged commit 8bd974e into openshift:master Sep 21, 2021

2uasimojo deleted the HIVE-1576.code branch September 22, 2021 13:32

ClusterPool RunningCount #1528

ClusterPool RunningCount #1528

Uh oh!

Conversation

2uasimojo commented Sep 3, 2021 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

2uasimojo commented Sep 3, 2021

Uh oh!

codecov bot commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

2uasimojo commented Sep 10, 2021

Uh oh!

Uh oh!

2uasimojo commented Sep 13, 2021

Uh oh!

2uasimojo commented Sep 13, 2021

Uh oh!

2uasimojo commented Sep 14, 2021

Uh oh!

2uasimojo commented Sep 14, 2021

Uh oh!

2uasimojo commented Sep 14, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joelddiaz commented Sep 17, 2021

Uh oh!

2uasimojo commented Sep 20, 2021

Uh oh!

2uasimojo commented Sep 20, 2021

Uh oh!

2uasimojo commented Sep 20, 2021

Uh oh!

2uasimojo commented Sep 21, 2021

Uh oh!

2uasimojo commented Sep 21, 2021

Uh oh!

joelddiaz left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Sep 21, 2021

Uh oh!

openshift-bot commented Sep 21, 2021

Uh oh!

openshift-bot commented Sep 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

2uasimojo commented Sep 3, 2021 •

edited by openshift-ci bot

Loading

codecov bot commented Sep 9, 2021 •

edited

Loading