[WIP] fix: moving the hardcoded configs in KFPv2 Launcher to configmap. Fixes #494 #191

amadhusu · 2024-01-03T09:58:58Z

Which issue is resolved by this Pull Request:
Resolves #494

Description of your changes:
Followed Option 1 in the suggestion mentioned in kubeflow/pipelines/issues/9689

We want to allow the above configs to be changed from their defaults, preferably on a per-namespace basis.

I propose we add these configs to the ConfigMap/kfp-launcher which already exists in each profile namespace to set defaultPipelineRoot.

For example, a new ConfigMap/kfp-launcher in a profile namespace might look like this:
data:
  defaultPipelineRoot: "minio://mlpipeline/v2/artifacts"
 
  ## minio endpoint config
  defaultMinioEndpoint: "minio.example.com:9000"
 
  ## minio auth configs
  minioAuthConfig: "mlpipeline-minio-artifact"
  minioAuthConfigAccessKey: "access_key"
  minioAuthConfigSecretKey: "access_key"
OPTION 1: have the kfp-launcher container read the ConfigMap/kfp-launcher
when it executes, it can pass the minio configs down as it calls objectstore.OpenBucket

Environment tested:

Checklist:

The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

Signed-off-by: Achyut Madhusudan <amadhusu@redhat.com>

openshift-ci · 2024-01-03T09:59:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amadhusu

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

Needs approval from an approver in each of these files:

OWNERS

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

HumairAK · 2024-01-04T21:19:41Z

backend/src/v2/config/env.go

+	configMapName                  = "kfp-launcher"
+	defaultPipelineRoot            = "minio://mlpipeline/v2/artifacts"
+	configKeyDefaultPipelineRoot   = "defaultPipelineRoot"
+	minioAuthConfig                = "mlpipeline-minio-artifact"
+	configKeyMinioAuthConfig       = "minioAuthConfig"
+	minioAuthConfigAccessKey       = "accesskey"
+	configMinioAuthConfigAccessKey = "minioAuthConfigAccessKey"
+	minioAuthConfigSecretKey       = "secretkey"
+	configMinioAuthConfigSecretKey = "minioAuthConfigSecretKey"
+	defaultMinioEndpoint           = "minio-service.kubeflow:9000"
+	configDefaultMinioEndpoint     = "defaultMinioEndpoint"


I'm finding the naming here to be very hard to digest, can you organize this differently

group the configs together, space them apart, etc.

note also that minioAuthConfigAccessKey and minioAuthConfigSecretKey consider what they are being used for, for example: defaultMinioSecretAccessKeyKey and defaultMinioSecretSecretKeyKey, this may look silly with the double usage of "secret" and "key", but it's more descriptive, as these are fields found in a k8s secret.

minioAuthConfig can be better named (I know this one comes from upstream but it's confusing), it's a default k8s secret name, something like defaultMiniotAuthSecretName is way more helpful

@HumairAK See if the changes here are easier to read now

HumairAK · 2024-01-04T21:20:58Z

backend/src/v2/config/env.go

+	if c == nil || c.data[configKeyMinioAuthConfig] == "" {
+		return minioAuthConfig, minioAuthConfigAccessKey, minioAuthConfigSecretKey
+	}
+	return c.data[configKeyMinioAuthConfig], c.data[configMinioAuthConfigAccessKey], c.data[configMinioAuthConfigSecretKey]


what if these fields are not present?

c.data[configMinioAuthConfigAccessKey]

c.data[configMinioAuthConfigSecretKey]

HumairAK · 2024-01-04T21:21:45Z

backend/src/v2/objectstore/object_store.go

+
+	accessKey := minioAuthConfigAccessKey
+	secretKey := minioAuthConfigSecretKey


this is not fetching the creds from the secret anymore , we need to retain the original behavior if the credentials aren't specified in a config

HumairAK · 2024-01-04T21:22:42Z

backend/src/v2/objectstore/object_store.go

+		cfg, err := config.FromConfigMap(ctx, k8sClient, namespace)
+		if err != nil {
+			return nil, err
+		}


in the original issue the recommendation was to do this in Execute within the launcher, and pass the creds down to OpenBucket, I agree with this approach more because "FromConfigMap" is a kfp-launcher concern, not object store, so we should not be reading kfp-launcher configs within the objectstore package.

HumairAK · 2024-01-04T21:28:11Z

backend/src/v2/objectstore/object_store.go

+			return nil, err
+		}
+		_, minioAuthConfigAccessKey, minioAuthConfigSecretKey := cfg.MinioAuthConfig()
+		defaultMinioEndpointInMultiUserMode := cfg.MinioEndpoint()


The double calls for the same thing seem weird to me for retrieving the minio endpoint, how about we just add a field to the Config struct at the top called endpoint, in kfp-launcher execute fetch it via MinioEndpoint(), in OpenBucket, check fo this field in bucketconfig if it doesn't exist, default to old behavior

Nevermind, I'm not sure about this one, adding fields in a generic way to the config struct is not completely clear, there is tight integration with minio but there is still "some" support for other storage providers (see ParseBucketConfig), but there doesn't seem to be a way to provide credentials for them, so I'm not sure if it makes sense to add things like "Region" (needed by aws s3 for example) or not to that struct.

Signed-off-by: Achyut Madhusudan <amadhusu@redhat.com>

gregsheremeta · 2024-01-11T16:51:57Z

~~Migrated to Jira: https://issues.redhat.com/browse/RHOAIENG-1628~~

sorry bad robot

amadhusu · 2024-01-12T17:42:48Z

Closing this PR as it has been moved to here

RichardPinter · 2024-05-30T23:17:49Z

Has this been resolved? I can connect to my external minio to write manifects, but for some logs it still tries to write to the internal minio?
ailed to execute component: failed to upload output artifact "data_out" to remote storage URI "minio://mlpipeline/v2/artifacts/xxxxx/data_out": uploadFile(): unable to complete copying "/minio/mlpipeline/v2/artifacts/xxxxx/gen-sine/data_out" to remote storage "gen-sine/data_out": failed to close Writer for bucket: blob (key "gen-sine/data_out") (code=NotFound): NoSuchBucket: The specified bucket does not exist

Moving the hardcoded configs in KFPv2 Launcher to configmap.

e19e5c2

Signed-off-by: Achyut Madhusudan <amadhusu@redhat.com>

openshift-ci bot added the do-not-merge/work-in-progress label Jan 3, 2024

amadhusu requested a review from HumairAK January 3, 2024 09:59

HumairAK requested changes Jan 4, 2024

View reviewed changes

openshift-ci bot assigned HumairAK Jan 4, 2024

amadhusu requested a review from HumairAK January 10, 2024 14:16

Moved the code within launcher to pass the creds down to object store.

1760331

Signed-off-by: Achyut Madhusudan <amadhusu@redhat.com>

amadhusu force-pushed the am/494 branch from 168833c to 1760331 Compare January 10, 2024 16:29

gregsheremeta closed this Jan 11, 2024

gregsheremeta reopened this Jan 11, 2024

amadhusu closed this Jan 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] fix: moving the hardcoded configs in KFPv2 Launcher to configmap. Fixes #494 #191

[WIP] fix: moving the hardcoded configs in KFPv2 Launcher to configmap. Fixes #494 #191

amadhusu commented Jan 3, 2024

openshift-ci bot commented Jan 3, 2024

HumairAK Jan 4, 2024

amadhusu Jan 10, 2024

HumairAK Jan 4, 2024

HumairAK Jan 4, 2024

HumairAK Jan 4, 2024

HumairAK Jan 4, 2024 •

edited

Loading

HumairAK Jan 5, 2024

gregsheremeta commented Jan 11, 2024 •

edited

Loading

amadhusu commented Jan 12, 2024

RichardPinter commented May 30, 2024


		accessKey := minioAuthConfigAccessKey
		secretKey := minioAuthConfigSecretKey

[WIP] fix: moving the hardcoded configs in KFPv2 Launcher to configmap. Fixes #494 #191

[WIP] fix: moving the hardcoded configs in KFPv2 Launcher to configmap. Fixes #494 #191

Conversation

amadhusu commented Jan 3, 2024

openshift-ci bot commented Jan 3, 2024

HumairAK Jan 4, 2024

Choose a reason for hiding this comment

amadhusu Jan 10, 2024

Choose a reason for hiding this comment

HumairAK Jan 4, 2024

Choose a reason for hiding this comment

HumairAK Jan 4, 2024

Choose a reason for hiding this comment

HumairAK Jan 4, 2024

Choose a reason for hiding this comment

HumairAK Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

HumairAK Jan 5, 2024

Choose a reason for hiding this comment

gregsheremeta commented Jan 11, 2024 • edited Loading

amadhusu commented Jan 12, 2024

RichardPinter commented May 30, 2024

HumairAK Jan 4, 2024 •

edited

Loading

gregsheremeta commented Jan 11, 2024 •

edited

Loading