✨ Refactor: Don't use infrav1.Instance internally #971

mdbooth · 2021-08-16T15:10:21Z

This is predominantly a non-functional change. It contains some minor
changes to log messages, and omits a check for instance name being empty
prior to delete in OpenStackMachine controller because this check is no
longer relevant when we delete by ID. Apart from that it contains no
(deliberate) functional changes.

Currently we use infrav1.Instance as our internal representation of an
OpenStack instance. There are several problems with this:

infrav1.Instance is part of the API, so is hard to change for internal
use
infrav1.Instance is used to represent both the 'spec' and 'status' of
an instance
When used as a spec for the Bastion host, it allows fields to be set
which will be ignored
When used as the status of an instance fetched from OpenStack not all
fields are populated, leading to potential accidental usage errors

This change creates several new types representing different types of
instance data purely for internal use. This has the following
advantages:

As they are not API types they can be updated easily
When used in a function signature they make it clear what data is
being used
The type system will prevent accidental use of uninitialised data

The new types are:

InstanceSpec
InstanceIdentifier
InstanceStatus
InstanceNetworkStatus

They are defined and described in instance_types.go.

The primary driver for this work is to eventually fix the documented
errors in InstanceNetworkStatus in IP() and FloatingIP().

What this PR does / why we need it:

This is a refactor in support of a future change to fix #926. I would like to merge these in separate PRs to separate potential errors introduced by a refactor from the functional changes required to fix the issue.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Special notes for your reviewer:

I suggest the easiest way to read this change is:

Read the commit message
Read instance_types.go
Everything else is pretty much a mechanical consequence

/hold

jichenjc · 2021-08-17T01:11:23Z

controllers/openstackcluster_controller.go

@@ -196,24 +196,34 @@ func deleteBastion(log logr.Logger, osProviderClient *gophercloud.ProviderClient
 		return err
 	}

-	instance, err := computeService.GetInstanceByName(openStackCluster, fmt.Sprintf("%s-bastion", cluster.Name))
+	instanceStatus, err := computeService.GetInstanceByName(openStackCluster, fmt.Sprintf("%s-bastion", cluster.Name))


not sure we need rename to GetInstanceStatusByName?

I'm inclined to leave it for now because it's already getting a bit long, but it's easy to change if we decide it's confusing. I think it should be ok, though, because now everything returns InstanceStatus.

+1 for rename it to GetInstanceStatusByName

jichenjc · 2021-08-17T01:20:48Z

pkg/cloud/services/compute/instance_types.go

@@ -0,0 +1,171 @@
+/*
+Copyright 2018 The Kubernetes Authors.


jichenjc · 2021-08-17T01:30:54Z

pkg/cloud/services/compute/instance_types.go

+}
+
+// APIInstance returns an infrav1.Instance object for use by the API.
+func (is *InstanceStatus) APIInstance() (*infrav1.Instance, error) {


looks like we have more param for Metadata, ConfigDrive etc for Infrav1.Instance
will it be inited here or somewhere else?

That's the intention, although I don't currently think we'll add any paramaters to APIInstance() specifically. We will almost definitely add parameters to NetworkStatus(), though.

hidekazuna · 2021-08-19T04:49:40Z

/lgtm

mdbooth · 2021-08-19T10:50:44Z

I've rebased on to master and fixed the copyright nit. No other changes.

jichenjc · 2021-08-20T02:32:25Z

/approve

ok, I think we can do additional update over time, so far it looks great

k8s-ci-robot · 2021-08-20T02:32:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc, 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:

~~OWNERS~~ [jichenjc]

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

tobiasgiese

Found a few nits. But all in all it looks good :)

tobiasgiese · 2021-08-20T06:03:31Z

controllers/openstackcluster_controller.go

@@ -196,24 +196,34 @@ func deleteBastion(log logr.Logger, osProviderClient *gophercloud.ProviderClient
 		return err
 	}

-	instance, err := computeService.GetInstanceByName(openStackCluster, fmt.Sprintf("%s-bastion", cluster.Name))
+	instanceStatus, err := computeService.GetInstanceByName(openStackCluster, fmt.Sprintf("%s-bastion", cluster.Name))


+1 for rename it to GetInstanceStatusByName

tobiasgiese · 2021-08-20T06:05:51Z

controllers/openstackcluster_controller.go

+	instanceNS, err := instanceStatus.NetworkStatus()
+	if err != nil {
+		return err
+	}


You should move this block inside the instanceStatus != nil condition, as it's only used there

This would also have been a bug!

nil pointer incoming :)

tobiasgiese · 2021-08-20T06:06:22Z

controllers/openstackcluster_controller.go

+		floatingIP := instanceNS.FloatingIP()
+		if err != nil {


You don't define err here, you can remove the err handling as no error will be returned

Whoops. FloatingIP() returned an error in an earlier version and I missed this!

tobiasgiese · 2021-08-20T06:11:03Z

controllers/openstackmachine_controller.go

+	ns, err := instance.NetworkStatus()
+	if err != nil {
+		handleUpdateMachineError(logger, openStackMachine, errors.Errorf("error getting network status for OpenStack instance %s with ID %s: %v", instance.Name(), instance.ID(), err))
+		return ctrl.Result{}, nil


Do you need the current status here? If yes, you should call GetInstanceByName (or GetInstanceStatusByName) again.

Also instance should be called instanceStatus :)

I don't think we want to fetch again here. Were you thinking of some specific reason we might need to? We'd potentially race with the prior delete, so it might even be a bug to re-fetch.

Why do we delete the instance and then check the status of the instance? Shouldn't we first check the status (i.e., to get the fips) and if that fails, we don't want to delete the instance? Otherwise we could have orphaned fips, right?

We're creating a InstanceNetworkStatus here so we can get the FloatingIP. This doesn't go back to OpenStack; it's just doing some extra work on the previously returned server object. The only reason it can fail is json parsing. All we need to know is which FloatingIP to delete.

The previous code also had this failure mode, but it was in the return of DeleteInstance()->GetInstance()->serverToInstance()->GetIPFromInstance(). Moving that call up to the 'top level' in the controller is the primary purpose of this refactor, because it means we can pass additional arguments to NetworkStatus() so we can have more context. I don't think it currently matters for FloatingIP(), for for IP() it means we can pass OpenStackCluster as an argument here and use the additional context to determine the correct 'primary' IP from those returned by OpenStack.

Incidentally, for robustness we should probably reverse the order of these deletions. The reason is that if deletion of the server succeeds but the deletion of the floating ip fails, the next reconcile will short-cut because there's no server and we'll leak the floating ip.

This patch is just a refactor, though, so I don't want to fix that here.

That's exactly the point I was worried about. We should first delete the floating IPs and the instance right after the successful deletion.
But okay for me to fix that after this refactor PR.

Further, we could maybe improve the error message for the failed json parsing. error getting network status for OpenStack instance is a bit misleading, tbh.

And starting errors with error is redundant. But we have to fix that in the complete repo anyway

mdbooth

Thanks for a thorough review, @tobiasgiese! I'm not in a hurry to land this as I still haven't written the fix for the actual multi-network bug, so I'd prefer to address these before merging.

With 2 people thinking I should rename the GetInstance functions I'll do that, too.

mdbooth · 2021-08-20T08:14:25Z

controllers/openstackcluster_controller.go

+		floatingIP := instanceNS.FloatingIP()
+		if err != nil {


Whoops. FloatingIP() returned an error in an earlier version and I missed this!

mdbooth · 2021-08-20T08:20:41Z

controllers/openstackmachine_controller.go

+	ns, err := instance.NetworkStatus()
+	if err != nil {
+		handleUpdateMachineError(logger, openStackMachine, errors.Errorf("error getting network status for OpenStack instance %s with ID %s: %v", instance.Name(), instance.ID(), err))
+		return ctrl.Result{}, nil


I don't think we want to fetch again here. Were you thinking of some specific reason we might need to? We'd potentially race with the prior delete, so it might even be a bug to re-fetch.

tobiasgiese · 2021-08-20T09:37:03Z

/lgtm

tobiasgiese · 2021-08-20T10:02:01Z

/lgtm cancel

waiting for the findings to be updated

This is predominantly a non-functional change. It contains some minor changes to log messages, and omits a check for instance name being empty prior to delete in OpenStackMachine controller because this check is no longer relevant when we delete by ID. Apart from that it contains no (deliberate) functional changes. Currently we use infrav1.Instance as our internal representation of an OpenStack instance. There are several problems with this: * infrav1.Instance is part of the API, so is hard to change for internal use * infrav1.Instance is used to represent both the 'spec' and 'status' of an instance * When used as a spec for the Bastion host, it allows fields to be set which will be ignored * When used as the status of an instance fetched from OpenStack not all fields are populated, leading to potential accidental usage errors This change creates several new types representing different types of instance data purely for internal use. This has the following advantages: * As they are not API types they can be updated easily * When used in a function signature they make it clear what data is being used * The type system will prevent accidental use of uninitialised data The new types are: * InstanceSpec * InstanceIdentifier * InstanceStatus * InstanceNetworkStatus They are defined and described in `instance_types.go`. The primary driver for this work is to eventually fix the documented errors in InstanceNetworkStatus in IP() and FloatingIP().

mdbooth · 2021-08-20T10:06:17Z

That change is intended to address all review comments. Please check!

tobiasgiese · 2021-08-20T10:08:56Z

/lgtm
👍🏻

jichenjc · 2021-08-20T11:12:40Z

/hold cancel

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 16, 2021

k8s-ci-robot requested review from jichenjc and prankul88 August 16, 2021 15:10

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 16, 2021

mdbooth mentioned this pull request Aug 16, 2021

Instance.IP is set randomly when a server has multiple networks #926

Closed

jichenjc reviewed Aug 17, 2021

View reviewed changes

k8s-ci-robot assigned hidekazuna Aug 19, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2021

mdbooth force-pushed the instance_status branch from 4f39d29 to 3d794dc Compare August 19, 2021 10:50

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2021

tobiasgiese reviewed Aug 20, 2021

View reviewed changes

mdbooth commented Aug 20, 2021

View reviewed changes

k8s-ci-robot assigned tobiasgiese Aug 20, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2021

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2021

mdbooth force-pushed the instance_status branch from 3d794dc to 6ef256a Compare August 20, 2021 10:05

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2021

k8s-ci-robot merged commit ed6893b into kubernetes-sigs:master Aug 20, 2021

mdbooth deleted the instance_status branch November 5, 2021 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Refactor: Don't use infrav1.Instance internally #971

✨ Refactor: Don't use infrav1.Instance internally #971

mdbooth commented Aug 16, 2021 •

edited

Loading

jichenjc Aug 17, 2021

mdbooth Aug 17, 2021

tobiasgiese Aug 20, 2021

jichenjc Aug 17, 2021

jichenjc Aug 17, 2021

mdbooth Aug 17, 2021

hidekazuna commented Aug 19, 2021

mdbooth commented Aug 19, 2021

jichenjc commented Aug 20, 2021

k8s-ci-robot commented Aug 20, 2021

tobiasgiese left a comment •

edited

Loading

tobiasgiese Aug 20, 2021

tobiasgiese Aug 20, 2021

mdbooth Aug 20, 2021

tobiasgiese Aug 20, 2021

tobiasgiese Aug 20, 2021

mdbooth Aug 20, 2021

tobiasgiese Aug 20, 2021

tobiasgiese Aug 20, 2021

mdbooth Aug 20, 2021

tobiasgiese Aug 20, 2021 •

edited

Loading

mdbooth Aug 20, 2021

mdbooth Aug 20, 2021

tobiasgiese Aug 20, 2021

tobiasgiese Aug 20, 2021 •

edited

Loading

mdbooth left a comment

mdbooth Aug 20, 2021

mdbooth Aug 20, 2021

tobiasgiese commented Aug 20, 2021

tobiasgiese commented Aug 20, 2021

mdbooth commented Aug 20, 2021

tobiasgiese commented Aug 20, 2021

jichenjc commented Aug 20, 2021

✨ Refactor: Don't use infrav1.Instance internally #971

✨ Refactor: Don't use infrav1.Instance internally #971

Conversation

mdbooth commented Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidekazuna commented Aug 19, 2021

mdbooth commented Aug 19, 2021

jichenjc commented Aug 20, 2021

k8s-ci-robot commented Aug 20, 2021

tobiasgiese left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasgiese Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasgiese Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

mdbooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasgiese commented Aug 20, 2021

tobiasgiese commented Aug 20, 2021

mdbooth commented Aug 20, 2021

tobiasgiese commented Aug 20, 2021

jichenjc commented Aug 20, 2021

mdbooth commented Aug 16, 2021 •

edited

Loading

tobiasgiese left a comment •

edited

Loading

tobiasgiese Aug 20, 2021 •

edited

Loading

tobiasgiese Aug 20, 2021 •

edited

Loading