✨Refactor CreateInstance and CreateBastion #1191
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Common network and security group handling between CreateBastion() and CreateInstance(). A principal advantage of this refactor is that it makes the marshalling of OpenStackMachineSpec and Instance respectively into an InstanceSpec a cheap operation which makes no API calls.
Refactor instance creation in machine controller and cluster controller (for the bastion) to call compute.CreateInstance() with an InstanceSpec.
k8s-ci-robot
added
the
do-not-merge/hold
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
I like this refactoring a lot! /lgtm |
k8s-ci-robot
added
the
lgtm
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
The primary purpose of this this PR is to cleanup the interface of
compute.CreateInstanceand make the separation of concerns between the machine controller (for Machines), the cluster controller (for the Bastion), and compute (for actual server creation) better defined.The Bastion host is represented in the cluster spec as an
Instance. An OpenStackMachine is represented by context inOpenStackMachineas well asOpenStackCluster. So while they both create a server in the same way, they both source the server's parameters in slightly different ways using different source objects.At some point in history we also used
Instanceas the intermediate representation for an OpenStackMachine. That is, we combined parameters from anOpenStackMachineand anOpenStackClusterinto anInstance, then passed that to CreateInstance.Instanceis not ideal for this purpose as it is both Spec and Status. It contains fields which cannot be used as input parameters to CreateInstance. Therefore we refactored this into the internal-onlyInstanceSpec, which contains only spec fields.This refactor takes this further. Firstly we ensure that creating an InstanceSpec from an OpenStackMachine or a cluster bastion is strictly data transformation and therefore very cheap. Anything expensive moves into the new CreateInstance. Secondly we move this code to the controller where it is relevant, and add new unit tests covering the transformation. This also reduces the scope of the CreateInstance unit tests, making them simpler.
Special notes for your reviewer:
I don't intend to squash these commits. They are intended to be independent logical steps. You may find it easier to review this PR by commit rather than as a single change.
This is the code motion portion of #1153 with functional changes to Bastion reconciliation removed.
/hold