-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
⚠️ Remove InstanceID from machine spec #1961
Conversation
[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 |
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This needs a docs update, and also an update to the machine printcolumns. |
d29b359
to
0e6a0b3
Compare
Should be good to go now. |
0e6a0b3
to
071f41f
Compare
/test pull-cluster-api-provider-openstack-e2e-full-test |
071f41f
to
f5c1f4b
Compare
openStackCluster.Status.Bastion.Name = is.Name() | ||
openStackCluster.Status.Bastion.SSHKeyName = is.SSHKeyName() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with SSHKeyName being removed, the function at line 91 can also be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it's worth documenting that these fields are being removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both addressed. Also noticed that we weren't using AvailabilityZone()
any more and removed that, too.
6b9c47b
to
5275848
Compare
/test pull-cluster-api-provider-openstack-e2e-full-test |
5275848
to
084caa7
Compare
@@ -342,7 +385,16 @@ func Convert_v1beta1_OpenStackMachineSpec_To_v1alpha6_OpenStackMachineSpec(in *i | |||
|
|||
/* OpenStackMachineStatus */ | |||
|
|||
func restorev1alpha6MachineStatus(previous *OpenStackMachineStatus, dst *OpenStackMachineStatus) { | |||
if dst.InstanceState == nil || *dst.InstanceState == "" { | |||
dst.InstanceState = previous.InstanceState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the other restore functions skip the setting of previous into dst when the resource is empty/nil. Should we follow the same pattern here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been adding that incrementally. As this is a new function I should have added it here.
@@ -157,6 +158,10 @@ spec: | |||
name: "MyOpenStackFloatingIPPool" | |||
``` | |||
|
|||
#### Removal of instanceID from spec | |||
|
|||
The OpenStackMachine controller previously set `instanceID` in the machine spec, in addition to `providerID`. `providerID` remains unchanged as it is required by Cluster API, but `instanceID` is no longer set. If still required, it is now available in the status in `dependentResources`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dependentResources should be renamed to resources
@@ -164,9 +159,9 @@ type OpenStackMachineStatus struct { | |||
// +kubebuilder:resource:path=openstackmachines,scope=Namespaced,categories=cluster-api,shortName=osm | |||
// +kubebuilder:subresource:status | |||
// +kubebuilder:printcolumn:name="Cluster",type="string",JSONPath=".metadata.labels.cluster\\.x-k8s\\.io/cluster-name",description="Cluster to which this OpenStackMachine belongs" | |||
// +kubebuilder:printcolumn:name="InstanceState",type="string",JSONPath=".status.instanceState",description="OpenStack instance state" | |||
// +kubebuilder:printcolumn:name="ServerID",type="string",JSONPath=".status.dependentResources.server.id",description="OpenStack instance ID" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dependentResources should now be resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated and re-tested on a running cluster:
> kc get openstackmachine
NAME CLUSTER SERVERID INSTANCESTATE READY MACHINE AGE
test-foo-c4wdz test-foo a5500a2d-d1f8-4b8e-a7bb-2b94977eb893 BUILD test-foo-c4wdz 4m26s
@@ -164,9 +159,9 @@ type OpenStackMachineStatus struct { | |||
// +kubebuilder:resource:path=openstackmachines,scope=Namespaced,categories=cluster-api,shortName=osm | |||
// +kubebuilder:subresource:status | |||
// +kubebuilder:printcolumn:name="Cluster",type="string",JSONPath=".metadata.labels.cluster\\.x-k8s\\.io/cluster-name",description="Cluster to which this OpenStackMachine belongs" | |||
// +kubebuilder:printcolumn:name="InstanceState",type="string",JSONPath=".status.instanceState",description="OpenStack instance state" | |||
// +kubebuilder:printcolumn:name="ServerID",type="string",JSONPath=".status.dependentResources.server.id",description="OpenStack instance ID" | |||
// +kubebuilder:printcolumn:name="InstanceState",type="string",JSONPath=".status.dependentResources.server.state",description="OpenStack instance state" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
} | ||
|
||
func Convert_v1alpha7_OpenStackMachineStatus_To_v1beta1_OpenStackMachineStatus(in *OpenStackMachineStatus, out *infrav1.OpenStackMachineStatus, s apiconversion.Scope) error { | ||
// ReferencedResources have no equivalent in v1beta1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably want to replace ReferencedResources
for Resolved
/test pull-cluster-api-provider-openstack-e2e-full-test |
/lgtm |
New changes are detected. LGTM label has been removed. |
api/v1beta1/types.go
Outdated
@@ -358,14 +358,15 @@ type AddressPair struct { | |||
} | |||
|
|||
type BastionStatus struct { | |||
ID string `json:"id,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably put bastion ID and State back, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was the idea that State and ID goes into Server to mimic what's on OpenStackMachineStatus. No strong opinion from my side here.
|
||
// Server describes the OpenStack server created for the bastion | ||
// +optional | ||
Server *ServerStatus `json:"server,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this?
This is a status field and should not be included in the spec. ProviderID is the same, but cannot be removed as it is required by Cluster API.
Okay, I think I found the issue. |
@mdbooth: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
PR needs rebase. 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've decided to replace this with #1988, which has a smaller scope. |
This is a status field and should not be included in the spec. ProviderID is the same, but cannot be removed as it is required by Cluster API.
While we're changing it, we change the default printcolumns for OpenStackMachine to report server ID instead of provider ID. We don't need to report provider ID here because we already know everything is an openstack server. New output example:
/hold