Set flyteadmin grpc port correctly in config / Flyte-core flyteadmin / datacatalog expose ports

[ddl-ebrown]

Tracking issue

https://github.com/flyteorg/flyte/issues/

Why are the changes needed?

• Flyteadmin setting for the grpc port is nested and grpcPort should
  not be used. Address incorrect config / documentation

• Both flyteadmin and datacatalog have configurable ports to use for
  HTTP and GRPC, but the k8s spec for the containers doesn't expose
  matching ports.

  Fix that!

What changes were proposed in this pull request?

Correctly use the values from values.yaml inside the k8s specs for listening container ports

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

Related PRs

Docs link

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.99%. Comparing base (b6f35ad) to head (53b70b7).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5013   +/-   ##
=======================================
  Coverage   58.99%   58.99%           
=======================================
  Files         645      645           
  Lines       55648    55648           
=======================================
  Hits        32831    32831           
  Misses      20222    20222           
  Partials     2595     2595           

Flag	Coverage Δ
unittests	58.99% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ddl-ebrown]

Moved back to draf for the moment - given what the config definition actually looks like.

flyte/flyteadmin/pkg/config/config.go

Lines 17 to 54 in fd42f65

	type ServerConfig struct {
	HTTPPort int `json:"httpPort" pflag:",On which http port to serve admin"`
	GrpcPort int `json:"grpcPort" pflag:",deprecated"`
	GrpcServerReflection bool `json:"grpcServerReflection" pflag:",deprecated"`
	KubeConfig string `json:"kube-config" pflag:",Path to kubernetes client config file, default is empty, useful for incluster config."`
	Master string `json:"master" pflag:",The address of the Kubernetes API server."`
	Security ServerSecurityOptions `json:"security"`
	GrpcConfig GrpcConfig `json:"grpc"`
	// Deprecated: please use auth.AppAuth.ThirdPartyConfig instead.
	DeprecatedThirdPartyConfig authConfig.ThirdPartyConfigOptions `json:"thirdPartyConfig" pflag:",Deprecated please use auth.appAuth.thirdPartyConfig instead."`
	DataProxy DataProxyConfig `json:"dataProxy" pflag:",Defines data proxy configuration."`
	ReadHeaderTimeoutSeconds int `json:"readHeaderTimeoutSeconds" pflag:",The amount of time allowed to read request headers."`
	KubeClientConfig KubeClientConfig `json:"kubeClientConfig" pflag:",Configuration to control the Kubernetes client"`
	}
	type DataProxyConfig struct {
	Upload DataProxyUploadConfig `json:"upload" pflag:",Defines data proxy upload configuration."`
	Download DataProxyDownloadConfig `json:"download" pflag:",Defines data proxy download configuration."`
	}
	type DataProxyDownloadConfig struct {
	MaxExpiresIn config.Duration `json:"maxExpiresIn" pflag:",Maximum allowed expiration duration."`
	}
	type DataProxyUploadConfig struct {
	MaxSize resource.Quantity `json:"maxSize" pflag:",Maximum allowed upload size."`
	MaxExpiresIn config.Duration `json:"maxExpiresIn" pflag:",Maximum allowed expiration duration."`
	DefaultFileNameLength int `json:"defaultFileNameLength" pflag:",Default length for the generated file name if not provided in the request."`
	StoragePrefix string `json:"storagePrefix" pflag:",Storage prefix to use for all upload requests."`
	}
	type GrpcConfig struct {
	Port int `json:"port" pflag:",On which grpc port to serve admin"`
	ServerReflection bool `json:"serverReflection" pflag:",Enable GRPC Server Reflection"`
	MaxMessageSizeBytes int `json:"maxMessageSizeBytes" pflag:",The max size in bytes for incoming gRPC messages"`
	EnableGrpcLatencyMetrics bool `json:"enableGrpcLatencyMetrics" pflag:",Enable grpc latency metrics. Note Histograms metrics can be expensive on Prometheus servers."`
	}

I think it should be more like .grpc.port instead of .grpcPort - verifying in my environment.

[ddl-ebrown]

Confirmed that the setting should be .grpc.port and not .grpcPort. I fixed all the locations where the old value was being set and regenerated all the Helm stuff, etc.

There were 4 files that I had to edit by hand that look like they should be generated, but I couldn't figure out the process for generating them:

• deployment/eks/flyte_generated.yaml
• deployment/gcp/flyte_generated.yaml
• deployment/sandbox/flyte_generated.yaml
• deployment/test/flyte_generated.yaml

[ddl-ebrown]

This corresponds to https://github.com/flyteorg/flyte/blob/master/flyteadmin/pkg/config/config.go#L24

[ddl-ebrown]

Not sure how this file is regenerated, so edited by hand

[ddl-ebrown]

Not sure how this file is regenerated, so edited by hand

[ddl-ebrown]

Not sure how this file is regenerated, so edited by hand

[ddl-ebrown]

Not sure how this file is regenerated, so edited by hand

[davidmirror-ops]

@ddl-ebrown I think running make helm from the base of your fork should re generate those files. The you can commit the changes and the kustomize test should pass

[ddl-ebrown]

@ddl-ebrown I think running make helm from the base of your fork should re generate those files. The you can commit the changes and the kustomize test should pass

I always run make helm when submitting charts PRs -- which is where many of these updates came from. make helm does not produce these files though:

• deployment/eks/flyte_generated.yaml
• deployment/gcp/flyte_generated.yaml
• deployment/sandbox/flyte_generated.yaml
• deployment/test/flyte_generated.yaml

UPDATE: After rebasing and regenerating, I see that those first 3 files are no longer part of the repo. So the only unexplained file is deployment/test/flyte_generated.yaml

[ddl-ebrown]

Looks like there's a new process for bumping the chart version number?

[davidmirror-ops]

@ddl-ebrown with the merge of #5072 you should rebase and merge master to pass the lint test.
Thank you

[ddl-ebrown]

@davidmirror-ops rebased and repushed - hopefully we're good to get this one merged.

[davidmirror-ops]

Thank you!