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

Consider adding status.address.url in CR #28

Closed
tarilabs opened this issue Nov 13, 2023 · 4 comments
Closed

Consider adding status.address.url in CR #28

tarilabs opened this issue Nov 13, 2023 · 4 comments

Comments

@tarilabs
Copy link
Member

Is your feature request related to a problem? Please describe.
Suggestion while discussed with @danielezonca

Complementary to: #19
in the MR custom resource as part of the reconcile loop,
consider adding status.address.url similar to Knative Addressable: https://knative.dev/docs/eventing/sinks/#:~:text=Addressable%20objects%20receive%20and%20acknowledge%20an%20event%20delivered%20over%20HTTP%20to%20an%20address%20defined%20in%20their%20status.address.url%20field

Describe the solution you'd like
As above

Describe alternatives you've considered
Currently UI and other service can only "match" on Service based on naming convention

Additional context
This solution avoid assuming "naming conventions" to where the MR is reachable and exposed

@dhirajsb
Copy link
Contributor

dhirajsb commented Dec 7, 2023

Since K8s itself supports using . as service address, there is no need to add a separate url in status. @lampajr has validated that it works.

@rareddy
Copy link
Contributor

rareddy commented Dec 7, 2023

close this?

@lampajr
Copy link
Contributor

lampajr commented Dec 12, 2023

@lampajr has validated that it works.

I can confirm that building the address as <service-name>.<namespace>.svc.cluster.local:<port> works for intra-cluster communications.

On the other hand IMO exposing the Route hostname/address in the CR status could be helpful as in that case whoever needs it can easily retrieve it from the CR rather than building it.

@dhirajsb dhirajsb closed this as not planned Won't fix, can't repro, duplicate, stale Dec 17, 2023
@danielezonca
Copy link

The suggestion behind this ticket is to expose the URL of the MR in the ModelRegistry CR object.
The reason for this was to decouple where the registry is deployed and the CR.
The naming convention that @lampajr tested makes strong assumption on the CR and the actual topology of the deployment: aka if the ModelRegistry object has a name, this name must be used for the service/route and the registry must be deployed in the same namespace.

This suggestion was also to be "more" future proof about making Model Registry a single instance: nobody should make assumption on where the instance is.

I hope this better clarifies the idea behind :)

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

No branches or pull requests

5 participants