Add Neutron's default route annotations #722
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: elvgarrui The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @elvgarrui. Thanks for your PR. I'm waiting for a openstack-k8s-operators member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/dcaa1d794753416fa1f9a5d8bd4a477f ❌ openstack-k8s-operators-content-provider FAILURE in 8m 22s |
|
@elvgarrui hi, just left you a comment on the neutron-operator CR. I think you just need to move the validation into the core NeutronAPISpecCore.Default() validation function which currently has nothing in it. There is actually a comment in the Neutron code that says 'only container image validations go here' for the NeutronAPISpec.Default() function. I'm not sure what else we can do other than have a comment to go along with these things. |
|
One more comment: We haven't actually switched to the core structs in OpenStackControlplane yet. But soon will once the OpenStackVersion stuff lands. |
|
It would be nice to include a functional test here confirming that the default is set for neutron as expected. |
| openstackcontrolplanelog.Info("Neutron Route Override initialized") | ||
|
|
||
| } | ||
| r.Spec.Neutron.APIOverride.Route.AddAnnotation(r.Spec.Neutron.Template.GetDefaultRouteAnnotations()) |
There was a problem hiding this comment.
I think this is the important part and you don't need a new field in neutron struct at all.
| @@ -381,6 +382,14 @@ func (r *OpenStackControlPlane) DefaultServices() { | |||
|
|
|||
| // Neutron | |||
| r.Spec.Neutron.Template.Default() | |||
| // WIP: Error when trying to trigger webhook by modifying the CR. | |||
| if r.Spec.Neutron.APIOverride.Route == nil { | |||
There was a problem hiding this comment.
This logic may be helpful for other services, consider introducing a helper function that would receive an override object and default annotations and that would initialize the override route accordingly.
There was a problem hiding this comment.
This should be defined in lib-common, right? Should I propose it there and then change it here on a follow-up patch? Or is there a better way?
There was a problem hiding this comment.
You could have it here for now since the code that will use it will be here too, no? If it implies lib-common, definitely don't go there not to slow down merging it.
(Other projects should probably take it from there and iterate over all service CRs to initialize defaults.)
There was a problem hiding this comment.
you are right, at first I thought of creating the function with a receiver, and that can only be done from the lib-common repository, but I can pass the * OverrideSpec as an argument.
| // WIP: Error when trying to trigger webhook by modifying the CR. | ||
| if r.Spec.Neutron.APIOverride.Route == nil { | ||
| r.Spec.Neutron.APIOverride.Route = &route.OverrideSpec{} | ||
| openstackcontrolplanelog.Info("Neutron Route Override initialized") |
elvgarrui
force-pushed
the
route_default_timeout
branch
from
2ff1821 to
8ddc851
Compare
|
pushed some of the requested changes and also rebased, but it is still missing the functional test |
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/a665d21a41f243e583df3da62ae18375 ❌ openstack-k8s-operators-content-provider FAILURE in 8m 09s |
elvgarrui
force-pushed
the
route_default_timeout
branch
from
8ddc851 to
722467e
Compare
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/f51f0adc131949d5971b7f57e03c74ef ❌ openstack-k8s-operators-content-provider FAILURE in 8m 02s |
| @@ -318,6 +319,13 @@ func (r *OpenStackControlPlane) Default() { | |||
| r.DefaultServices() | |||
| } | |||
|
|
|||
| // Helper function to initialize overrideSpec object. Could be moved to lib-common. | |||
| func initializeOverride(override **route.OverrideSpec) { | |||
There was a problem hiding this comment.
I thought it would also receive route annotations as arg, then it would
override.Route.AddAnnotations(annotations)...
otherwise this helper is very bare and probably is not very helpful
There was a problem hiding this comment.
Oooh I see... I have changed it now to include that. Let me know if it looks better. Functional test was also added.
|
looks fine but need a test, thanks |
elvgarrui
force-pushed
the
route_default_timeout
branch
from
722467e to
8b2a577
Compare
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/c9a4bb09236541d5bdbb8fe7e2aff526 ❌ openstack-k8s-operators-content-provider FAILURE in 8m 43s |
elvgarrui
force-pushed
the
route_default_timeout
branch
from
8b2a577 to
1c976b7
Compare
|
/ok-to-test |
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/bce6346a44804b7bad9d85282ef6da35 ❌ openstack-k8s-operators-content-provider FAILURE in 8m 24s |
|
recheck zuul one with Depends-On |
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/27f6cc34a4894b9ea4f92d28fdf5f3b5 ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 56m 15s |
|
/retest-required |
|
recheck |
|
I think this needs a |
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/66a972c5d0f44c3b9a8d9d09c667bb28 ❌ openstack-k8s-operators-content-provider FAILURE in 8m 02s |
Hi! I've tried running that command, and it creates no changes on the go.mod or any other file. |
elvgarrui
force-pushed
the
route_default_timeout
branch
from
1c976b7 to
0c0b3e3
Compare
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/3873c3bc3cce4d8782f3268697c7d194 ❌ openstack-k8s-operators-content-provider FAILURE in 8m 07s |
@elvgarrui My bad, the command needs |
elvgarrui
force-pushed
the
route_default_timeout
branch
from
0c0b3e3 to
840aebe
Compare
|
@abays Oooh that makes sense, thanks :) |
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/ef6518f661d747858d36e08787e71a08 ❌ openstack-k8s-operators-content-provider FAILURE in 6m 22s |
|
@elvgarrui Bah, I'm so sorry. I was wrong again. I was thinking of containers, not Golang. The proper command is: |
elvgarrui
force-pushed
the
route_default_timeout
branch
from
840aebe to
e3a9c72
Compare
|
@abays Nevermind! I'm really grateful for the quick help :) Re-pushed again with tags from main |
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/c7648470749d41619f621c0cc077d38a ❌ openstack-k8s-operators-content-provider FAILURE in 6m 17s |
When the webhook calls for Neutron defaults, it should also write the route override annotation on Neutron's CR section. Signed-off-by: Elvira García <[email protected]>
elvgarrui
force-pushed
the
route_default_timeout
branch
from
e3a9c72 to
e655525
Compare
|
/lgtm neutron-operator updated as part of #716 |
|
/lgtm |
When the webhook calls for Neutron defaults, it should also write the route override annotation on Neutron's CR section so that the route gets modified.
Currently it is failing when I try to modify the openstack controlplane CR, and I don't get to see anything under neutron apiOverride on the yaml. But I do see the Annotation applied on the OverrideSpec object. I assume I'm doing something wrong here, any help appreciated!
(Goes together with openstack-k8s-operators/neutron-operator#324)
Depends-On: openstack-k8s-operators/neutron-operator#324