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

refactor(crd): rewrite the RisingWave and the controller #109

Merged
merged 17 commits into from
Jul 15, 2022

Conversation

arkbriar
Copy link
Collaborator

@arkbriar arkbriar commented Jul 13, 2022

Signed-off-by: arkbriar arkbriar@gmail.com

What's changed and what's your intention?

PLEASE DO NOT LEAVE THIS EMPTY !!!

Please explain IN DETAIL what the changes are in this PR and why they are needed:

  • Rewrite the API structs according to the proposal RFC: a new design of RisingWave CRD #105 and refactor the controller manager
    • Rewrite the RisingWave and add a new CR RisingWavePodTemplate
    • Add a validating webhook for RisingWavePodTemplate to ensure immutability
    • Refactor the controllers and webhooks
  • Provide several examples to demonstrate how to work with the new CRD
  • Move the legacy APIs into the legacy directory
  • Revise the readme and dev docs
  • Misc things
    • Bump controller-gen to 0.9.2 to fix an issue
    • Bump ctrlkit-gen to latest to fix an issue
    • Re-generate the CRDs as well as the manifests
    • Embed the apis package in the outside package
    • Declare the CRs in categories all and streaming

Now you can view the RisingWave related resources with the following command:

k get streaming
NAME                                                               AGE
risingwavepodtemplate.risingwave.singularity-data.com/privileged   5s

NAME                                                               RUNNING   STORAGE(META)   STORAGE(OBJECT)   AGE
risingwave.risingwave.singularity-data.com/risingwave-etcd-minio   True      Etcd            MinIO             2m22s
risingwave.risingwave.singularity-data.com/risingwave-in-memory    True      Memory          Memory            2m9s
risingwave.risingwave.singularity-data.com/risingwave-privileged   False     Memory          Memory            5s

Guidance to review:

  • The APIs under apis/risingwave/v1alpha1
  • The object factory under pkg/factory
  • The controller manager under pkg/manager
  • The others are generally generated or modified passively

Checklist

  • I have written the necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

Signed-off-by: arkbriar <arkbriar@gmail.com>
@arkbriar arkbriar force-pushed the refactor/risingwave-v1alpha1 branch from d4c4967 to 07b6124 Compare July 13, 2022 18:02
Signed-off-by: arkbriar <arkbriar@gmail.com>
Signed-off-by: arkbriar <arkbriar@gmail.com>
Signed-off-by: arkbriar <arkbriar@gmail.com>
Signed-off-by: arkbriar <arkbriar@gmail.com>
Signed-off-by: arkbriar <arkbriar@gmail.com>
Signed-off-by: arkbriar <arkbriar@gmail.com>
Signed-off-by: arkbriar <arkbriar@gmail.com>
Signed-off-by: arkbriar <arkbriar@gmail.com>
Signed-off-by: arkbriar <arkbriar@gmail.com>
Signed-off-by: arkbriar <arkbriar@gmail.com>
Signed-off-by: arkbriar <arkbriar@gmail.com>
…e golangci-lint

Signed-off-by: arkbriar <arkbriar@gmail.com>
…tus; remove useless default values from CRD

Signed-off-by: arkbriar <arkbriar@gmail.com>
Signed-off-by: arkbriar <arkbriar@gmail.com>
Signed-off-by: arkbriar <arkbriar@gmail.com>
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@Nebulazhang Nebulazhang left a comment

Choose a reason for hiding this comment

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

Rest LGTM! Do you have tested on AWS cluster? I suggest testing it on local cluster and AWS cluster.

README.md Show resolved Hide resolved
```

If you edit the `ConfigMap`, please restart the `risingwave-operator` to reload the configuration.

## Monitoring
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the Monitoring part?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's generally not telling the truth, so I think it should be removed for now, especially the service monitor part. We can add it back to the readme after implementing it.

Signed-off-by: arkbriar <arkbriar@gmail.com>
@arkbriar arkbriar added the mergify/can-merge Indicates that the PR can be added to the merge queue label Jul 15, 2022
@arkbriar arkbriar added this to the v0.1 milestone Jul 15, 2022
@mergify mergify bot merged commit 12c222b into main Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergify/can-merge Indicates that the PR can be added to the merge queue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants