Add specify namespace installation #2700

fengshunli · 2023-03-06T10:06:45Z

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

fixes #2679

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

cheyang · 2023-03-06T10:13:18Z

/test fluid-e2e

codecov · 2023-03-06T10:24:25Z

Codecov Report

Merging #2700 (9083050) into master (73f1384) will decrease coverage by 0.01%.
The diff coverage is 72.72%.

❗ Current head 9083050 differs from pull request most recent head a110858. Consider uploading reports for the commit a110858 to get more accurate results

@@            Coverage Diff             @@
##           master    #2700      +/-   ##
==========================================
- Coverage   67.55%   67.54%   -0.01%     
==========================================
  Files         381      381              
  Lines       21968    21982      +14     
==========================================
+ Hits        14840    14848       +8     
- Misses       5381     5386       +5     
- Partials     1747     1748       +1

Impacted Files	Coverage Δ
pkg/utils/mount.go	`84.84% <40.00%> (-8.14%)`	⬇️
pkg/utils/helm/utils.go	`82.88% <100.00%> (+0.80%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

allenhaozi · 2023-03-06T11:17:03Z

charts/fluid/fluid/templates/_helpers.tpl

+{{- if .Values.installation.namespace -}}
+    {{ .Values.installation.namespace }}
+{{- else -}}
+    {{ .Release.Namespace}}


{{ .Release.Namespace }}

allenhaozi · 2023-03-06T11:35:10Z

good job !

The namespace setting are also set in the code, and need to be modified together

e.g.:
https://github.com/fluid-cloudnative/fluid/blob/master/pkg/common/constants.go#L157
https://github.com/fluid-cloudnative/fluid/blob/master/cmd/alluxio/app/alluxio.go#L72

You can check by keyword "fluid-system" with IDE's global search

fengshunli · 2023-03-06T13:20:08Z

The leader-election-namespace parameter currently does not support injection @allenhaozi @cheyang

fengshunli · 2023-03-07T03:56:45Z

good job !

The namespace setting are also set in the code, and need to be modified together

e.g.: https://github.com/fluid-cloudnative/fluid/blob/master/pkg/common/constants.go#L157 https://github.com/fluid-cloudnative/fluid/blob/master/cmd/alluxio/app/alluxio.go#L72

You can check by keyword "fluid-system" with IDE's global search

done

allenhaozi · 2023-03-07T11:19:18Z

good job !
The namespace setting are also set in the code, and need to be modified together
e.g.: https://github.com/fluid-cloudnative/fluid/blob/master/pkg/common/constants.go#L157 https://github.com/fluid-cloudnative/fluid/blob/master/cmd/alluxio/app/alluxio.go#L72
You can check by keyword "fluid-system" with IDE's global search

done

I found another reference that needs to be checked to see if it needs to be modified synchronously

https://github.com/fluid-cloudnative/fluid/blob/master/pkg/controllers/deploy/runtime_controllers.go#L55

fengshunli · 2023-03-07T11:43:53Z

good job !
The namespace setting are also set in the code, and need to be modified together
e.g.: https://github.com/fluid-cloudnative/fluid/blob/master/pkg/common/constants.go#L157 https://github.com/fluid-cloudnative/fluid/blob/master/cmd/alluxio/app/alluxio.go#L72
You can check by keyword "fluid-system" with IDE's global search

done

I found another reference that needs to be checked to see if it needs to be modified synchronously

https://github.com/fluid-cloudnative/fluid/blob/master/pkg/controllers/deploy/runtime_controllers.go#L55

Is it possible to change it to datasetKey.Namespace?

allenhaozi · 2023-03-07T14:36:19Z

good job !
The namespace setting are also set in the code, and need to be modified together
e.g.: https://github.com/fluid-cloudnative/fluid/blob/master/pkg/common/constants.go#L157 https://github.com/fluid-cloudnative/fluid/blob/master/cmd/alluxio/app/alluxio.go#L72
You can check by keyword "fluid-system" with IDE's global search

done

I found another reference that needs to be checked to see if it needs to be modified synchronously
https://github.com/fluid-cloudnative/fluid/blob/master/pkg/controllers/deploy/runtime_controllers.go#L55

Is it possible to change it to datasetKey.Namespace?

This should be the same as the dataset controller's namespace, two ways to get this value are recommended.

env:
- name: MY_POD_NAMESPACE
          valueFrom:
            fieldRef:
              apiVersion: v1
              fieldPath: metadata.namespace

filePath :="/var/run/secrets/kubernetes.io/serviceaccount/namespace"
        nsRead ,err :=ioutil.ReadFile(filePath)
        if err !=nil {
            return ctrl.Result{}, err
        }

fengshunli · 2023-03-07T14:42:44Z

Can it be obtained through common.MyPodNamespace @allenhaozi

allenhaozi · 2023-03-08T02:26:06Z

Can it be obtained through common.MyPodNamespace @allenhaozi

https://github.com/fluid-cloudnative/fluid/blob/master/charts/fluid/fluid/templates/controller/dataset_controller.yaml#L50

The env is not set in the deployment of dataset controller. We need to add it synchronically

allenhaozi · 2023-03-08T04:25:09Z

charts/fluid/fluid/values.yaml

 image:
  imagePullSecrets: []

+installation:


helm charts values are all installation dependent,
so direct use of namespace is recommended

allenhaozi

/lgtm

TrafalgarZZZ · 2023-03-08T04:58:35Z

/test fluid-e2e

zwwhdls · 2023-03-09T01:44:06Z

charts/fluid/fluid/values.yaml

 image:
  imagePullSecrets: []

+namespace:


how about set fluid-system as default value to maintain previous behavior and prevent conflicts when upgrading?

-n fluid-system

I agree with @zwwhdls because we'd better not introduce any installation or upgrade change for the existing users. I think they will use fluid-system namespace if not specifying anything.

cheyang · 2023-03-09T08:29:52Z

pkg/controllers/deploy/runtime_controllers.go

 		}

 		if match {
+			ns, err := utils.GetEnvByKey(common.MyPodNamespace)


I think the variable names should be descriptive so that anyone reading the code can understand their purpose. For example, the variable names ns is not descriptive enough. How about using namespace instead?

cheyang · 2023-03-09T08:32:19Z

pkg/controllers/deploy/runtime_controllers.go

 		if match {
+			ns, err := utils.GetEnvByKey(common.MyPodNamespace)
+			if err != nil {
+				return controllerName, scaleout, errors.Wrapf(err, "get namespace from env failed, env key:%s", common.MyPodNamespace)


And if it's failed to get the namespace from environment variable, how about using default namespace fluid-system instead? In fact, I think the most of users will not change the default namespace.

The default is default, if you want to install to fluid-system, you need to specify -n fluid-system

But the existing default management namespace is fluid-system, I think we should keep this consistency.

Managed by the Release.Namespace mechanism

helm install -n fluid-system will change helm release from default to fluid-system. It doesn't make sense with existing users.

I also prefer using the system namespace with pre-defined name like other projects.

https://github.com/karmada-io/karmada/blob/master/charts/karmada/values.yaml#L23
https://github.com/openkruise/charts/blob/master/versions/kruise/1.3.0/values.yaml#L8

These lose the meaning of -n xxx

The fetch environment variable is set synchronously with the configuration

https://github.com/fluid-cloudnative/fluid/pull/2700/files#diff-f0b09f81d101553640da1f883b65ee8c3edc7e9d4d1d2834edfd0df652585a07R70

this shouldn't return error ?

These lose the meaning of -n xxx

I admire your professional，but the paramount concern is to ensure that our existing users are not adversely affected in production.

allenhaozi · 2023-03-10T01:57:24Z

charts/fluid/fluid/templates/controller/alluxioruntime_controller.yaml

 metadata:
  name: alluxioruntime-controller
-  namespace: fluid-system
+  namespace: {{ .Release.Namespace }}


This is a general approach, such as the following project
https://github.com/kubevela/kubevela/blob/master/charts/vela-core/templates/kubevela-controller.yaml#L200
https://github.com/crossplane/crossplane/blob/master/cluster/charts/crossplane/templates/deployment.yaml#L5

But given the compatibility of previous strategy

# values.yaml # If the user wants to use -n mode, remove the default value of values namespace: "fluid-system"

# _helpers.tpl {{- define "fluid.namespace" -}} {{- if .Values.namespace }} {{- .Values.namespace }} {{- else }} {{- .Release.Namespace -}} {{- end }} {{- end }}

kind: Deployment metadata: name: alluxioruntime-controller namespace: {{ include "fluid.namespace" . }}

If the user wants to use -n mode, remove the default value of values

use --set to override the default
helm install fluid ./fluid -n test --set namespace=''

Signed-off-by: fengshunli <1171313930@qq.com>

sonarqubecloud · 2023-03-10T06:25:39Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

cheyang · 2023-03-11T03:25:37Z

charts/fluid/fluid/values.yaml

  image: fluidcloudnative/fluid-crd-upgrader:v0.9.0-a21f6b3

+##  if unspecified, will use built-in variable `.Release.Namespace`.
+namespace: fluid-system


Thanks very much!

cheyang · 2023-03-11T06:42:00Z

/test fluid-e2e

cheyang · 2023-03-11T10:04:43Z

/lgtm
/approve

fluid-e2e-bot · 2023-03-11T10:04:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: allenhaozi, cheyang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [cheyang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cheyang · 2023-03-11T10:05:08Z

Thanks @fengshunli for your contribution!

cheyang requested review from TrafalgarZZZ, allenhaozi and zwwhdls March 6, 2023 10:13

fengshunli force-pushed the ns branch from c7ab519 to 3eb9e79 Compare March 6, 2023 10:17

allenhaozi reviewed Mar 6, 2023

View reviewed changes

fengshunli force-pushed the ns branch from 3eb9e79 to 2d56132 Compare March 6, 2023 12:04

fengshunli force-pushed the ns branch 2 times, most recently from ffb8db5 to fad9b96 Compare March 7, 2023 03:47

fengshunli force-pushed the ns branch 3 times, most recently from f6b8ce2 to 56a0edc Compare March 8, 2023 03:06

allenhaozi reviewed Mar 8, 2023

View reviewed changes

fengshunli force-pushed the ns branch from 56a0edc to c4dceca Compare March 8, 2023 04:39

allenhaozi approved these changes Mar 8, 2023

View reviewed changes

zwwhdls reviewed Mar 9, 2023

View reviewed changes

fengshunli force-pushed the ns branch 2 times, most recently from 2221430 to 3c78efb Compare March 9, 2023 04:11

cheyang reviewed Mar 9, 2023

View reviewed changes

cheyang requested a review from allenhaozi March 9, 2023 08:41

allenhaozi reviewed Mar 10, 2023

View reviewed changes

fengshunli force-pushed the ns branch 3 times, most recently from 643caa1 to b64d49d Compare March 10, 2023 05:04

Add specify namespace installation

a110858

Signed-off-by: fengshunli <1171313930@qq.com>

fengshunli force-pushed the ns branch from b64d49d to a110858 Compare March 10, 2023 06:25

cheyang reviewed Mar 11, 2023

View reviewed changes

cheyang approved these changes Mar 11, 2023

View reviewed changes

fluid-e2e-bot bot added the approved label Mar 11, 2023

fluid-e2e-bot bot assigned cheyang Mar 11, 2023

fluid-e2e-bot bot added the lgtm label Mar 11, 2023

fluid-e2e-bot bot merged commit 4045191 into fluid-cloudnative:master Mar 11, 2023

RongGu mentioned this pull request Aug 18, 2024

[Community] Nominating Shunli Feng (GithubID: fengshunli) for Fluid committer fluid-cloudnative/community#65

Closed

Add specify namespace installation #2700

Add specify namespace installation #2700

Uh oh!

Conversation

fengshunli commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Uh oh!

cheyang commented Mar 6, 2023

Uh oh!

codecov bot commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allenhaozi commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fengshunli commented Mar 6, 2023

Uh oh!

fengshunli commented Mar 7, 2023

Uh oh!

allenhaozi commented Mar 7, 2023

Uh oh!

fengshunli commented Mar 7, 2023

Uh oh!

allenhaozi commented Mar 7, 2023

Uh oh!

fengshunli commented Mar 7, 2023

Uh oh!

allenhaozi commented Mar 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allenhaozi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TrafalgarZZZ commented Mar 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zwwhdls Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheyang Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fengshunli commented Mar 6, 2023 •

edited

Loading

codecov bot commented Mar 6, 2023 •

edited

Loading

allenhaozi commented Mar 6, 2023 •

edited

Loading

allenhaozi left a comment •

edited

Loading

zwwhdls Mar 9, 2023 •

edited

Loading

cheyang Mar 9, 2023 •

edited

Loading

fengshunli Mar 10, 2023 •

edited

Loading