Skip to content

e2e-pool: Start real pool provisions after inventory added#2080

Merged
openshift-merge-robot merged 1 commit into
openshift:masterfrom
2uasimojo:e2e-pool/inventory-stale
Aug 9, 2023
Merged

e2e-pool: Start real pool provisions after inventory added#2080
openshift-merge-robot merged 1 commit into
openshift:masterfrom
2uasimojo:e2e-pool/inventory-stale

Conversation

@2uasimojo
Copy link
Copy Markdown
Member

Goal: reduce e2e-pool wallclock time by ~35m.

Problem Statement: When ClusterPool inventory
(ClusterDeploymentCustomization) testing was added to e2e-pool (4fddbe7 / #1672), it triggered ClusterPool's staleness algorithm such that we were actually wasting a whole cluster while waiting for the real pool to become ready. Grab a cup of coffee...

To make the flow of the test a little bit easier, we were creating the real pool, then using its definition to generate the fake pool definition -- which does not have inventory -- and then adding inventory to the real pool.

But if you add or change a pool's inventory, we mark all its clusters stale. So because of the flow above, when we initially created the real pool without inventory, it started provisioning a cluster. Then when we updated it (mere seconds later, if that), that cluster immediately became stale.

Now, the way we decided to architect replacement of stale clusters, we prioritize having claimable clusters over all clusters being current. Thus in this scenario we were actually ending up waiting until the stale cluster was fully provisioned before deleting it and starting over with the (inventory-affected) cluster.

Solution: Create the real pool with an initial size=0. Scale it up to size=1 after adding the inventory.

@openshift-ci openshift-ci Bot requested review from lleshchi and suhanime August 8, 2023 22:38
@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 8, 2023
@2uasimojo
Copy link
Copy Markdown
Member Author

/assign @dlom

/cc @abraverm

@openshift-ci openshift-ci Bot requested a review from abraverm August 8, 2023 22:38
@dlom
Copy link
Copy Markdown
Contributor

dlom commented Aug 8, 2023

/lgtm

Nice detective work

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2023
@2uasimojo
Copy link
Copy Markdown
Member Author

/test e2e

Goal: reduce e2e-pool wallclock time by ~35m.

Problem Statement: When ClusterPool inventory
(ClusterDeploymentCustomization) testing was added to e2e-pool (4fddbe7
/ openshift#1672), it triggered ClusterPool's staleness algorithm such that we
were actually wasting a whole cluster while waiting for the real pool to
become ready. Grab a cup of coffee...

To make the flow of the test a little bit easier, we were creating the
real pool, then using its definition to generate the fake pool
definition -- which does not have inventory -- and then adding inventory
to the real pool.

But if you add or change a pool's inventory, we mark all its clusters
stale. So because of the flow above, when we initially created the real
pool without inventory, it started provisioning a cluster. Then when we
updated it (mere seconds later, if that), that cluster immediately
became stale.

Now, the way we decided to architect replacement of stale clusters, we
prioritize _having claimable clusters_ over _all clusters being
current_. Thus in this scenario we were actually ending up waiting until
the stale cluster was fully provisioned before deleting it and starting
over with the (inventory-affected) cluster.

Solution: Create the real pool with an initial `size=0`. Scale it up to
`size=1` _after_ adding the inventory.
@2uasimojo 2uasimojo force-pushed the e2e-pool/inventory-stale branch from 653387b to c891224 Compare August 8, 2023 23:59
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2023
@2uasimojo
Copy link
Copy Markdown
Member Author

@dlom I did a silly, I can't have comments in the middle of a backslash-escaped multi-line shell command.

@dlom
Copy link
Copy Markdown
Contributor

dlom commented Aug 9, 2023

Good to know, I didn't know that was illegal

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2023
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Aug 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Aug 9, 2023

@2uasimojo: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 01532c6 into openshift:master Aug 9, 2023
@2uasimojo 2uasimojo deleted the e2e-pool/inventory-stale branch August 9, 2023 14:43
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.

3 participants