Revendor installer, repair OpenStack MachinePool UT#2251
Conversation
openshift-ci
bot
added
the
approved
|
/assign @lleshchi |
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
| logger: logger, | ||
| osImage: osImage, | ||
| kubeClient: kubeClient, | ||
| trunkSupportDiscoverer: installosp.CheckNetworkExtensionAvailability, |
There was a problem hiding this comment.
nit: the function can be scoped to only care about the Trunk extension, if that's all we're interested about:
type OpenStackActuator struct {
// ...
trunkSupportDiscoverer func(cloud string, opts *clientconfig.ClientOpts) (bool, error)actuator := &OpenStackActuator{
// ...
trunkSupportDiscoverer: func(cloud string, opts *clientconfig.Clientopts) (bool, error) {
return installosp.CheckNetworkExtensionAvailability(cloud, "trunk", opts)
},There was a problem hiding this comment.
Thanks for the review!
I like your suggestion. (And actually since we know we're passing in a non-nil opts we could even use a dummy/nil value for cloud -- though that does require knowledge of the internals of CheckNetworkExtensionAvailability and is thus not particularly future-proof.)
I feel like this controller has way too many func-of-func indirections already, and the existing approach is just a tiny bit cleaner looking. So I might keep it.
@lleshchi what are your thoughts?
There was a problem hiding this comment.
I also think you should use what your team is used to! :)
There was a problem hiding this comment.
I do think the current approach is cleaner in terms of readability
2uasimojo
force-pushed
the
HIVE-2476/repair-openstack-machinepool-ut
branch
from
a493487 to
632f4be
Compare
|
/cc @patrickdillon would you please have a look at the third commit, which incorporates the new CAPI SG filters from openshift/installer#8006 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2251 +/- ##
==========================================
+ Coverage 58.32% 58.47% +0.14%
==========================================
Files 182 182
Lines 25704 25719 +15
==========================================
+ Hits 14993 15038 +45
+ Misses 9452 9411 -41
- Partials 1259 1270 +11
|
|
/test e2e-gcp |
A previous commit revendored installer which picked up openshift/installer#8006 which adds two default security group filters. We need to take this into account when the `extra-worker-security-group` annotation is in play, as we need to add that additional SG name and its VPC filter to *all* the SecurityGroups in the worker MachineSets. This needs to be done both for day 0 (install manager) and day 2 (MachinePool controller). Carried along with HIVE-2476
2uasimojo
force-pushed
the
HIVE-2476/repair-openstack-machinepool-ut
branch
from
632f4be to
6de60ba
Compare
|
/test e2e-gcp |
1 similar comment
|
/test e2e-gcp |
|
@2uasimojo: all tests passed! 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo, lleshchi 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 |
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
| assert.Contains(t, awsMachineTemplate.SecurityGroups[0].Filters[1].Values, "testvpc123", "expected testvpc123 to be configured within Security Group Filters in AWSMachineProviderConfig") | ||
| if assert.Equal(t, tc.expectModified, len(awsMachineTemplate.SecurityGroups), "unexpected number of security groups") { | ||
| for i := 0; i < tc.expectModified; i++ { | ||
| assert.Contains(t, awsMachineTemplate.SecurityGroups[i].Filters[0].Values, "test-security-group", "expected test-security-group to be configured within Security Group Filters in AWSMachineProviderConfig") |
There was a problem hiding this comment.
Need to verify that this contains the original name (test-czcpt-worker-sg) as well!
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
3 participants
MachineSets()generatorA subsequent PR will use this to
Part of HIVE-2476