support rad install kubernetes --set values #5243

nithyatsu · 2023-03-07T18:56:20Z

Description

Introduce rad install kubernetes --set
which can set single or multiple comma separated values for helm chart.

ex:
rad install kubernetes --set global.ucp.image=radius.azurecr.io/ucpd,global.ucp.tag=latest

rad install kubernetes --set global.tracerProvider.zipkin.url=http://jaeger-collector.radius-monitoring.svc.cluster.local:9411/api/v2/spans

Issue reference

Partially Fixes: radius-project/core-team#1117 based on discussion here : https://github.com/project-radius/radius/issues/5173#issuecomment-1452655212

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Adds necessary unit tests for change
Adds necessary E2E tests for change
Unit tests passing
Extended the documentation / Created issue for it

ytimocin · 2023-03-08T20:17:21Z

cmd/applink-rp/radius-cloud.yaml

 logging:
  log: "info"
  json: true
+tracerProvider:
+  serviceName: "applications.core"


Should this be link?

ytimocin · 2023-03-08T20:18:01Z

cmd/rad/cmd/envInit.go

@@ -399,6 +399,6 @@ func createEnvironmentResource(ctx context.Context, kubeCtxName, resourceGroupNa
 }

 func isEmpty(chartArgs *setup.ChartArgs) bool {
-	var emptyChartArgs setup.ChartArgs
-	return (chartArgs == nil || *chartArgs == emptyChartArgs)
+	//var emptyChartArgs setup.ChartArgs


Why do we need this change?

Since I introduced a slice of string it does not allow it any more. I might have to add some more code to test fr isEmpty once I get the main functionality working.

ytimocin · 2023-03-08T20:18:30Z

deploy/Chart/charts/appcore-rp/link-self-host.yaml

@@ -40,7 +40,8 @@ logging:
  json: true
 tracerProvider:
  serviceName: "applications.link"
-
+  zipkin:
+    url: "http://jaeger-collector.radius-monitoring.svc.cluster.local:9411/api/v2/spans"


commented out code can be deleted

ytimocin · 2023-03-08T20:18:46Z

deploy/Chart/charts/appcore-rp/radius-self-host.yaml

-#    url: "http://jaeger-collector.radius-monitoring.svc.cluster.local:9411/api/v2/spans"
+    #url: "http://jaeger-collector.radius-monitoring.svc.cluster.local:9411/api/v2/spans"


same here (commented out code can be deleted)

ytimocin · 2023-03-08T20:19:18Z

deploy/Chart/charts/appcore-rp/templates/appcore-rp.yaml

@@ -57,7 +57,7 @@ spec:
        - containerPort: {{ .Values.global.prometheus.port }}
          name: metrics
          protocol: TCP
-        {{ end }}
+        {{ end }}       


this might create an issue while deploying the chart (extra spaces need to be deleted)

ytimocin · 2023-03-08T20:19:40Z

deploy/Chart/charts/ucp/ucp-config.yaml

@@ -64,7 +64,9 @@ logging:
  json: true

 tracerProvider:
-  serviceName: "ucp"
+  serviceName: "applications.core"


Should this be ucp?

ytimocin · 2023-03-08T20:19:48Z

deploy/Chart/charts/ucp/ucp-config.yaml

-  serviceName: "ucp"
+  serviceName: "applications.core"
+  zipkin:
+    url: "http://jaeger-collector.radius-monitoring.svc.cluster.local:9411/api/v2/spans"

 # TODO: Enable trace provider in 0.19


nit: remove

ytimocin · 2023-03-08T20:20:15Z

pkg/cli/helm/cluster.go

@@ -105,7 +105,9 @@ func PopulateDefaultClusterOptions(cliOptions CLIClusterOptions) ClusterOptions
 	if cliOptions.Radius.AWSProvider != nil {
 		options.Radius.AWSProvider = cliOptions.Radius.AWSProvider
 	}
-
+	if len(cliOptions.Radius.Values) > 0 {


Not quite sure why we need the values?

we are trying to implement https://github.com/project-radius/radius/issues/5173#issuecomment-1452655212
it would be somewhat like a wrapper around helm install ( or upgrade)'s set command

ytimocin · 2023-03-08T20:20:38Z

pkg/cli/helm/radiusclient.go

@@ -194,6 +207,35 @@ func GetAzProvider(options RadiusOptions, kubeContext string) (*azure.Provider,

 }

+func GetHelmValues(kubeContext string) (map[string]interface{}, error) {
+


nit: remove extra line

ytimocin · 2023-03-08T20:20:54Z

pkg/cli/setup/chart.go

@@ -24,8 +24,13 @@ type ChartArgs struct {
 	// for display purposes. This is useful when the the actual public IP address of a cluster's ingress
 	// is not a routable IP. This comes up all of the time for a local cluster.
 	PublicEndpointOverride string
+	Values                 []string


Do we need this?

yes, thats the support for rad install kubernetes --set command. (https://github.com/project-radius/radius/issues/5173#issuecomment-1452655212

github-actions · 2023-03-08T20:36:02Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.0 %
main branch coverage: 60.3 %

github-actions · 2023-03-08T21:20:18Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.1 %
main branch coverage: 60.3 %

github-actions · 2023-03-08T21:39:48Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.2 %
main branch coverage: 60.3 %

youngbupark

Please use the existing pkgs from helm repo as possible instead of creating new wheel.

youngbupark · 2023-03-08T21:41:31Z

pkg/cli/setup/chart.go

@@ -24,8 +24,13 @@ type ChartArgs struct {
 	// for display purposes. This is useful when the the actual public IP address of a cluster's ingress
 	// is not a routable IP. This comes up all of the time for a local cluster.
 	PublicEndpointOverride string
+	Values                 []string


Please add comments here.

Suggested change

Values []string

Values []string

youngbupark · 2023-03-08T21:47:33Z

pkg/cli/helm/radiusclient.go

@@ -213,6 +221,38 @@ func runRadiusHelmUpgrade(helmConf *helm.Configuration, releaseName string, helm
 	return runUpgrade(installClient, releaseName, helmChart)
 }

+func addChartValues(helmChart *chart.Chart, values []string) error {


We do not need to implement key and value parser.
Please refer the following helm chart pkgs:

https://github.com/helm/helm/blob/main/pkg/strvals/parser.go

https://github.com/helm/helm/blob/main/pkg/cli/values/options.go

reimplemented.

github-actions · 2023-03-08T22:24:10Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.1 %
main branch coverage: 60.3 %

vinayada1 · 2023-03-10T18:11:06Z

cmd/rad/cmd/envInit.go

@@ -400,5 +401,6 @@ func createEnvironmentResource(ctx context.Context, kubeCtxName, resourceGroupNa

 func isEmpty(chartArgs *setup.ChartArgs) bool {
 	var emptyChartArgs setup.ChartArgs
-	return (chartArgs == nil || *chartArgs == emptyChartArgs)
+	emptyChartArgs.Values = []string{}
+	return (chartArgs == nil || reflect.DeepEqual(*chartArgs, emptyChartArgs))


isn't it sufficient to check len?

chartArgs now has a slice Values, therefore deepcopy was needed..

Please see my comment. I recommend to use string for values arg because helm's parser parse strings.

I reimplemented using string. Please see if that looks better.

vinayada1 · 2023-03-10T18:53:51Z

pkg/cli/setup/chart.go

@@ -37,6 +42,7 @@ func RegisterPersistentChartArgs(cmd *cobra.Command) {
 	cmd.PersistentFlags().String("ucp-image", "", "Specify the UCP image to use")
 	cmd.PersistentFlags().String("ucp-tag", "", "Specify the UCP tag to use")
 	cmd.PersistentFlags().String("public-endpoint-override", "", "Specify the public IP address or hostname of the Kubernetes cluster. It must be in the format: <hostname>[:<port>]. Ex: 'localhost:9000'")
+	cmd.PersistentFlags().StringArrayVar(&values, "set", []string{}, "set values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)")


The name "values" isn't very intuitive. Have you checked this with PM?

@AaronCrawfis approved rad install kuberenetes --set.

I got the idea of values from dapr and it seemed to make sense since we are somewhat implementing a wrapper around helm's install/ upgrade, which will overwrite the values.yaml values (https://github.com/project-radius/radius/issues/5173#issuecomment-1452655212 ). Should I rename it to something else?

I would suggest run it by him once....whether we want to call it helmValues or something

vinayada1 · 2023-03-10T18:54:25Z

pkg/cli/setup/chart.go

@@ -24,8 +24,13 @@ type ChartArgs struct {
 	// for display purposes. This is useful when the the actual public IP address of a cluster's ingress
 	// is not a routable IP. This comes up all of the time for a local cluster.
 	PublicEndpointOverride string
+	Values                 []string


Could you add some comments here?

github-actions · 2023-03-13T02:54:06Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.3 %
main branch coverage: 60.3 %

youngbupark · 2023-03-13T19:14:38Z

pkg/cli/setup/chart.go

@@ -37,6 +44,7 @@ func RegisterPersistentChartArgs(cmd *cobra.Command) {
 	cmd.PersistentFlags().String("ucp-image", "", "Specify the UCP image to use")
 	cmd.PersistentFlags().String("ucp-tag", "", "Specify the UCP tag to use")
 	cmd.PersistentFlags().String("public-endpoint-override", "", "Specify the public IP address or hostname of the Kubernetes cluster. It must be in the format: <hostname>[:<port>]. Ex: 'localhost:9000'")
+	cmd.PersistentFlags().StringArrayVar(&values, "set", []string{}, "set values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)")


Use String() instead of StringArrayVar(). helm's strvals.ParseInto(val, helmChart.Values) has its own parser to parse comma and KV. So we do not need to get this as value array. Then you do not need addChartValues because you can just call strvals.ParseInto directly.

github-actions · 2023-03-13T21:17:52Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.4 %
main branch coverage: 60.3 %

youngbupark · 2023-03-14T15:58:12Z

pkg/cli/helm/radiusclient.go

 	err = addRadiusValues(helmChart, &options)
 	if err != nil {
 		return false, fmt.Errorf("failed to add radius values, err: %w, helm output: %s", err, helmOutput.String())
 	}

+	if options.Values != "" {


I think empty string should be handled by ParseInto. so we do not need this if else.

youngbupark · 2023-03-14T16:07:45Z

pkg/cli/helm/radiusclient.go

 	err = addRadiusValues(helmChart, &options)
 	if err != nil {
 		return false, fmt.Errorf("failed to add radius values, err: %w, helm output: %s", err, helmOutput.String())
 	}

+	if options.Values != "" {
+		err := strvals.ParseInto(options.Values, helmChart.Values)


isn't it overriding the values from L78 ?

it would overwrite if the keys match, for example if someone specified a set with globar.rp.container. I thought eventually when we refactor helm in next sprint, we would remove addRadiusValues and just document --set usage to set different values.
So I had added a TODO comment at L77. Should I add checks to make sure we arent using set for thos eparameters alone?

#5284 created to use this function for functionalities of --tag, --image etc

ok. then I would move this strvals.ParseInto to the end of the existing addRadiusValues function and have the unit-test for addRadiusValues.

github-actions · 2023-03-16T18:50:25Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.3 %
main branch coverage: 60.3 %

github-actions · 2023-03-16T19:48:04Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.6 %
main branch coverage: 60.3 %

youngbupark · 2023-03-17T00:30:35Z

@nithyatsu Thanks! You increased 0.3 % of our test coverage!

youngbupark

It increased 0.3% coverage!!!

Thanks for this! I left two comments. Please resolve them before merge :)

youngbupark · 2023-03-17T00:41:21Z

pkg/cli/helm/radiusclient.go

@@ -284,6 +287,13 @@ func addRadiusValues(helmChart *chart.Chart, options *RadiusOptions) error {
 		de["tag"] = options.DETag
 	}

+	if options.Values != "" {


ParseInfo takes care of empty string. So we do not need this check.

youngbupark · 2023-03-17T00:41:43Z

pkg/cli/helm/radiusclient_test.go

+
+func Test_AddRadiusValues(t *testing.T) {
+	var helmChart chart.Chart
+	helmChart.Values = map[string]interface{}{}


Suggested change

helmChart.Values = map[string]interface{}{}

helmChart.Values = map[string]any{}

github-actions · 2023-03-20T17:17:55Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.5 %
main branch coverage: 60.3 %
diff coverage: .2 %

rynowak · 2023-03-20T18:39:53Z

😍 really glad to see this getting worked on

rynowak · 2023-03-20T18:42:25Z

@nithyatsu

Can you please link the issue?
If there was a design review, can you make sure the relevant details got copied into the issue?
If there wasn't a design review, please get one queued up. We should be doing design reviews for CLI UX changes.

rynowak · 2023-03-20T18:44:35Z

I'm confused about something.... the title says introduce --set and then the commit message says --args. But I don't see either of these things in the PR.

nithyatsu · 2023-03-20T18:50:51Z

Missed updating description... Should be OK now. Also added the github user stopry that gets partially fixed by this command.

rynowak · 2023-03-20T19:01:39Z

Thanks 👍 - we need to get in the habit of including this info for when we're open source.

This reverts commit 4122725.

This reverts commit 9b7bce4.

This reverts commit d5c719c.

This reverts commit bb03870.

rynowak · 2023-03-20T19:06:38Z

pkg/cli/setup/chart.go

@@ -37,6 +40,7 @@ func RegisterPersistentChartArgs(cmd *cobra.Command) {
 	cmd.PersistentFlags().String("ucp-image", "", "Specify the UCP image to use")
 	cmd.PersistentFlags().String("ucp-tag", "", "Specify the UCP tag to use")
 	cmd.PersistentFlags().String("public-endpoint-override", "", "Specify the public IP address or hostname of the Kubernetes cluster. It must be in the format: <hostname>[:<port>]. Ex: 'localhost:9000'")
+	cmd.PersistentFlags().String("set", "", "Set values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)")


Can you add an example of this to rad install kubernetes?

rynowak · 2023-03-20T19:07:16Z

You're also doing #3937

github-actions · 2023-03-20T19:11:37Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.5 %
main branch coverage: 60.3 %
diff coverage: .2 %

nithyatsu · 2023-03-20T19:21:12Z

For tracer feature mvp, I am templatizing config map and exposing tracer url as a templatized value. This is what I am doing as part of https://dev.azure.com/azure-octo/Incubations/_workitems/edit/6520 . Perhaps I should have a discussion around what else should be available as template?

rynowak · 2023-03-20T21:24:24Z

But you did implement #3937 - this is a new CLI feature to pass values to the helm chart during installation.

nithyatsu changed the title ~~initial commit~~ support rad install kubernetes --set values Mar 7, 2023

ytimocin reviewed Mar 8, 2023

View reviewed changes

nithyatsu marked this pull request as ready for review March 8, 2023 21:26

nithyatsu requested a review from a team as a code owner March 8, 2023 21:26

nithyatsu requested a review from youngbupark March 8, 2023 21:27

youngbupark reviewed Mar 8, 2023

View reviewed changes

vinayada1 reviewed Mar 10, 2023

View reviewed changes

nithyatsu requested review from vinayada1 and youngbupark March 13, 2023 15:00

youngbupark reviewed Mar 13, 2023

View reviewed changes

nithyatsu requested a review from youngbupark March 13, 2023 22:25

youngbupark reviewed Mar 14, 2023

View reviewed changes

nithyatsu requested a review from youngbupark March 14, 2023 18:39

nithyatsu force-pushed the nithya/chartargs branch from 0af3ea8 to d3747eb Compare March 16, 2023 19:39

youngbupark approved these changes Mar 17, 2023

View reviewed changes

nithyatsu added 20 commits March 20, 2023 12:03

initial commit

4951e15

wip

fbf620b

wip

3983f94

add zipkin endpoint

a894fb1

add comemnt

d7fe7e0

Revert "add comemnt"

1615ce5

This reverts commit 4122725.

Revert "add zipkin endpoint"

0fbc6fe

This reverts commit 9b7bce4.

Revert "wip"

acf8975

This reverts commit d5c719c.

Revert "wip"

a816412

This reverts commit bb03870.

improve isEmpty

b1f9284

nit

886b957

fix map

c528428

add comment

1558906

wip

bf562fc

wip

713c052

wip

57050da

wip

0e4bd1e

nit

d36545c

add unit test

5bc09ac

wip

ccdf87f

nithyatsu force-pushed the nithya/chartargs branch from 73d22d3 to ccdf87f Compare March 20, 2023 19:04

rynowak reviewed Mar 20, 2023

View reviewed changes

nithyatsu merged commit 5918d73 into main Mar 20, 2023

nithyatsu deleted the nithya/chartargs branch March 20, 2023 20:17

		# url: "http://jaeger-collector.radius-monitoring.svc.cluster.local:9411/api/v2/spans" No newline at end of file
		#url: "http://jaeger-collector.radius-monitoring.svc.cluster.local:9411/api/v2/spans"

		@@ -194,6 +207,35 @@ func GetAzProvider(options RadiusOptions, kubeContext string) (*azure.Provider,

		}

		func GetHelmValues(kubeContext string) (map[string]interface{}, error) {

	helmChart.Values = map[string]interface{}{}
	helmChart.Values = map[string]any{}

support rad install kubernetes --set values #5243

support rad install kubernetes --set values #5243

Conversation

nithyatsu commented Mar 7, 2023 • edited Loading

Description

Issue reference

Checklist

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithyatsu Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Mar 8, 2023

github-actions bot commented Mar 8, 2023

github-actions bot commented Mar 8, 2023

youngbupark left a 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

github-actions bot commented Mar 8, 2023

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

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

github-actions bot commented Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 13, 2023

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

github-actions bot commented Mar 16, 2023

github-actions bot commented Mar 16, 2023

youngbupark commented Mar 17, 2023

youngbupark left a comment

Choose a reason for hiding this comment

It increased 0.3% coverage!!!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 20, 2023

rynowak commented Mar 20, 2023

rynowak commented Mar 20, 2023

rynowak commented Mar 20, 2023

nithyatsu commented Mar 20, 2023

rynowak commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented Mar 20, 2023

github-actions bot commented Mar 20, 2023

nithyatsu commented Mar 20, 2023

rynowak commented Mar 20, 2023

nithyatsu commented Mar 7, 2023 •

edited

Loading

nithyatsu Mar 8, 2023 •

edited

Loading