Turning on strictMode for propeller and manager config

[pmahindrakar-oss]

Signed-off-by: pmahindrakar-oss [email protected]

TL;DR

Type

• Bug Fix
• Feature
• Plugin

Are all requirements met?

• Code completed
• Smoke tested
• Unit tests added
• Code documentation added
• Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

fixes flyteorg/flyte#3113

Follow-up issue

NA

[codecov]

Codecov Report

Merging #505 (036bf04) into master (9c9918b) will not change coverage.
The diff coverage is n/a.

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

[hamersaw]

@pmahindrakar-oss I do not see StrictMode set at all in FlyteAdmin and on datacatalog it is set similarly (ie. not on initConfig). Is this something we need to take care of in those repos as well?

[pmahindrakar-oss]

yes i agree @hamersaw. I was trying to address the one where i saw this initially . But i wanted to understand the reason we don't have this turned on by default for all flyte repos.

By definition

	// Instructs parser to fail if any section/key in the config file read do not have a corresponding registered section.
	StrictMode bool

The way this was discovered was flytepropeller accepted a config like this one though the key config doesn't exist in structure and it continued processing with the invalid log config

plugins:
  # Logging configuration
  logs:
    config:
      kubernetes-enabled: true
      kubernetes-url: "http://localhost:30082"

So turning on this helps as it will immediately throw an error once we turn on strictMode to show something like this has invalid keys: config .

This seems to be right error to catch earlier than to find issues with running flyte instances. The reason we could want non-strict behavior i assume is in case of maintaining backward compatible configs in which we delete deprecated keys in the structure but we expect to support reading older configs without erroring out which i find error prone too or i might be missing something.

[hamersaw]

@pmahindrakar-oss had a chance to take a deeper look into this. I can see the benefit but I am concerned that this will break existing deployments in a number of scenarios. For example, propeller manager requires additional configuration. This is currently added to the propeller configmap. In this case StrictMode will result in propeller instances failing to load because there is a manager configuration section which is not included in the configuration. Another example is single binary where all Flyte configuration is in a single file - if we require StrictMode there it will be unable to start.

I think it would be VERY nice to have this feature in all of our repos, but I can't see a way to enable it with backwards compatibility. Do you have any ideas?

[pmahindrakar-oss]

@hamersaw i see your point. I think flyteadmin does deal with similar issue but it can handle this problem since it treats flyteadmin configuration as one single entity and scheduler, clusterresourcecontroller etc as subconfigurations.

So when flyteadmin, scheduler or clusterresourcecontroller loads the configurations , it loads the entire flyteadmin config for the app even though it might just use a section of it.

• flyteadmin-serve: https://github.com/flyteorg/flyteadmin/blob/master/cmd/entrypoints/serve.go#L29
• clusterresourcecotnroller : https://github.com/flyteorg/flyteadmin/blob/master/cmd/entrypoints/clusterresource.go#L28
• scheduler : https://github.com/flyteorg/flyteadmin/blob/master/cmd/scheduler/entrypoints/scheduler.go#L21

So if we turn on strictMode on all the components will still continue to function correctly
But i see propeller manager doesn't do this https://github.com/flyteorg/flytepropeller/blob/master/cmd/manager/cmd/root.go#L85 and uses two independent config structs even though they are being loaded from the same config file.

I think if we define the way flyteadmin does it then that should work and also be backward compatible . Let me know what you think.

[hamersaw]

I think if we define the way flyteadmin does it then that should work and also be backward compatible . Let me know what you think.

This won't work with single binary though right? Because we have the configuration for all of the components in a single file.

[pmahindrakar-oss]

yeah you are right @hamersaw that we wont be able to change for single binary but we can keep loading the config in non strict mode for it https://github.com/flyteorg/flyte/blob/master/cmd/single/root.go#L67

The current change will only affect propeller when its run in an independent mode