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

feat(k8s): init operator project & add webhook #259

Merged
merged 8 commits into from
Jul 28, 2023
Merged

feat(k8s): init operator project & add webhook #259

merged 8 commits into from
Jul 28, 2023

Conversation

Radeity
Copy link
Member

@Radeity Radeity commented Jul 12, 2023 •
edited
Loading

Purpose of the PR

Main Changes

  • Re-initialize go project (operator).
  • Add Webhook to deny update. Launch pod with two containers (original controller and new manager).

List some TODO here:

  • Update doc.
  • Modify CI and build workflow.
  • Make sure how to generate CRD for <1.16. Currently, meet incompatible api, here are error logs: apiVersion "apiextensions.k8s.io/v1beta1" is not supported when executing make gen-all under crd-generate

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows.
  • Manually test first.
    If you execute kubectl create -f hugegraphjob.yaml twice, you can get error log:
image

You can do the following things for local test

  • Use description yaml under computer-k8s-operator/manifests, or you can generate by yourself. 
    cd computer-k8s-operator/crd-generate
make gen-all
  • Build and Push manager image: 
    docker build -t hugegraph/hugegraph-computer-operator-manager:latest ./computer-k8s-operator/crd-generate
docker push hugegraph/hugegraph-computer-operator-manager:latest
  • Build and Push controller image: 
    mvn clean package -DskipTests
docker build -t hugegraph/hugegraph-computer-operator:latest -f ./computer-k8s-operator/Dockerfile ./computer-k8s-operator
docker push hugegraph/hugegraph-computer-operator:latest

Does this PR potentially affect the following parts?

  • Nope
  • Dependencies (add/update license info)
  • Modify configurations
  • The public API
  • Other affects (typed here)

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@Radeity Radeity marked this pull request as draft July 12, 2023 16:39
@Radeity Radeity marked this pull request as ready for review July 13, 2023 14:49
@codecov
Copy link

codecov bot commented Jul 13, 2023 •
edited
Loading

Codecov Report

Merging #259 (0de4449) into master (024d1aa) will decrease coverage by 0.14%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #259      +/-   ##
============================================
- Coverage     85.86%   85.72%   -0.14%     
+ Complexity     3235     3232       -3     
============================================
  Files           344      344              
  Lines         12115    12115              
  Branches       1092     1092              
============================================
- Hits          10402    10386      -16     
- Misses         1190     1207      +17     
+ Partials        523      522       -1
Impacted Files Coverage Δ
...a/org/apache/hugegraph/computer/k8s/Constants.java 90.90% <ø> (ø)
...h/computer/k8s/crd/model/HugeGraphComputerJob.java 100.00% <ø> (ø)

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Radeity
Copy link
Member Author

Radeity commented Jul 13, 2023

Hi @coderzc @imbajin @simon824 @javeme ,

Now, hugegraph-computer-operator-controller-manager pod will launch two containers: manager and controller.

  • controller remains the same which contains controller logic.
  • manager is launched for creating Webhook to deny update, cuz java client does not have convenient api, so this part is implemented in go, that's why we have to declare two containers.

We have to update doc, images, resource yaml file, I will be appreciated if you can instruct me how to finish these :D

Radeity
Radeity commented Jul 14, 2023
View reviewed changes
computer-k8s-operator/go.mod Outdated Show resolved Hide resolved
@coderzc
Copy link
Member

coderzc commented Jul 14, 2023

We have to update doc, images, resource yaml file, I will be appreciated if you can instruct me how to finish these :D

Good job！

BTY，I think we should consider rewriting controller in the future using the Go API.

@Radeity
Copy link
Member Author

Radeity commented Jul 15, 2023

Hi @coderzc , I have updated deployment guide in PR description. Can you help test it when you are available?

coderzc
coderzc previously approved these changes Jul 20, 2023
View reviewed changes
@coderzc
Copy link
Member

coderzc commented Jul 20, 2023 •
edited by imbajin
Loading

@javeme @imbajin PTAL

@Radeity Radeity changed the title feat: init operator project & add webhook feat(k8s): init operator project & add webhook Jul 21, 2023
javeme
javeme reviewed Jul 22, 2023
View reviewed changes
Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

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

some tiny comments

computer-k8s-operator/crd-generate/cmd/main.go
)

var (
scheme = runtime.NewScheme()
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant spaces?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not redundant, which is go's align way.

computer-k8s-operator/crd-generate/cmd/main.go
"sigs.k8s.io/controller-runtime/pkg/log/zap"

operatorv1 "hugegraph.apache.org/operator/api/v1"
//+kubebuilder:scaffold:imports
Copy link
Contributor

Choose a reason for hiding this comment

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

can we removed the unused code?

Copy link
Member Author

Choose a reason for hiding this comment

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

These comments are used by kubebuilder to locate code places for future possible auto-generation. We can not remove these, unless we maintain this file by ourselves from now on.

computer-k8s-operator/crd-generate/cmd/main.go
utilruntime.Must(clientgoscheme.AddToScheme(scheme))

utilruntime.Must(operatorv1.AddToScheme(scheme))
//+kubebuilder:scaffold:scheme
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

computer-k8s-operator/crd-generate/cmd/main.go Outdated Show resolved Hide resolved
computer-k8s-operator/crd-generate/cmd/main.go

setupLog.Info("starting manager")
if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {
setupLog.Error(err, "problem running manager")
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer "Failed to start Manager"

computer-k8s-operator/crd-generate/cmd/main.go
// LeaderElectionReleaseOnCancel: true,
})
if err != nil {
setupLog.Error(err, "unable to start manager")
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer "Unable to create Manager"

computer-k8s-operator/crd-generate/cmd/main.go
})
if err != nil {
setupLog.Error(err, "unable to start manager")
os.Exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just return err?

computer-k8s-operator/crd-generate/cmd/main.go

if err = (&operatorv1.HugeGraphComputerJob{}).SetupWebhookWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create webhook", "webhook", "HugeGraphComputerJob")
os.Exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

computer-k8s-operator/crd-generate/cmd/main.go
setupLog.Info("starting manager")
if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {
setupLog.Error(err, "problem running manager")
os.Exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Member Author

Choose a reason for hiding this comment

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

These os.Exit(1) and above log message are auto-generated, do we have to modify them?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, let's just keep as it is

computer-k8s-operator/crd-generate/config/default/manager_config_patch.yaml Outdated Show resolved Hide resolved
javeme
javeme approved these changes Jul 24, 2023
View reviewed changes
computer-k8s-operator/crd-generate/cmd/main.go
setupLog.Info("starting manager")
if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {
setupLog.Error(err, "problem running manager")
os.Exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, let's just keep as it is

imbajin
imbajin approved these changes Jul 28, 2023
View reviewed changes
Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

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

Busy with other tasks before 🚌

Thanks for the contribution, merge it now

@imbajin imbajin merged commit 1c60eee into apache:master Jul 28, 2023
6 of 7 checks passed
This was referenced Oct 17, 2023
Merged
Merged
@Radeity Radeity mentioned this pull request Oct 31, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javeme javeme javeme approved these changes

@imbajin imbajin imbajin approved these changes

@coderzc coderzc coderzc left review comments

@corgiboygsj corgiboygsj Awaiting requested review from corgiboygsj

Assignees
No one assigned
Labels
feature New feature improvement
Projects
HugeGraph Tasks
Status: ✅ Done
Milestone
No milestone
4 participants
@Radeity @coderzc @javeme @imbajin