Skip to content

Conversation

@KPostOffice
Copy link
Contributor

Issue link

fixes: #436
fixes: #414

What changes have been made

Verification steps

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 5, 2024
@KPostOffice KPostOffice force-pushed the use-route-for-oauth branch 8 times, most recently from 5060506 to eb78f79 Compare January 10, 2024 17:53
@KPostOffice KPostOffice changed the title WIP: use route instead of ingress for oauth endpoint use route instead of ingress for oauth endpoint Jan 10, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2024
@Srihari1192 Srihari1192 self-requested a review January 11, 2024 12:18
@Srihari1192
Copy link
Contributor

@KPostOffice While testing this changes got below error at the step cluster.up()

Reason: Not Found
HTTP response headers: HTTPHeaderDict({'Audit-Id': 'b0fbe973-cdb4-4add-b7c1-8205f9fe1d96, b0fbe973-cdb4-4add-b7c1-8205f9fe1d96', 'Cache-Control': 'no-cache, private, no-store', 'Content-Length': '230', 'Content-Type': 'application/json', 'Date': 'Thu, 11 Jan 2024 12:15:00 GMT', 'Strict-Transport-Security': 'max-age=31536000; includeSubDomains; preload', 'X-Kubernetes-Pf-Flowschema-Uid': '2376fd90-99e5-4fd2-ab41-85d6965009e4', 'X-Kubernetes-Pf-Prioritylevel-Uid': 'edf2bb96-ae3c-4161-8ef3-1ba5c36f4d62'})
HTTP response body: b'{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"routes.route.openshift.io \\"mnist\\" not found","reason":"NotFound","details":{"name":"mnist","group":"route.openshift.io","kind":"routes"},"code":404}\n'


During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "mnist_raycluster_sdk.py", line 56, in <module>
    cluster.up()
  File "/codeflare-sdk/codeflare-sdk/src/codeflare_sdk/cluster/cluster.py", line 230, in up
    create_openshift_oauth_objects(
  File "/codeflare-sdk/codeflare-sdk/src/codeflare_sdk/utils/openshift_oauth.py", line 28, in create_openshift_oauth_objects
    _create_or_replace_oauth_route_object(
  File "/codeflare-sdk/codeflare-sdk/src/codeflare_sdk/utils/openshift_oauth.py", line 186, in _create_or_replace_oauth_route_object
    existing_route = v1_routes.get(name=cluster_name, namespace=namespace)
  File "/opt/app-root/lib64/python3.8/site-packages/kubernetes/dynamic/client.py", line 112, in get
    return self.request('get', path, **kwargs)
  File "/opt/app-root/lib64/python3.8/site-packages/kubernetes/dynamic/client.py", line 57, in inner
    raise api_exception(e)
kubernetes.dynamic.exceptions.NotFoundError: 404
Reason: Not Found
HTTP response headers: HTTPHeaderDict({'Audit-Id': 'b0fbe973-cdb4-4add-b7c1-8205f9fe1d96, b0fbe973-cdb4-4add-b7c1-8205f9fe1d96', 'Cache-Control': 'no-cache, private, no-store', 'Content-Length': '230', 'Content-Type': 'application/json', 'Date': 'Thu, 11 Jan 2024 12:15:00 GMT', 'Strict-Transport-Security': 'max-age=31536000; includeSubDomains; preload', 'X-Kubernetes-Pf-Flowschema-Uid': '2376fd90-99e5-4fd2-ab41-85d6965009e4', 'X-Kubernetes-Pf-Prioritylevel-Uid': 'edf2bb96-ae3c-4161-8ef3-1ba5c36f4d62'})
HTTP response body: b'{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"routes.route.openshift.io \\"mnist\\" not found","reason":"NotFound","details":{"name":"mnist","group":"route.openshift.io","kind":"routes"},"code":404}\n'
Original traceback: ```

Copy link
Contributor

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

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

As discussed during scrum, perhaps we would want to re-introduce the port_name and host for oauth route objects. Other than that, and Srihari's finding, this is a solid PR. Great job!

@KPostOffice KPostOffice force-pushed the use-route-for-oauth branch 2 times, most recently from 0b20c74 to 175d0d2 Compare January 11, 2024 17:24
Signed-off-by: Kevin <kpostlet@redhat.com>
Copy link
Contributor

@Srihari1192 Srihari1192 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2024
Copy link
Contributor

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

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

/lgtm great job!

@KPostOffice
Copy link
Contributor Author

/approve 🤫

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ChristianZaccaria, KPostOffice, Srihari1192

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 8a5ea19 into project-codeflare:main Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

3 participants