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.

Sign up for GitHub

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

Jump to bottom

KubeRay: Relocate files to enable controller extension with Kubebuilder #268

Merged
merged 5 commits into from
May 19, 2022
Merged

KubeRay: Relocate files to enable controller extension with Kubebuilder #268

merged 5 commits into from
May 19, 2022

Conversation

brucez-anyscale
Copy link
Contributor

Why are these changes needed?

As discussed, we want to have more controllers for serve and job. This pr relocates controller files to enable future api create by kubebuilder

Related issue number

#214

Checks

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

@Jeffwan
Copy link
Collaborator

Jeffwan commented May 19, 2022

Seem PR makes following changes to prepare for next PR.

  1. rename api to apis. create clients under corresponding
  2. move ray_controllers.go to separate folder.

@Jeffwan
Copy link
Collaborator

Jeffwan commented May 19, 2022 •
edited

The change looks good to me. Please double check whether the common used Make commands works. I think CI covers some of them.

brucez-anyscale
brucez-anyscale commented May 19, 2022
View reviewed changes
ray-operator/Makefile
@@ -82,14 +82,14 @@ docker-push: ## Push docker image with the manager.
##@ Deployment

install: manifests kustomize ## Install CRDs into the K8s cluster specified in ~/.kube/config.
$(KUSTOMIZE) build config/crd | kubectl apply -f -
($(KUSTOMIZE) build config/crd | kubectl create -f -) || ($(KUSTOMIZE) build config/crd | kubectl replace -f -)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use create and replace instead of apply due to
The CustomResourceDefinition "rayclusters.ray.io" is invalid: metadata.annotations: Too long: must have at most 262144 bytes

@brucez-anyscale
Copy link
Contributor Author

The change looks good to me. Please double check whether the common used Make commands works. I think CI covers some of them.

Thanks. I do make manifests to auto generate the yaml again. And then I need to improve the Makefile accordingly.

DmitriGekhtman
DmitriGekhtman reviewed May 19, 2022
View reviewed changes
ray-operator/config/crd/bases/ray.io_rayclusters.yaml
@@ -321,10 +321,53 @@ spec:
of {key,value} pairs.
type: object
type: object
namespaceSelector:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also saw these changes when running Make manifests locally.
It looks like the embedded pod template has been updated to a newer Kubernetes version (namespaceSelector was introduced in K8s 1.21).

Looks like some component was upgraded from K8s 1.19 to 1.23 in this PR? Not sure if that's related.

cc @chenk008 @Jeffwan -- do you know what's up here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. Due to that version upgrade, controller-runtime was upgraded as well. namespaceSelector was introduced to extend pod affinity (it's still alpha in 1.21).

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's still compatible with 1.19 cluster and user needs to use kubectl replace -f xx.yaml to upgrade crd.

Copy link
Collaborator

@DmitriGekhtman DmitriGekhtman May 19, 2022 •
edited

Choose a reason for hiding this comment

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

Should we have the CI run Make manifests and check for changes? Maybe also a pre-push hook?

Copy link
Collaborator

Choose a reason for hiding this comment

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

At least in CI please

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me open an issue for that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

#269

@brucez-anyscale brucez-anyscale merged commit e0de2bd into master May 19, 2022
This was referenced May 19, 2022
Closed
Closed
@DmitriGekhtman DmitriGekhtman deleted the brucez/refactorRayCluster branch December 3, 2022 00:01
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
@brucez-anyscale
KubeRay: Relocate files to enable controller extension with Kubebuild…
944e94c 
…er (ray-project#268)

* KubeRay: Relocate files to enable controller extension with Kubebuilder

* update suite test

* address comments

* Fix apply issue

* make manifests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DmitriGekhtman DmitriGekhtman DmitriGekhtman approved these changes

@Jeffwan Jeffwan Jeffwan approved these changes

@simon-mo simon-mo Awaiting requested review from simon-mo

@akanso akanso Awaiting requested review from akanso

Assignees

@Jeffwan Jeffwan

@DmitriGekhtman DmitriGekhtman

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@brucez-anyscale @Jeffwan @simon-mo @DmitriGekhtman