OSASINFRA-3420: openstack: Decouple OpenStack API calls from Machine generation#8187
Conversation
|
/cc mandre 2uasimojo patrickdillon |
|
This is great, and will suit hive's needs nicely, thanks @pierreprinetti! |
|
cf. HIVE-2476 |
|
/approve |
To make unit tests possible (and separate responsibilities), remove OSP API calls from the functions that generate Machines and MachineSets.
pierreprinetti
force-pushed
the
declouple_trunkcheck
branch
from
65fa6b2 to
b99470c
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon 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
|
/lgtm |
|
@pierreprinetti: 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. |
|
Critical fixes has been turned off; how do we make this go? |
pierreprinetti
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@pierreprinetti: This pull request references OSASINFRA-3420 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.16.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@pierreprinetti: This pull request references OSASINFRA-3420 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[ART PR BUILD NOTIFIER] This PR has been included in build ose-installer-altinfra-container-v4.16.0-202403251146.p0.gb873ca2.assembly.stream.el8 for distgit ose-installer-altinfra. |
|
Ugh, I'm sorry I didn't notice this on review, but we're going to need to do this again :( TL;DR: we need to be able to pass custom client opts into CheckNetworkExtensionAvailability(), because we have knobs for custom certs and whatnot. I'll post the PR. |
|
@2uasimojo Indeed I removed that because the code in Installer only ever passed |
Totally, it just looked like tech debt.
Will do. Thanks! |
Specifically to pick up openshift/installer#8187 and openshift/installer#8209 so we can fix our unit tests around MachinePools+OpenStack. But also just because we do this periodically. Needed by HIVE-2476
A prior commit revendored installer to pull in openshift/installer#8187 and openshift/installer#8209 which decoupled the OpenStack cloud call for trunk support discovery from the `MachineSets()` generator. This commit refactors our invocation of that generator accordingly, and repairs our long-broken unit test framework around it by attaching a `trunkSupportDiscoverer` func to the actuator. In the production code path, this is set to the `CheckNetworkExtensionAvailability` function publicized by the aforementioned installer PRs, whereas in the unit test path it is set to a no-op shim. Future tests can make this shim return any permutation of possible values to exercise different code paths. Subsequent commits will enhance this unit test suite to reproduce the issue in the referenced card. Part of HIVE-2476
A prior commit revendored installer to pull in openshift/installer#8187 and openshift/installer#8209 which decoupled the OpenStack cloud call for trunk support discovery from the `MachineSets()` generator. This commit refactors our invocation of that generator accordingly, and repairs our long-broken unit test framework around it by attaching a `trunkSupportDiscoverer` func to the actuator. In the production code path, this is set to the `CheckNetworkExtensionAvailability` function publicized by the aforementioned installer PRs, whereas in the unit test path it is set to a no-op shim. Future tests can make this shim return any permutation of possible values to exercise different code paths. Subsequent commits will enhance this unit test suite to reproduce the issue in the referenced card. Part of HIVE-2476
Specifically to pick up openshift/installer#8187 and openshift/installer#8209 so we can fix our unit tests around MachinePools+OpenStack. But also just because we do this periodically. Needed by HIVE-2476
A prior commit revendored installer to pull in openshift/installer#8187 and openshift/installer#8209 which decoupled the OpenStack cloud call for trunk support discovery from the `MachineSets()` generator. This commit refactors our invocation of that generator accordingly, and repairs our long-broken unit test framework around it by attaching a `trunkSupportDiscoverer` func to the actuator. In the production code path, this is set to the `CheckNetworkExtensionAvailability` function publicized by the aforementioned installer PRs, whereas in the unit test path it is set to a no-op shim. Future tests can make this shim return any permutation of possible values to exercise different code paths. Subsequent commits will enhance this unit test suite to reproduce the issue in the referenced card. Part of HIVE-2476
This is *not* a cherry-pick of openshift#2253 / (c6b37ee & f59f327). That solution revendored installer to pick up the fix from upstream. In older branches, this would have dragged in too many dependencies, so we instead fix it "locally" with an explicit nil check. Note also that the original fix added unit tests. We can't do that here either because the new tests rely on the OpenStack UT suite being un-broken [1], which again relied on upstream changes [2][3] we can't pull into older branches. [1] openshift#2251 [2] openshift/installer#8187 [3] openshift/installer#8209 HIVE-2476
This is *not* a cherry-pick of openshift#2253 / (c6b37ee & f59f327). That solution revendored installer to pick up the fix from upstream. In older branches, this would have dragged in too many dependencies, so we instead fix it "locally" with an explicit nil check. Note also that the original fix added unit tests. We can't do that here either because the new tests rely on the OpenStack UT suite being un-broken [1], which again relied on upstream changes [2][3] we can't pull into older branches. [1] openshift#2251 [2] openshift/installer#8187 [3] openshift/installer#8209 HIVE-2476
This is *not* a cherry-pick of openshift#2253 / (c6b37ee & f59f327). That solution revendored installer to pick up the fix from upstream. In older branches, this would have dragged in too many dependencies, so we instead fix it "locally" with an explicit nil check. Note also that the original fix added unit tests. We can't do that here either because the new tests rely on the OpenStack UT suite being un-broken [1], which again relied on upstream changes [2][3] we can't pull into older branches. [1] openshift#2251 [2] openshift/installer#8187 [3] openshift/installer#8209 HIVE-2476
6 participants
To make unit tests possible (and separate responsibilities), remove OSP API calls from the functions that generate Machines and MachineSets.