Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Return Kafka admin server URL in kas-fleet-manager Kafka Resource. #1026

Merged
merged 1 commit into from
May 25, 2022

Conversation

VanillaSpoon
Copy link
Contributor

@VanillaSpoon VanillaSpoon commented May 16, 2022

Description

Return Admin Server URL as it is currently being manually constructed
MGDSTRM-8163

Note:
The Admin Server URL will only be returned when a kafka is in a ready status.

Verification Steps

  • Create a cluster on OpenShift or with CRC
  • Deploy kas-fleet-manager

To get the kafka information,you may run:
curl -v -XGET -H "Authorization: Bearer $(ocm token)" http://localhost:8000/api/kafkas_mgmt/v1/kafkas/<kafkaID> | jq

Once "status": "ready", your output should look like this:

{
"id": "ca6ao5f13honsptlr6mg",
"kind": "Kafka",
"href": "/api/kafkas_mgmt/v1/kafkas/ca6ao5f13honsptlr6mg",
"status": "ready",
"cloud_provider": "aws",
"multi_az": true,
"region": "us-east-1",
"owner": "egallina_kafka_service",
"name": "serviceapi",
"bootstrap_server_host": "[serviceapi-ca-ao-f--honsptlr-mg.kas.egallina-kfm.qgrd.s1.devshift.org:443](http://serviceapi-ca-ao-f--honsptlr-mg.kas.egallina-kfm.qgrd.s1.devshift.org:443/)",
"admin_api_server_url": "http://admin-server-serviceapi-ca-ao-f--honsptlr-mg.kas.egallina-kfm.qgrd.s1.devshift.org/",
"created_at": "2022-05-24T10:57:41.45165+01:00",
"updated_at": "2022-05-24T11:05:44.178069+01:00",
"version": "3.0.1",
"instance_type": "standard",
"instance_type_name": "Standard",
"reauthentication_enabled": true,
"kafka_storage_size": "1000Gi",
"browser_url": "http://localhost:8080/ca6ao5f13honsptlr6mg/dashboard",
"size_id": "x1",
"ingress_throughput_per_sec": "50Mi",
"egress_throughput_per_sec": "100Mi",
"total_max_connections": 3000,
"max_partitions": 1500,
"max_data_retention_period": "P14D",
"max_connection_attempts_per_sec": 100
}

Note:
This is the URL we have returned:

+ "admin_api_server_url": "http://admin-server-serviceapi-ca-ao-f--honsptlr-mg.kas.egallina-kfm.qgrd.s1.devshift.org/",

Check the Database:

You may also check that the url has been added to the database in the kafka_requests table as admin_api_server_url

  • To access the database, run make db/login.
  • To pull the contents of admin_api_server_url, run select admin_api_server_url from kafka_requests;
    Your output should look as follows:
                                   admin_api_server_url                                    
-------------------------------------------------------------------------------------------
 http://admin-server-serviceapi-ca-afhv--hol-rnheg-g.kas.egallina-kfm.qgrd.s1.devshift.org
(1 row)

Checklist (Definition of Done)

  • All acceptance criteria specified in JIRA have been completed
  • Unit and integration tests added that prove the fix is effective or the feature works (tested against emulated and non-emulated OCM environment)
  • Documentation added for the feature
  • CI and all relevant tests are passing
  • Code Review completed
  • Verified independently by reviewer
  • All PR comments are resolved either by addressing them or creating follow up tasks
  • Required metrics/dashboards/alerts have been added (or PR created).
  • Required Standard Operating Procedure (SOP) is added.
  • JIRA has been created for changes required on the client side

@@ -1660,6 +1663,7 @@ components:
owner: "api_kafka_service"
name: "serviceapi"
bootstrap_server_host: "serviceapi-1isy6rq3jki8q0otmjqfd3ocfrg.apps.mk-bttg0jn170hp.x5u8.s1.devshift.org"
admin_api_server_url: "admin-server-mk-e-e-e-e-c---{}ld{-}-n-vp--bltg.bf2.kafka-stage.rhcloud.com"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
admin_api_server_url: "admin-server-mk-e-e-e-e-c---{}ld{-}-n-vp--bltg.bf2.kafka-stage.rhcloud.com"
admin_api_server_url: "https://admin-server-mk-e-e-e-e-c---{}ld{-}-n-vp--bltg.rhcloud.com"

@@ -1685,6 +1689,7 @@ components:
owner: "api_kafka_service"
name: "serviceapi"
bootstrap_server_host: "serviceapi-1isy6rq3jki8q0otmjqfd3ocfrg.apps.mk-bttg0jn170hp.x5u8.s1.devshift.org"
admin_api_server_url: "admin-server-mk-e-e-e-e-c---{}ld{-}-n-vp--bltg.bf2.kafka-stage.rhcloud.com"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
admin_api_server_url: "admin-server-mk-e-e-e-e-c---{}ld{-}-n-vp--bltg.bf2.kafka-stage.rhcloud.com"
admin_api_server_url: "https://admin-server-mk-e-e-e-e-c---{}ld{-}-n-vp--bltg.bf2.rhcloud.com"

Comment on lines 530 to 534
endpoint:
type: object
properties:
adminServerURI:
type: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VanillaSpoon why the endpoint object? I was expecting something similar to

Suggested change
endpoint:
type: object
properties:
adminServerURI:
type: string
adminServerURI:
type: string

Copy link
Contributor

@machi1990 machi1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @VanillaSpoon, this is really great work.
I've left a couple of comments in line with each code change.

I also think we could add an integration test
And also perform a verification against a real OpenShift cluster.

@VanillaSpoon VanillaSpoon force-pushed the adminServerURL branch 2 times, most recently from eeed6c1 to 9b16e39 Compare May 24, 2022 14:30
@VanillaSpoon VanillaSpoon marked this pull request as ready for review May 24, 2022 14:30
@VanillaSpoon VanillaSpoon requested review from a team, valdar and dhirajsb as code owners May 24, 2022 14:30
Copy link
Contributor

@JameelB JameelB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this @VanillaSpoon!

Because the admin_api_server_url is not required, if it's empty, this field isn't going to be included in the response. Once the kafka is in a ready state, only then it will appear in the response. Might be good to confirm this behaviour with UI/CLI team

Other than that, looks good to me 👍

@machi1990
Copy link
Contributor

Great work on this @VanillaSpoon!

Because the admin_api_server_url is not required, if it's empty, this field isn't going to be included in the response. Once the kafka is in a ready state, only then it will appear in the response. Might be good to confirm this behaviour with UI/CLI team

Other than that, looks good to me +1

/cc @wtrocki @riccardo-forina FYI

@riccardo-forina
Copy link

Thank you for looping me in @machi1990. I think we are good with this, we show skeleton loaders for the connection details panel for a pending Kafka instance. Not getting the field should be ok.

@wtrocki
Copy link
Contributor

wtrocki commented May 24, 2022

Because the admin_api_server_url is not required, if it's empty, this field isn't going to be included in the response. Once the kafka is in a ready state, only then it will appear in the response.

This doesnt matter for consumers. Consumers will not rely on responses but rather on sdk objects where this field is always present but can be empty

@machi1990 machi1990 dismissed their stale review May 25, 2022 07:20

comments addressed

@machi1990 machi1990 merged commit f8245e5 into bf2fc6cc711aee1a0c2a:main May 25, 2022
@machi1990
Copy link
Contributor

Thanks for the great work @VanillaSpoon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants