[ISSUE-479] [operator] refine operator's build system #491

advancedxy · 2023-01-16T12:11:11Z

What changes were proposed in this pull request?

image name defaults to rss-$module when no registry is specifed
upgrade envtest to 1.24.1 to support Apple M1 chips
modify Dockerfile to replace base image from alpine to debian slim
remove gorequest dep
modify go.mod and related changes to upgrade go version to 1.17
add new changes in auto-generated code(by go 1.17 and above)

Why are the changes needed?

to address #479.

Does this PR introduce any user-facing change?

No

How was this patch tested?

No need

advancedxy · 2023-01-16T12:11:46Z

@wangao1236 please take a look when you have time.

zuston · 2023-01-16T12:28:33Z

I have a question about this, when I built the rss image from the master branch, the image version is release.0.4.0.316.g01d37beb

This makes me confused.

advancedxy · 2023-01-16T12:48:47Z

I have a question about this, when I built the rss image from the master branch, the image version is release.0.4.0.316.g01d37beb

This makes me confused.

Me too. I'm not sure why this version format is used. Maybe @wangao1236 could give us some hint.

wangao1236 · 2023-01-16T12:57:01Z

I have a question about this, when I built the rss image from the master branch, the image version is release.0.4.0.316.g01d37beb
This makes me confused.

Me too. I'm not sure why this version format is used. Maybe @wangao1236 could give us some hint.

This is just a way of differentiating versions. You can customize the version name by setting value of "IMAGE_VERSION". Maybe we can define a new way to differentiate versions. Do you have any suggestions?

codecov-commenter · 2023-01-17T03:14:26Z

Codecov Report

Merging #491 (7bb48f9) into master (0a6605b) will increase coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #491      +/-   ##
============================================
+ Coverage     59.73%   59.78%   +0.04%     
- Complexity     1764     1770       +6     
============================================
  Files           205      205              
  Lines         11527    11531       +4     
  Branches       1033     1033              
============================================
+ Hits           6886     6894       +8     
+ Misses         4234     4232       -2     
+ Partials        407      405       -2

Impacted Files	Coverage Δ
...org/apache/uniffle/server/ShuffleFlushManager.java	`84.37% <0.00%> (+0.33%)`	⬆️
...he/uniffle/server/storage/LocalStorageManager.java	`87.00% <0.00%> (+1.12%)`	⬆️
...pache/hadoop/mapreduce/task/reduce/RssFetcher.java	`92.30% <0.00%> (+1.53%)`	⬆️

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

wangao1236 · 2023-01-17T03:53:45Z

deploy/kubernetes/operator/Makefile


 MODULES ?= webhook controller

 # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
-ENVTEST_K8S_VERSION = 1.22.1
+ENVTEST_K8S_VERSION = 1.24.1


Maybe you can update it in go.mod as well?

which lib should I update the version? k8s.io/api v0.22.2 -> what version?

I'm not quite family with envtest...

Actually my question is why update envtest?

It seems that the names of individual fields in k8s v1.24 have been changed, and I recommend a separate PR to change the k8s version.

Actually my question is why update envtest?

To quote:upgrade envtest to 1.24.1 to support Apple M1 chips. envtest v1.22.x doesn't support M1 chip, not publish related arm archives. So, you cannot test the controller on M1.

The operator's required k8s in go.mod doesn't change, and still goes to 1.22 (k8s.io/api v0.22.x). And RSS's operator doesn't seem to use too much advanced features.

As discussed earlier, why upgrade envtest to 1.24 when you don't need to upgrade k8s to 1.24?
cc @advancedxy

As discussed earlier, why upgrade envtest to 1.24 when you don't need to upgrade k8s to 1.24? cc @advancedxy

Yeah, as discussed earlier. Envtest is upgraded to 1.24 to support Apple M1's chips as 1.22 doesn't work for M1 chip.

The operator itself doesn't need to upgrade to 1.24 and it shouldn't since many K8S clusters are in old versions.

As discussed earlier, why upgrade envtest to 1.24 when you don't need to upgrade k8s to 1.24? cc @advancedxy

Yeah, as discussed earlier. Envtest is upgraded to 1.24 to support Apple M1's chips as 1.22 doesn't work for M1 chip.

The operator itself doesn't need to upgrade to 1.24 and it shouldn't since many K8S clusters are in old versions.

Ok, I get it.

advancedxy · 2023-01-17T09:04:51Z

deploy/kubernetes/operator/api/uniffle/v1alpha1/zz_generated.deepcopy.go

@@ -1,3 +1,4 @@
+//go:build !ignore_autogenerated


This is due to locally dev machine's go version is 1.17 or higher(such as 1.19).

I will create a followup pr to upgrade the go version to 1.17.

advancedxy · 2023-01-17T09:10:14Z

deploy/kubernetes/operator/hack/Dockerfile

 WORKDIR /
 COPY --from=builder /workspace/${MODULE} .
-
-CMD ["${MODULE}"]


Previously, this is wrong:

the ARG MODULE is not defined in the second stage. the module might be empty or undefined.

MODULE is only available in build time. You cannot start the container by default since the MODULE env is not defined in the container.

cc @wangao1236

advancedxy · 2023-01-17T12:02:36Z

@zuston could you also help review this pr.

zuston · 2023-01-17T13:46:50Z

@zuston could you also help review this pr.

OK. I will take a look tomorrow, but to be honest, I don’t have much golang experience.

deploy/kubernetes/operator/pkg/webhook/util/util.go

kaijchen · 2023-01-28T16:12:25Z

Rerun CI to see if #512 can be fixed.

advancedxy · 2023-01-29T07:19:31Z

@wangao1236 @kaijchen please take a look at this. I would like this to be merged today as there are other changes based on this.

kaijchen

I took a glance of the changes, mostly looks good.
I'm not familiar with the build-operator.sh and the Makefile part.

deploy/kubernetes/operator/hack/Dockerfile

advancedxy · 2023-01-29T10:58:10Z

ping @wangao1236 @kaijchen

wangao1236 · 2023-01-29T12:35:35Z

deploy/kubernetes/operator/Makefile


 MODULES ?= webhook controller

 # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
-ENVTEST_K8S_VERSION = 1.22.1
+ENVTEST_K8S_VERSION = 1.24.1


As discussed earlier, why upgrade envtest to 1.24 when you don't need to upgrade k8s to 1.24? cc @advancedxy

Yeah, as discussed earlier. Envtest is upgraded to 1.24 to support Apple M1's chips as 1.22 doesn't work for M1 chip.

The operator itself doesn't need to upgrade to 1.24 and it shouldn't since many K8S clusters are in old versions.

Ok, I get it.

kaijchen

LGTM, thanks @advancedxy for the improvement.

wangao1236 · 2023-01-29T14:52:05Z

deploy/kubernetes/operator/hack/Dockerfile

-
-CMD ["${MODULE}"]
+ENV OPERATOR_BIN=/${MODULE}
+ENTRYPOINT "/${OPERATOR_BIN}"


I suggest using exec mode in dockerfile. For example, ENTRYPOINT ["${OPERATOR_BIN}"].
Ref: https://emmer.dev/blog/docker-shell-vs.-exec-form/
However, this does not affect normal running.

Good suggestion. Let's raise a follow up pr to quick address it.

advancedxy force-pushed the issue_479 branch from 36e47ad to 71f3ebf Compare January 17, 2023 02:51

wangao1236 reviewed Jan 17, 2023

View reviewed changes

advancedxy commented Jan 17, 2023

View reviewed changes

advancedxy requested review from zuston and kaijchen January 17, 2023 12:02

zuston reviewed Jan 18, 2023

View reviewed changes

deploy/kubernetes/operator/pkg/webhook/util/util.go Show resolved Hide resolved

advancedxy changed the title ~~[operator] better build system support~~ [ISSUE-479] [operator] better build system support Jan 19, 2023

advancedxy force-pushed the issue_479 branch from a298ded to 9aa72f5 Compare January 29, 2023 02:34

advancedxy mentioned this pull request Jan 29, 2023

[ISSUE-512][Operator] Bump golang to 1.17 #515

Merged

advancedxy changed the title ~~[ISSUE-479] [operator] better build system support~~ [ISSUE-479] [operator] refine operator's build system Jan 29, 2023

advancedxy added 8 commits January 29, 2023 14:00

[operator] better build system support

af80264

build docker image on pr

83a8e95

replace [[ ]] test with [ ]

b667d0e

use grep

5e43467

upgrade to go 1.17

53ccea6

makefile tweak

b29c5c9

upgrade to go 1.17

475eb09

change default shell to bash

d34d5de

advancedxy force-pushed the issue_479 branch from 08922fd to d34d5de Compare January 29, 2023 06:11

kaijchen reviewed Jan 29, 2023

View reviewed changes

deploy/kubernetes/operator/hack/Dockerfile Show resolved Hide resolved

Add comment for Dockerfile

93dae74

fix webhook panic

7bb48f9

wangao1236 approved these changes Jan 29, 2023

View reviewed changes

kaijchen approved these changes Jan 29, 2023

View reviewed changes

kaijchen merged commit e1be8d6 into apache:master Jan 29, 2023

wangao1236 reviewed Jan 29, 2023

View reviewed changes

advancedxy mentioned this pull request Jan 30, 2023

[Improvement] [Operator] better build system support #479

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE-479] [operator] refine operator's build system #491

[ISSUE-479] [operator] refine operator's build system #491

advancedxy commented Jan 16, 2023 •

edited

advancedxy commented Jan 16, 2023

zuston commented Jan 16, 2023

advancedxy commented Jan 16, 2023

wangao1236 commented Jan 16, 2023

codecov-commenter commented Jan 17, 2023 •

edited

wangao1236 Jan 17, 2023

advancedxy Jan 17, 2023

wangao1236 Jan 17, 2023

wangao1236 Jan 17, 2023

advancedxy Jan 17, 2023

wangao1236 Jan 29, 2023 •

edited

advancedxy Jan 29, 2023

wangao1236 Jan 29, 2023

advancedxy Jan 17, 2023

advancedxy Jan 17, 2023

advancedxy commented Jan 17, 2023

zuston commented Jan 17, 2023

kaijchen commented Jan 28, 2023

advancedxy commented Jan 29, 2023

kaijchen left a comment

advancedxy commented Jan 29, 2023

wangao1236 Jan 29, 2023

kaijchen left a comment

wangao1236 Jan 29, 2023

advancedxy Jan 30, 2023

[ISSUE-479] [operator] refine operator's build system #491

[ISSUE-479] [operator] refine operator's build system #491

Conversation

advancedxy commented Jan 16, 2023 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

advancedxy commented Jan 16, 2023

zuston commented Jan 16, 2023

advancedxy commented Jan 16, 2023

wangao1236 commented Jan 16, 2023

codecov-commenter commented Jan 17, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangao1236 Jan 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

advancedxy commented Jan 17, 2023

zuston commented Jan 17, 2023

kaijchen commented Jan 28, 2023

advancedxy commented Jan 29, 2023

kaijchen left a comment

Choose a reason for hiding this comment

advancedxy commented Jan 29, 2023

Choose a reason for hiding this comment

kaijchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

advancedxy commented Jan 16, 2023 •

edited

codecov-commenter commented Jan 17, 2023 •

edited

wangao1236 Jan 29, 2023 •

edited