✨ Re-work ports management #1788

EmilienM · 2023-12-11T20:23:07Z

What this PR does / why we need it:

This moves the ports management for both the OpenStackMachines and Bastion to be done outside of instances. This will make CAPO more resilient.

List of changes:

API: Add OpenStackMachineStatus.DependentResources and OpenStackCluster.Status.Bastion.DependentResources which will contain resolved dependent resources that were created by the machine.
API: Add PortsStatus to DependentResources which for now only contains Neutron Ports IDs that have been adopted (meaning they actually exist) in a cluster.
OpenStackCluster: reconcileDelete: we remove the security groups later since we need to remove the ports first.
deleteBastion: remove bastion ports after the bastion instance was removed.
When reconciling a Machine or a Bastion, resolve the dependent resources (only ports for now), which will 1) find out what are the expected ports for a cluster and 2) adopt the ports that currently exist and put them in Status. This will help later to know which ones to create or delete.

Note: we didn't add Conditions yet, but this might be something we do in the future.

squashed commits
if necessary:
- includes documentation
- adds unit tests

/hold

netlify · 2023-12-11T20:23:12Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`7ec4c14`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65dccc94f20c880008624f2d
😎 Deploy Preview	https://deploy-preview-1788--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

EmilienM · 2023-12-14T17:49:37Z

/cc jichenjc

EmilienM · 2023-12-14T17:49:55Z

/cc lentzi90

EmilienM · 2024-02-20T15:10:03Z

/test pull-cluster-api-provider-openstack-e2e-full-test

EmilienM · 2024-02-21T15:04:23Z

/test pull-cluster-api-provider-openstack-e2e-full-test

EmilienM · 2024-02-21T15:05:13Z

/test pull-cluster-api-provider-openstack-test pull-cluster-api-provider-openstack-build

EmilienM · 2024-02-21T17:06:12Z

/retest

EmilienM · 2024-02-22T14:30:39Z

/test pull-cluster-api-provider-openstack-e2e-full-test

lentzi90

Looking really great!

pkg/cloud/services/compute/referenced_resources_test.go

pkg/cloud/services/networking/port_test.go

lentzi90 · 2024-02-26T10:57:41Z

controllers/openstackmachine_controller.go

@@ -446,7 +489,38 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 	return ctrl.Result{}, nil
 }

-func (r *OpenStackMachineReconciler) getOrCreate(logger logr.Logger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, computeService *compute.Service, userData string) (*compute.InstanceStatus, error) {
+func getOrCreateMachinePorts(scope scope.Scope, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, networkingService *networking.Service, clusterName string) error {
+	scope.Logger().Info("Reconciling ports for machine", "machine", machine.Name)


Is this important enough to be info level? I don't have a strong opinion, just a thought

I think it should be enough with changes, meaning if a port is created/deleted or updated?

Info is already used elsewhere in the controller, for example when handling machines or also load balancers, etc. I think this is fine for now.

Yeah I know it is used for "Reconciling machine" for example. It just feels to me like "ports for machine" would be already implied by "Reconciling machine", as in this is a lower level. Load balancers on the other hand is not part of that machine so it makes sense that they get a separate log message.

If we end up creating or changing a port during this reconciliation I would definitely expect an Info level message for that, but this is just information that we made it this far in the loop.

Good catch. I'll fix that right on a next iteration.

EmilienM · 2024-02-26T17:38:39Z

/test pull-cluster-api-provider-openstack-e2e-full-test

mdbooth

This is fantastic, great work.

Returning changed from ResolveReferencedMachineResources is a bit jarring, but I understand where you're coming from.

Of course this struct should not change after creation, which is its whole purpose. This commit from my API cleanup series turns it into a pointer. I think that change would allow us to more easily set this struct atomically. i.e. It's either entirely set, or not present.

I think this is good, though, and I've approved it. If you wanted to incorporate this change, go for it. If not I'll likely do it as part of my API cleanup series instead.

I'm also going to wait for other eyes to sign off on this change too as it's pretty big.

k8s-ci-robot · 2024-02-27T10:33:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EmilienM, 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~~ [mdbooth]

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

lentzi90

My comments have been addressed, so
/lgtm
Hold is still there if you want to do something about that log level or @mdbooth 's comment

EmilienM · 2024-02-27T13:27:38Z

I'm taking a look at Matt & Lennart's comments today.

EmilienM · 2024-02-27T14:11:49Z

Thanks you all for the reviews. I'll address the logging and the API cleanup right after this one merges (if possible today), if you're fine with it.

EmilienM · 2024-02-27T14:31:20Z

/hold cancel

EmilienM · 2024-02-27T14:36:05Z

WOW WOW WOW

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 11, 2023

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 11, 2023

k8s-ci-robot requested review from mdbooth and seanschneeweiss December 11, 2023 20:23

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 11, 2023

EmilienM force-pushed the ports branch from b9b6cd1 to 3139895 Compare December 11, 2023 20:34

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 Dec 11, 2023

EmilienM mentioned this pull request Dec 11, 2023

✨ Reconcile SecurityGroups <-> Instances separately #1774

Closed

6 tasks

EmilienM force-pushed the ports branch from 3139895 to d9d0d6f Compare December 13, 2023 14:20

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

EmilienM force-pushed the ports branch 3 times, most recently from db13cf1 to 40b2256 Compare December 13, 2023 14:50

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 Dec 13, 2023

EmilienM force-pushed the ports branch 5 times, most recently from dbaca3c to 625ec6f Compare December 14, 2023 17:43

EmilienM changed the title ~~WIP - move ports out of instance~~ ✨ Re-work ports management Dec 14, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 14, 2023

k8s-ci-robot requested a review from jichenjc December 14, 2023 17:49

k8s-ci-robot requested a review from lentzi90 December 14, 2023 17:49

EmilienM force-pushed the ports branch from 8ac779d to cc3f782 Compare February 21, 2024 15:04

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2024

EmilienM force-pushed the ports branch from cc3f782 to d6d1839 Compare February 22, 2024 14:30

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2024

EmilienM requested review from mdbooth and jichenjc February 22, 2024 20:10

lentzi90 reviewed Feb 26, 2024

View reviewed changes

EmilienM force-pushed the ports branch from d6d1839 to fe4d725 Compare February 26, 2024 13:35

Move ports management outside of instances

7ec4c14

EmilienM force-pushed the ports branch from fe4d725 to 7ec4c14 Compare February 26, 2024 17:38

mdbooth approved these changes Feb 27, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2024

lentzi90 reviewed Feb 27, 2024

View reviewed changes

k8s-ci-robot assigned lentzi90 Feb 27, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2024

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

k8s-ci-robot merged commit fd37971 into kubernetes-sigs:main Feb 27, 2024
10 checks passed

EmilienM deleted the ports branch February 27, 2024 14:36

EmilienM mentioned this pull request Feb 27, 2024

Ports follow-up #1910

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Re-work ports management #1788

✨ Re-work ports management #1788

EmilienM commented Dec 11, 2023 •

edited

Loading

netlify bot commented Dec 11, 2023 •

edited

Loading

EmilienM commented Dec 14, 2023

EmilienM commented Dec 14, 2023

EmilienM commented Feb 20, 2024

EmilienM commented Feb 21, 2024

EmilienM commented Feb 21, 2024

EmilienM commented Feb 21, 2024

EmilienM commented Feb 22, 2024

lentzi90 left a comment

lentzi90 Feb 26, 2024

huxcrux Feb 26, 2024

EmilienM Feb 26, 2024

lentzi90 Feb 27, 2024

EmilienM Feb 27, 2024

EmilienM commented Feb 26, 2024

mdbooth left a comment

k8s-ci-robot commented Feb 27, 2024

lentzi90 left a comment

EmilienM commented Feb 27, 2024

EmilienM commented Feb 27, 2024

EmilienM commented Feb 27, 2024

EmilienM commented Feb 27, 2024

✨ Re-work ports management #1788

✨ Re-work ports management #1788

Conversation

EmilienM commented Dec 11, 2023 • edited Loading

netlify bot commented Dec 11, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

EmilienM commented Dec 14, 2023

EmilienM commented Dec 14, 2023

EmilienM commented Feb 20, 2024

EmilienM commented Feb 21, 2024

EmilienM commented Feb 21, 2024

EmilienM commented Feb 21, 2024

EmilienM commented Feb 22, 2024

lentzi90 left a comment

Choose a reason for hiding this comment

lentzi90 Feb 26, 2024

Choose a reason for hiding this comment

huxcrux Feb 26, 2024

Choose a reason for hiding this comment

EmilienM Feb 26, 2024

Choose a reason for hiding this comment

lentzi90 Feb 27, 2024

Choose a reason for hiding this comment

EmilienM Feb 27, 2024

Choose a reason for hiding this comment

EmilienM commented Feb 26, 2024

mdbooth left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 27, 2024

lentzi90 left a comment

Choose a reason for hiding this comment

EmilienM commented Feb 27, 2024

EmilienM commented Feb 27, 2024

EmilienM commented Feb 27, 2024

EmilienM commented Feb 27, 2024

EmilienM commented Dec 11, 2023 •

edited

Loading

netlify bot commented Dec 11, 2023 •

edited

Loading