Port: add propagationUplinkStatus field #641
Conversation
|
weird... the uplink_status_propagation extension was not added to the devstack deployment. I tested locally and it only works by setting Has anyone ever seen something like this? |
winiciusallan
force-pushed
the
port-propagate-uplink-status
branch
from
8cbef64 to
4d21547
Compare
|
Ok, here we go. I needed to add the neutron plugin on CI so that devstack can run this line and enable uplink-status-propagation accordingly. However, it looks like that Dalmatian release doesn't enable the ability to update About the job failing on Flamingo I really don't get the why. The E2E job has failed in cc. @mandre I'll wait for you feedback before making any additional change. |
api/v1alpha1/port_types.go
Outdated
| // the port. | ||
| // +optional | ||
| PropagateUplinkStatus *bool `json:"propagateUplinkStatus,omitempty"` | ||
| PropagateUplinkStatus bool `json:"propagateUplinkStatus,omitempty"` |
There was a problem hiding this comment.
I believe we want to keep the pointer, don't we? This signifies that neutron doesn't have enable the uplink-status-propagation extension.
There was a problem hiding this comment.
Yep, we do. I'll change it together with making the field immutable.
| // propagateUplinkStatus represents the uplink status propagation of | ||
| // the port. | ||
| // +optional | ||
| PropagateUplinkStatus *bool `json:"propagateUplinkStatus,omitempty"` |
There was a problem hiding this comment.
We've discussed this privately already, after you brought the issue to my attention: gophercloud implements the PropagateUplinkStatus in the response Port structure as a bool while it should really be a *bool with omitempty, and because of this we can't reliably know the status for PropagateUplinkStatus. I suggest that until the issue if fixed in gophercloud, we make that field immutable.
.github/workflows/e2e.yaml
Outdated
| enable_workaround_docker_io: 'false' | ||
| branch: ${{ matrix.openstack_version }} | ||
| enabled_services: "openstack-cli-server" | ||
| enabled_services: "openstack-cli-server,neutron-uplink-status-propagation" |
There was a problem hiding this comment.
Interesting, the dalmatian job still failed?
There was a problem hiding this comment.
Yep. I don't know the why, but there is two extensions - one for enable the field, and one for make it mutable. In dalmatian, the devstack configuration enable just the first one, so the jobs are failing on port-update test.
For reference, compare these two [1][2]
Since we're making this field immutable, this wouldn't be a problem for now.
[1] https://github.com/openstack/neutron/blob/master/devstack/lib/uplink_status_propagation
[2] https://github.com/openstack/neutron/blob/stable/2024.2/devstack/lib/uplink_status_propagation
There was a problem hiding this comment.
Gotcha, thanks for the explanation.
There was a problem hiding this comment.
So according to the commit in neutron, openstack/neutron@498be54 we'll need neutron-lib >= v3.16.0, which we don't have in Dalmatian. I believe we'll have to make the field immutable until we drop support for Dalmatian.
I re-ran this job and the error is gone. I was most likely a flake (bound to happen as we add more tests). |
We have a missing pointer on Gophercloud, this is causing a misbehavior. For workarouding purposes, was added a new struct to support a pointer for that field. Tests were also fixed.
github-actions
bot
added
semver:minor
2 participants
This PR introduces the propagationUplinkStatus
https://docs.openstack.org/api-ref/network/v2/index.html#uplink-status-propagation
Notes:
false, even if the doc says that the default value istrue. The reason is that in environments where this extension is not enabled, we can't update that field and it always returns false, so if we enforce it as true, it may raise an error. So if the user wants to enable it, they must explicitly define it.Partial #616