Mapo nets to ports #13

iamemilio · 2021-12-14T23:13:24Z

Converts all nets to ports and wires in allowed address pairs fix

openshift-ci · 2021-12-14T23:13:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamemilio

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~~ [iamemilio]

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

mandre · 2021-12-15T12:35:49Z

/test e2e-openstack

mandre · 2021-12-15T14:48:23Z

Worker nodes don't come up.

From MAPO logs:

2021-12-15T13:44:49.627165518Z E1215 13:44:49.627097       1 actuator.go:416] Machine error 09nspy7c-39327-z6jqq-worker-0-b4cbk: error creating Openstack instance: network filter for port  returns more than one result
2021-12-15T13:44:49.627246661Z W1215 13:44:49.627230       1 controller.go:367] 09nspy7c-39327-z6jqq-worker-0-b4cbk: failed to create machine: error creating Openstack instance: network filter for port  returns more than one result
2021-12-15T13:44:49.627449566Z E1215 13:44:49.627419       1 controller.go:317] controller/machine_controller "msg"="Reconciler error" "error"="error creating Openstack instance: network filter for port  returns more than one result" "name"="09nspy7c-39327-z6jqq-worker-0-b4cbk" "namespace"="openshift-machine-api"

iamemilio · 2021-12-15T18:56:23Z

/hold needs a bugfix

This code will convert all networks in the machine spec to capo ports, which will help us transition to a port only api in the future. This will disable some of the super user behavior, but that is for the better in the long run.

iamemilio · 2021-12-16T02:16:49Z

/retest

openshift-ci · 2021-12-16T06:13:35Z

@iamemilio: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-openstack	`8a84a8b`	link	false	`/test e2e-openstack`

Full PR test history. Your PR dashboard.

Details

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.

mandre · 2021-12-16T08:00:22Z

The "[sig-installer][Feature:openstack] The OpenStack platform on instance creation should follow machineset specs [Suite:openshift/conformance/parallel]" test failed.

That's likely because the metadata.annotations.openstack-resourceId annotation is missing from the machines in the machineset.

https://github.com/openshift/openstack-test/blob/13bb9fd/test/extended/openstack/servers.go#L71

mandre · 2021-12-16T09:53:04Z

pkg/apis/openstackproviderconfig/v1alpha1/convert.go

+	portSecurity := net.PortSecurity
+	if net.PortSecurity != nil {
+		ps := *portSecurity
+		ps = !ps
+		portSecurity = &ps
+	}


It's hard to follow what you're doing here. Perhaps this makes it a bit easier to read?

Suggested change

portSecurity := net.PortSecurity

if net.PortSecurity != nil {

ps := *portSecurity

ps = !ps

portSecurity = &ps

}

var disablePortSecurity NetworkParam.PortSecurity

if net.PortSecurity != nil {

ps := *portSecurity

ps = !ps

disablePortSecurity = &ps

}

Then you'd also need to modify line 198 with disablePortSecurity instead of portSecurity: https://github.com/openshift/machine-api-provider-openstack/pull/13/files#diff-0fca389902b53f54c2c5a39550587a96d9003a5713a8313db3d571ec72422993R198

OK let’s start a contest!

this is my take at making things a little bit more readable:

var disablePortSecurity *bool if portSecurity != nil { disablePortSecurity = new(bool) *disablePortSecurity = !*portSecurity }

https://go.dev/play/p/EPPYFDA1LQc

As far as I can tell, your code wants the new *bool to be nil if portsec is nil, or the opposite of portsec if it’s not nil.

if I wasn’t on a phone, I would dig and try to understand if you really need your new value to be a pointer. How many different behaviours can descend from it? If it’s just « disable » or « don’t disable », then you don’t need a pointer

There are 3 behaviors: nil: use openstack default, &true: explicitly true, &false: explicitly false

mandre · 2021-12-16T09:55:24Z

pkg/apis/openstackproviderconfig/v1alpha1/convert.go

+			// Fetch the UUID of the network subnet is attached to or the conversion will fail
+			netID, err := subnet.Filter.getNetworkID(networkService)
+			if err != nil {
+				return []capov1.PortOpts{}, nil


Didn't you mean to return the error?

Suggested change

return []capov1.PortOpts{}, nil

return []capov1.PortOpts{}, err

iamemilio · 2021-12-16T15:15:56Z

The "[sig-installer][Feature:openstack] The OpenStack platform on instance creation should follow machineset specs [Suite:openshift/conformance/parallel]" test failed.

That's likely because the metadata.annotations.openstack-resourceId annotation is missing from the machines in the machineset.

https://github.com/openshift/openstack-test/blob/13bb9fd/test/extended/openstack/servers.go#L71

What is the meaning of this annotation?

iamemilio · 2021-12-16T16:14:21Z

I fixed the comments you left. This patch wont make the tests green, but it looks like they did install all the way, which is important, and is what we were looking for

mandre · 2021-12-16T16:15:29Z

I fixed the comments you left. This patch wont make the tests green, but it looks like they did install all the way, which is important, and is what we were looking for

We're tracking the remaining issue with https://bugzilla.redhat.com/show_bug.cgi?id=2033382.

iamemilio · 2021-12-16T16:26:22Z

/hold cancel

mandre

/lgtm
/hold for CI

iamemilio · 2021-12-17T15:42:11Z

/hold cancel

bump capo version to main

71acf23

openshift-ci bot requested review from EmilienM and pierreprinetti December 14, 2021 23:13

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2021

mandre mentioned this pull request Dec 15, 2021

Mapo build prep openshift/release#24617

Merged

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2021

Emilio Garcia added 2 commits December 15, 2021 14:37

Convert Networks to Ports

8681951

This code will convert all networks in the machine spec to capo ports, which will help us transition to a port only api in the future. This will disable some of the super user behavior, but that is for the better in the long run.

support for independent subnets

8a84a8b

iamemilio force-pushed the mapo-nets-to-ports branch from d1474a3 to 8a84a8b Compare December 15, 2021 20:42

mandre reviewed Dec 16, 2021

View reviewed changes

clarification of code

d919393

mandre closed this Dec 16, 2021

mandre reopened this Dec 16, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2021

mandre reviewed Dec 16, 2021

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2021

openshift-ci bot assigned mandre Dec 16, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 17, 2021

openshift-merge-robot merged commit 11d415e into openshift:master Dec 17, 2021

pierreprinetti deleted the mapo-nets-to-ports branch December 17, 2021 21:02

	return []capov1.PortOpts{}, nil
	return []capov1.PortOpts{}, err

Mapo nets to ports #13

Mapo nets to ports #13

Uh oh!

Conversation

iamemilio commented Dec 14, 2021

Uh oh!

openshift-ci bot commented Dec 14, 2021

Uh oh!

mandre commented Dec 15, 2021

Uh oh!

mandre commented Dec 15, 2021

Uh oh!

iamemilio commented Dec 15, 2021

Uh oh!

iamemilio commented Dec 16, 2021

Uh oh!

openshift-ci bot commented Dec 16, 2021

Uh oh!

mandre commented Dec 16, 2021

Uh oh!

mandre Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

pierreprinetti Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

pierreprinetti Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

iamemilio Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

mandre Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

iamemilio Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

iamemilio commented Dec 16, 2021

Uh oh!

iamemilio commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mandre commented Dec 16, 2021

Uh oh!

iamemilio commented Dec 16, 2021

Uh oh!

mandre left a comment

Choose a reason for hiding this comment

Uh oh!

iamemilio commented Dec 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iamemilio commented Dec 16, 2021 •

edited

Loading