✨Return all NodeAddresses in OpenStackMachine.Status.Addresses #1004

mdbooth · 2021-09-22T13:28:12Z

This commit makes 2 closely related changes:

Add NetworkStatus.Addresses(), and return this in OpenStackMachine
Update IP() and FloatingIP() to return individual addresses by
network name

There are also a couple of incidental changes:

Don't bother with DisassociateFloatingIP before DeleteFloatingIP for
bastion host
Delete floating IPs before server during machine delete: avoids
orphaned floating IPs if floating IP delete fails.

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

netlify · 2021-09-22T13:28:20Z

✔️ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

🔨 Explore the source changes: aea62dd

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/615625a6ee1bca00079ce7ee

😎 Browse the preview: https://deploy-preview-1004--kubernetes-sigs-cluster-api-openstack.netlify.app

mdbooth · 2021-09-22T16:10:09Z

Multiple addresses: https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-openstack/1004/pull-cluster-api-provider-openstack-e2e-test/1440688542236807168/artifacts/clusters/bootstrap/resources/e2e-0vpq66/Machine/cluster-e2e-0vpq66-control-plane-28crg.yaml

jichenjc · 2021-09-24T00:20:10Z

controllers/openstackcluster_controller.go

-			if err = networkingService.DeleteFloatingIP(openStackCluster, floatingIP); err != nil {
-				handleUpdateOSCError(openStackCluster, errors.Errorf("failed to delete floating IP: %v", err))
-				return errors.Errorf("failed to delete floating IP: %v", err)
+		for _, address := range addresses {


just curious, what's the reason behind the removal of disasociate floating ip for bastion only?

Consistency. We've never done it for controllers:

cluster-api-provider-openstack/controllers/openstackmachine_controller.go

Lines 246 to 251 in cf5e5cb

if !openStackCluster.Spec.ManagedAPIServerLoadBalancer && util.IsControlPlaneMachine(machine) && openStackCluster.Spec.APIServerFloatingIP == "" && floatingIP != "" {

if err = networkingService.DeleteFloatingIP(openStackCluster, floatingIP); err != nil {

handleUpdateMachineError(logger, openStackMachine, errors.Errorf("error deleting Openstack floating IP: %v", err))

return ctrl.Result{}, nil

}

}

Also it's redundant, because the floating IP will be disassociated automatically on deletion.

ok, thanks for the confirmation
I don't know the disassociate also happen on deletion.. thanks:)

jichenjc · 2021-09-24T00:21:00Z

controllers/openstackmachine_controller.go

 	}

 	if instanceStatus == nil {
 		logger.Info("Skipped deleting machine that is already deleted")


maybe add instance ID here?

I added this, but then removed it again because after looking at how we create the log object I realised it's redundant. We don't have instance ID here because the machine has been deleted. I had added machine name, but that's already included by the logger:

cluster-api-provider-openstack/controllers/openstackmachine_controller.go

Line 88 in cf5e5cb

log = log.WithValues("openStackMachine", openStackMachine.Name)

jichenjc · 2021-09-24T00:24:40Z

pkg/cloud/services/compute/instance_types.go

+			case "fixed":
+				addressType = corev1.NodeInternalIP
+			default:
+				// Ignore unknown address types


maybe add some log as reference? though should not reach here..

I thought about this, and I'm not sure.

The most likely reason that we would get an unknown address type here is that OpenStack introduced a new address type which we don't use, yet. This would be a new feature, and logging something here because of a new feature we don't support yet would be very noisy. I guess we could log this and IPv6 addresses at debug level just in case?

Interestingly, I was speaking to a Nova developer about addresses yesterday, and his impression is that addresses is more likely to go away entirely rather than gain new features because it's viewed as a proxy API. The replacement would be to directly query neutron for ports associated with the device id. The only reason I didn't make that change here is that as long as Nova is caching NetworkInfo, getting it from Nova requires 1 less API call.

TL;DR I might add this at debug.

I've added this.

jichenjc · 2021-09-24T00:28:58Z

pkg/cloud/services/compute/instance_types.go

+
+// FloatingIP returns the first listed floating ip of an instance for the given
+// network name.
+func (ns *InstanceNetworkStatus) FloatingIP(networkName string) string {


I guess I lost some background
usually ,multiple network is reasonable, so we have multiple NICs from various network
but multiple NIC from one network is not so familiar to me, anyway, seems no harm if we
return the first ip

We're actually only using this method in 1 place: APIInstance() above. And the only thing that uses APIInstance() is the bastion host. I guess we're not likely to put a second floating IP on the bastion host, so this is really just something to make the behaviour defined if we ever did.

jichenjc · 2021-09-24T00:30:33Z

/lgtm

overall looks great, just very some minor questions.. thanks~

macaptain

Looks great! Unit tests and e2e tests, we are spoiled.

macaptain · 2021-09-29T17:54:07Z

pkg/cloud/services/compute/instance_types.go

+	networks := make([]string, 0, len(ns.addresses))
+	for network := range ns.addresses {
+		networks = append(networks, network)
+	}


Would it be more idiomatic to do a copy here?

I read your comment, looked at my code, and agreed with you.

I was trying to work out why I wrote it this way when I realised the code is actually correct, because the range iterates over only the keys of the map, not the elements.

I think that's a fairly clear indication this code needs another comment 😂 I'll add one.

Added a comment.

macaptain · 2021-09-29T18:04:39Z

test/e2e/suites/e2e/e2e_test.go

+			Expect(len(workerMachines)).To(Equal(1))
+			Expect(len(controlPlaneMachines)).To(Equal(3))


I see that this creates a four node cluster with the machines having ports on two different networks. Did you also want to test your functions? I think you should be able to see e.g. controlPlaneMachines[0].Status.Addresses, if the controller is working, this should show multiple addresses, since they get assigned in reconciliation:

cluster-api-provider-openstack/controllers/openstackmachine_controller.go

Line 334 in fcbf207

openStackMachine.Status.Addresses = addresses

This was a lazy late addition, tbh. I'd previously written it as a standalone create/delete test which I expected to fail. However, it turns out that it eventually comes up because although IP selection was non-deterministic and failed when it picked the wrong one, the reconciler would just retry and eventually it picked the right one 🤷‍♂️

You are right: this test would be massively improved by testing the resulting addresses.

I've expanded the e2e test to explicitly check ports and addresses.

Incidentally, @EmilienM you may want to do something like this in #1008? I'm iterating over openstack ports per machine, which I think it what we were trying to do in that test.

This commit makes 2 closely related changes: * Add NetworkStatus.Addresses(), and return this in OpenStackMachine * Update IP() and FloatingIP() to return individual addresses by network name There are also a couple of incidental changes: * Don't bother with DisassociateFloatingIP before DeleteFloatingIP for bastion host * Delete floating IPs before server during machine delete: avoids orphaned floating IPs if floating IP delete fails.

hidekazuna · 2021-10-04T01:28:01Z

/approve

k8s-ci-robot · 2021-10-04T01:28:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hidekazuna, macaptain, mdbooth

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [hidekazuna]

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

macaptain · 2021-10-04T10:36:05Z

/lgtm

k8s-ci-robot · 2021-10-04T10:36:13Z

@macaptain: changing LGTM is restricted to collaborators

Details

In response to this:

/lgtm

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.

chrischdi

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 22, 2021

k8s-ci-robot requested review from hidekazuna and seanschneeweiss September 22, 2021 13:28

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

mdbooth force-pushed the alladdresses branch from a8bfa75 to 7440d78 Compare September 22, 2021 14:44

jichenjc reviewed Sep 24, 2021

View reviewed changes

k8s-ci-robot assigned jichenjc Sep 24, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 24, 2021

mdbooth force-pushed the alladdresses branch 2 times, most recently from 1c090b4 to fcbf207 Compare September 24, 2021 09:53

macaptain approved these changes Sep 29, 2021

View reviewed changes

mdbooth added 3 commits September 30, 2021 20:25

Make specName constant in e2e tests

062cad8

Remove a level of indirection from DumpOpenStackPorts

cc4b233

mdbooth force-pushed the alladdresses branch from fcbf207 to 6a5c44b Compare September 30, 2021 20:26

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 30, 2021

Add multi-network e2e test

aea62dd

mdbooth force-pushed the alladdresses branch from 6a5c44b to aea62dd Compare September 30, 2021 21:01

mdbooth mentioned this pull request Sep 30, 2021

🐛Fix Multi network support #984

Closed

macaptain approved these changes Oct 1, 2021

View reviewed changes

mdbooth requested a review from jichenjc October 1, 2021 10:21

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

chrischdi reviewed Oct 4, 2021

View reviewed changes

k8s-ci-robot assigned chrischdi Oct 4, 2021

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

k8s-ci-robot merged commit 1358f6e into kubernetes-sigs:master Oct 4, 2021

macaptain mentioned this pull request Oct 6, 2021

REQUEST: New membership for macaptain kubernetes/org#3030

Closed

7 tasks

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

mdbooth mentioned this pull request Dec 2, 2021

Bug 2022627: Fix nodelink and CSR approval when a machine has multiple addresses openshift/cluster-api-provider-openstack#210

Merged

mdbooth mentioned this pull request Dec 13, 2021

REQUEST: New membership for mdbooth kubernetes/org#3142

Closed

7 tasks

	if !openStackCluster.Spec.ManagedAPIServerLoadBalancer && util.IsControlPlaneMachine(machine) && openStackCluster.Spec.APIServerFloatingIP == "" && floatingIP != "" {
	if err = networkingService.DeleteFloatingIP(openStackCluster, floatingIP); err != nil {
	handleUpdateMachineError(logger, openStackMachine, errors.Errorf("error deleting Openstack floating IP: %v", err))
	return ctrl.Result{}, nil
	}
	}

		Expect(len(workerMachines)).To(Equal(1))
		Expect(len(controlPlaneMachines)).To(Equal(3))

✨Return all NodeAddresses in OpenStackMachine.Status.Addresses #1004

✨Return all NodeAddresses in OpenStackMachine.Status.Addresses #1004

Uh oh!

Conversation

mdbooth commented Sep 22, 2021

Uh oh!

netlify bot commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdbooth commented Sep 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jichenjc commented Sep 24, 2021

Uh oh!

macaptain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidekazuna commented Oct 4, 2021

Uh oh!

k8s-ci-robot commented Oct 4, 2021

Uh oh!

macaptain commented Oct 4, 2021

Uh oh!

k8s-ci-robot commented Oct 4, 2021

Uh oh!

chrischdi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

netlify bot commented Sep 22, 2021 •

edited

Loading