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: use configured RayCluster service account when autoscaling #259

Merged
merged 1 commit into from
May 18, 2022

Conversation

davidxia
Copy link
Contributor

@davidxia davidxia commented May 11, 2022

Related issue number

closes #242

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

@davidxia davidxia marked this pull request as ready for review May 11, 2022 21:29
@davidxia

This comment was marked as outdated.

@davidxia davidxia force-pushed the patch1 branch 2 times, most recently from afdee67 to 4aa4af1 Compare May 12, 2022 18:47
ray-operator/controllers/common/rbac_test.go Outdated Show resolved Hide resolved
ray-operator/controllers/common/rbac.go Outdated Show resolved Hide resolved
@davidxia davidxia force-pushed the patch1 branch 3 times, most recently from 64a8b35 to b22feeb Compare May 12, 2022 20:58
@davidxia
Copy link
Contributor Author

This is ready for review now

@Jeffwan
Copy link
Collaborator

Jeffwan commented May 12, 2022

The change looks good to me and please help move test to _test.go. Thanks! @davidxia

@davidxia
Copy link
Contributor Author

The change looks good to me and please help move test to _test.go. Thanks! @davidxia

@Jeffwan whoops, thanks. Done!

@davidxia
Copy link
Contributor Author

@Jeffwan lmk next steps if any that are required for merging this. Thanks! 🙏

@Jeffwan Jeffwan merged commit 600722b into ray-project:master May 18, 2022
@Jeffwan
Copy link
Collaborator

Jeffwan commented May 18, 2022

Thanks for the contribution!

davidxia added a commit to davidxia/kuberay that referenced this pull request May 25, 2022
Currently the patch in ray-project#259 isn't working because a later line overwrites with
the original bad value. This change sets the head pod's service account
to the one configured in the RayCluster.
davidxia added a commit to davidxia/kuberay that referenced this pull request May 26, 2022
Currently the patch in ray-project#259 isn't working because a later line overwrites with
the original bad value. This change sets the head pod's service account
to the one configured in the RayCluster.
davidxia added a commit to davidxia/kuberay that referenced this pull request Jun 6, 2022
Currently the patch in ray-project#259 isn't working because a later line overwrites with
the original bad value. This change sets the head pod's service account
to the one configured in the RayCluster.
Jeffwan pushed a commit that referenced this pull request Jun 8, 2022
Currently the patch in #259 isn't working because a later line overwrites with
the original bad value. This change sets the head pod's service account
to the one configured in the RayCluster.
davidxia added a commit to davidxia/kuberay that referenced this pull request Jun 19, 2022
instead of always using RayCluster name. Default to RayCluster
name if the ServiceAccount doesn't exist.

This is a follow up fix after
ray-project#259 and
ray-project#276.

The use case is when using the RayCluster autoscaler. The autoscaler
will use the head group's K8s ServiceAccount to make K8s API requests.
Always using the RayCluster name as the RoleBinding's subject name won't
be correct and will result in the autoscaler sidecar container not being
authorized by the K8s API.
DmitriGekhtman pushed a commit that referenced this pull request Jun 21, 2022
instead of always using RayCluster name. Default to RayCluster
name if the ServiceAccount doesn't exist.

This is a follow up fix after
#259 and
#276.

The use case is when using the RayCluster autoscaler. The autoscaler
will use the head group's K8s ServiceAccount to make K8s API requests.
Always using the RayCluster name as the RoleBinding's subject name won't
be correct and will result in the autoscaler sidecar container not being
authorized by the K8s API.
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
Currently the patch in ray-project#259 isn't working because a later line overwrites with
the original bad value. This change sets the head pod's service account
to the one configured in the RayCluster.
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
…roject#315)

instead of always using RayCluster name. Default to RayCluster
name if the ServiceAccount doesn't exist.

This is a follow up fix after
ray-project#259 and
ray-project#276.

The use case is when using the RayCluster autoscaler. The autoscaler
will use the head group's K8s ServiceAccount to make K8s API requests.
Always using the RayCluster name as the RoleBinding's subject name won't
be correct and will result in the autoscaler sidecar container not being
authorized by the K8s API.
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.

[Bug] The new SA used by autoscaler caused permission errors
2 participants