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

Fix ingress annotation warnings #1196

Merged
merged 7 commits into from
Mar 5, 2024

Conversation

karlaspuldaro
Copy link
Contributor

@karlaspuldaro karlaspuldaro commented Jan 26, 2024

Summary

Details and comments

Since Kubernetes only supports their few latest minor versions, this PR removes support to the deprecated way of setting annotations.

@CLAassistant
Copy link

CLAassistant commented Jan 26, 2024

CLA assistant check
All committers have signed the CLA.

Remove annotation support to older kube versions
@karlaspuldaro
Copy link
Contributor Author

karlaspuldaro commented Feb 5, 2024

@psschwei @akihikokuroda Can you help with some guidance of how I can add tests to this fix?
For a test case, I imagine checking the className value in the gateway config, perhaps adding to this test file?

@akihikokuroda
Copy link
Collaborator

@karlaspuldaro It is difficult to add the test verifying the changes. We don't have the kubernetes level end to end testing now. We have the notebook tests that verify most of the middleware functionalities but it's based on the docker compose that doesn't use the kubernetes ingress. I believe that this PR can be accepted without adding the test. @psschwei WDYT?

@psschwei
Copy link
Collaborator

psschwei commented Feb 6, 2024

Yeah, I think we can accept this without tests in the CI, just run it locally to verify that it does what we expect it to.

@karlaspuldaro karlaspuldaro changed the title [WIP] Fix ingress annotation warnings Fix ingress annotation warnings Feb 6, 2024
@karlaspuldaro
Copy link
Contributor Author

@psschwei Local testing update: I'm going through some build issues related to kind and docker. I'll come back with another update once I confirm the warning is gone.

@karlaspuldaro
Copy link
Contributor Author

karlaspuldaro commented Feb 15, 2024

Update: still having issues completing my build and deployment.

So far, I was able to build gateway and ray node, but couldn't load the ray image into my kind cluster.
For context, I am using containerd (the only way I could successfully build from source) and it is apparently not supported by kind (even though I could load the gateway image just fine, just not ray).
Once I get the ray image push done, I can install quantum-serverless helm chart - and test if the warning messages are really gone.

@Tansito
Copy link
Member

Tansito commented Feb 19, 2024

Let us know if we can help you with something @karlaspuldaro 👍

@karlaspuldaro
Copy link
Contributor Author

@Tansito @akihikokuroda I think I need help reproducing the warnings locally from the main branch, then checking out to this PRs branch and comparing logs to see if they're gone.
I thought the warnings would be logged when installing helm charts, after building gateway and ray node, but I was wrong.
If someone could point me to the exact deployment step to recreate the issue, it would be very helpful. Thx!

@psschwei
Copy link
Collaborator

If someone could point me to the exact deployment step to recreate the issue, it would be very helpful. Thx!

You can modify the ingress section of values.yaml to include host/tls info... something like:

ingress:
  annotations:
    # For IBM Cloud the valid ingress class values are: public-iks-k8s-nginx and private-iks-k8s-nginx
    kubernetes.io/ingress.class: "nginx"
    nginx.ingress.kubernetes.io/proxy-buffers-number: "4"
    nginx.ingress.kubernetes.io/proxy-buffer-size: "512k"
    nginx.ingress.kubernetes.io/proxy-body-size: 50m
#  tls: []
  tls:
    - hosts:
      - "quantum-serverless-url.cloud"
      secretName: "tls-secret-name"
#  hosts: []
  hosts:
    - host: "quantum-serverless-url.cloud"
      paths:
      - path: /
        pathType: Prefix
        serviceName: "gateway"
        servicePort: 8000

@psschwei psschwei marked this pull request as ready for review March 4, 2024 20:20
@karlaspuldaro
Copy link
Contributor Author

Thank you @psschwei for helping test this PR locally and confirm the changes fix the issue.
For reviewers, to test locally:

  1. Reproduce ingress warnings on main branch: Deploy serverless helm chart on k8s 1.28+ (make sure ingress config is also deployed). helm install step should output ingress warnings.
  2. Check out to this branch and repeat steps above. helm install completes with no ingress warnings.

Copy link
Member

@Tansito Tansito left a comment

Choose a reason for hiding this comment

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

Thank you so much @karlaspuldaro ! 😄

@psschwei psschwei merged commit 194ec73 into Qiskit:main Mar 5, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

annotation "kubernetes.io/ingress.class" is deprecated, please use 'spec.ingressClassName' instead
5 participants