Update glanceapi to use service override #309

stuggi · 2023-09-07T07:50:17Z

Removes creation of routes.Those get done in the openstack-operator. Via service overrides the service can be customized. The service operator adds annotation to the service which needs to be exposed as a route.

Jira: OSP-26690

Depends-On: openstack-k8s-operators/lib-common#332
Depends-On: openstack-k8s-operators/keystone-operator#307

stuggi · 2023-09-07T07:50:23Z

/hold

openshift-ci · 2023-09-07T07:50:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stuggi

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

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

fmount · 2023-09-14T13:50:16Z

controllers/glanceapi_controller.go

+		})
+
+		// add Annotation to whether creating an ingress is required or not
+		if endpointType == service.EndpointPublic && svc.GetServiceType() == corev1.ServiceTypeClusterIP {


@stuggi I think I understand the main change here: we create the service at the service operator level, we annotate the service with AnnotationIngressCreateKey = "core.openstack.org/ingress_create" in case we need to build an ingress.
E.g. :

apiVersion: v1 kind: Service metadata: annotations: core.openstack.org/ingress_create: "true" core.openstack.org/ingress_name: glance

Does the openstack-operator watches for the Service resources and create the Routes for the services that are created with those annotations ?
In the Glance use case this would end up having a Route only for the GlanceAPIExternal, while the internal endpoint is still tied to metalLB ipaddresspool, right?

@stuggi I think I understand the main change here: we create the service at the service operator level, we annotate the service with AnnotationIngressCreateKey = "core.openstack.org/ingress_create" in case we need to build an ingress. E.g. :

apiVersion: v1 kind: Service metadata: annotations: core.openstack.org/ingress_create: "true" core.openstack.org/ingress_name: glance

Does the openstack-operator watches for the Service resources and create the Routes for the services that are created with those annotations ?

It does not watch the services, it watches the service CR it creates, e.g. glance. When glance updates its status because of condition update, or so, the openstack-operator reconciles and checks the services of glance. For those with the annotation it creates the route and passes in the endpointURL for the service. In a follow up for TLS it then passes the https url to be used for register in keystone. For TLS-E it then will then also create the cert ans passes in the secret name to be used by glance api.

In the Glance use case this would end up having a Route only for the GlanceAPIExternal, while the internal endpoint is still tied to metalLB ipaddresspool, right?

yes correct. the internal endpoint can be customized using the service override, but for now we don't expect to create a route for it.

Ack it works from me, I'm overall +1 with this change, I was trying to properly understand the interaction between the openstack-operator (that is supposed to create the Route) and the <Service>operator, that is supposed to build the service and register endpoints in keystone.

fmount · 2023-09-14T13:57:04Z

README.md

-        - "172.17.0.202"
-        sharedIP: true
-        sharedIPKey: ""
+    override:


Do we want to provide a sample file in config/samples? It can be a follow up though, so we can have a final working solution w/ tls

since we don't have a sample for the current one, I planned to only have a network-isolation sample in the openstack-operator as it is today. but we can also add a sample here

ack, we can provide a sample based on the basic openstackcontrolplane in a follow up, the description in the Readme is enough.

fmount · 2023-09-14T13:59:53Z

api/v1beta1/common.go

+// APIOverrideSpec to override the generated manifest of several child resources.
+type APIOverrideSpec struct {
+	// Override configuration for the Service created to serve traffic to the cluster.
+	Service *service.RoutedOverrideSpec `json:"service,omitempty"`


So this replace actually the way we used to config services w/ metallb and we're going to provide RouteOverrides under annotations form:

override: service: metadata: annotations: metallb.universe.tf/address-pool: internalapi metallb.universe.tf/allow-shared-ip: internalapi metallb.universe.tf/loadBalancerIPs: 172.17.0.90 spec: type: LoadBalancer

yes, via the overrides you pass in the annotation used for the LoadBalancer you use, metallb here (but could be also a different one. The benefit is that its native k8s service style and probably known by users if they already are familiar with k8s

abays · 2023-09-20T11:05:25Z

@stuggi: The following tests 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/glance-operator-build-deploy-tempest aa05d80 link true /test glance-operator-build-deploy-tempest
ci/prow/glance-operator-build-deploy-kuttl aa05d80 link true /test glance-operator-build-deploy-kuttl

Full PR test history. Your PR dashboard.

Hiccup

  - lastTransitionTime: "2023-09-20T10:21:52Z"
    message: 'OpenStackControlPlane OVN error occured Internal error occurred: failed
      calling webhook "movndbcluster.kb.io": failed to call webhook: Post "https://ovn-operator-controller-manager-service.openstack-operators.svc:443/mutate-ovn-openstack-org-v1beta1-ovndbcluster?timeout=10s":
      no endpoints available for service "ovn-operator-controller-manager-service"'
    reason: Error
    severity: Warning
    status: "False"

/test glance-operator-build-deploy-kuttl

stuggi · 2023-09-20T13:06:23Z

/test functional

stuggi · 2023-09-20T14:19:54Z

The issue we right now is that the we are building the new operator with the current version of the openstack-operator which has not the functionality to create the route for the new service operator version. With this the public endpoint will be a ClusterIP service and is not reachable by the tempest job which is running outside the OCP env.

I think we have two options:

update the current samples in the openstack-operator for network-isolation (if that is used by tempest) to also use a LoadBalancer type service for the public endpoint and see if that works for the tempest job
force merge with failed tempest job result until we merged the openstack-operator PR

stuggi · 2023-09-20T14:27:44Z

The issue we right now is that the we are building the new operator with the current version of the openstack-operator which has not the functionality to create the route for the new service operator version. With this the public endpoint will be a ClusterIP service and is not reachable by the tempest job which is running outside the OCP env.

I think we have two options:

update the current samples in the openstack-operator for network-isolation (if that is used by tempest) to also use a LoadBalancer type service for the public endpoint and see if that works for the tempest job

This would only work if the host tempest is running is using our DNS server where the service endpoint gets registered

force merge with failed tempest job result until we merged the openstack-operator PR

Removes creation of routes.Those get done in the openstack-operator. Via service overrides the service can be customized. The service operator adds annotation to the service which needs to be exposed as a route. Jira: OSP-26690 Depends-On: openstack-k8s-operators/lib-common#332

openshift-ci · 2023-09-21T07:37:20Z

@stuggi: 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/glance-operator-build-deploy-tempest	`82f0dc7`	link	true	`/test glance-operator-build-deploy-tempest`

Full PR test history. Your PR dashboard.

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.

fmount · 2023-09-21T08:11:57Z

The issue we right now is that the we are building the new operator with the current version of the openstack-operator which has not the functionality to create the route for the new service operator version. With this the public endpoint will be a ClusterIP service and is not reachable by the tempest job which is running outside the OCP env.

I think we have two options:
* update the current samples in the openstack-operator for network-isolation (if that is used by tempest) to also use a LoadBalancer type service for the public endpoint and see if that works for the tempest job

* force merge with failed tempest job result until we merged the openstack-operator PR

I agree we should force-merge these patches and avoid growing the patch series to get the Routes creation moved at the openstack-operator level.
I'm going to force-merge this and work with @stuggi to land the whole patch chain, otherwise any other patch will be stuck failing in CI.

openshift-ci bot requested review from abays and dprince September 7, 2023 07:50

openshift-ci bot added the do-not-merge/hold label Sep 7, 2023

openshift-ci bot added the approved label Sep 7, 2023

stuggi mentioned this pull request Sep 7, 2023

Create several services routes based on annotations on the svc endpoint openstack-k8s-operators/openstack-operator#457

Merged

stuggi force-pushed the handle_ingress_v3 branch 3 times, most recently from 809d292 to b1835ec Compare September 12, 2023 06:40

stuggi mentioned this pull request Sep 12, 2023

Update glanceapi to use service override #285

Closed

stuggi requested a review from fmount September 12, 2023 15:26

fmount reviewed Sep 14, 2023

View reviewed changes

stuggi force-pushed the handle_ingress_v3 branch 4 times, most recently from 2656dbf to aa05d80 Compare September 20, 2023 09:24

stuggi force-pushed the handle_ingress_v3 branch from aa05d80 to 95ff975 Compare September 20, 2023 12:49

stuggi force-pushed the handle_ingress_v3 branch from 95ff975 to 82f0dc7 Compare September 21, 2023 06:47

fmount merged commit 8916408 into openstack-k8s-operators:main Sep 21, 2023
1 check passed

stuggi removed the do-not-merge/hold label Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update glanceapi to use service override #309

Update glanceapi to use service override #309

stuggi commented Sep 7, 2023 •

edited by openshift-ci bot

Loading

stuggi commented Sep 7, 2023

openshift-ci bot commented Sep 7, 2023

fmount Sep 14, 2023

stuggi Sep 14, 2023

fmount Sep 18, 2023

fmount Sep 14, 2023

stuggi Sep 14, 2023

fmount Sep 18, 2023

fmount Sep 14, 2023

stuggi Sep 14, 2023

abays commented Sep 20, 2023

stuggi commented Sep 20, 2023

stuggi commented Sep 20, 2023

stuggi commented Sep 20, 2023

openshift-ci bot commented Sep 21, 2023

fmount commented Sep 21, 2023

Update glanceapi to use service override #309

Update glanceapi to use service override #309

Conversation

stuggi commented Sep 7, 2023 • edited by openshift-ci bot Loading

stuggi commented Sep 7, 2023

openshift-ci bot commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abays commented Sep 20, 2023

stuggi commented Sep 20, 2023

stuggi commented Sep 20, 2023

stuggi commented Sep 20, 2023

openshift-ci bot commented Sep 21, 2023

fmount commented Sep 21, 2023

stuggi commented Sep 7, 2023 •

edited by openshift-ci bot

Loading