Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Partially configured task resource configuration can cause nil pointers and crash Flyte Admin #5977

Open
2 tasks done
Sovietaced opened this issue Nov 8, 2024 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@Sovietaced
Copy link
Contributor

Describe the bug

A task resource configuration that is not completely filled out will cause nil pointers in Flyte Admin because the code assumes every fields is present and complete.

runtime/debug.Stack()
	/usr/local/go/src/runtime/debug/stack.go:24 +0x5e
github.com/flyteorg/flyte/flyteadmin/pkg/rpc/adminservice.(*AdminService).interceptPanic(0xc00215e000, {0x330c7a8, 0xc004784e70}, {0x33004b0?, 0xc00453cc40})
	/go/src/github.com/flyteorg/flyteadmin/pkg/rpc/adminservice/base.go:55 +0x7a
panic({0x27d3a40?, 0x4b0b9c0?})
	/usr/local/go/src/runtime/panic.go:914 +0x21f
github.com/flyteorg/flyte/flyteadmin/pkg/manager/impl/util.fromAdminProtoTaskResourceSpec({0x330c7a8, 0xc004784e70}, _)
	/go/src/github.com/flyteorg/flyteadmin/pkg/manager/impl/util/resources.go:61 +0x73
github.com/flyteorg/flyte/flyteadmin/pkg/manager/impl/util.GetTaskResources({_, _}, _, {_, _}, {_, _})
	/go/src/github.com/flyteorg/flyteadmin/pkg/manager/impl/util/resources.go:108 +0x3c5
github.com/flyteorg/flyte/flyteadmin/pkg/manager/impl.(*TaskManager).CreateTask(0xc000b727e0, {0x330c7a8, 0xc004784e70}, {{{}, {}, {}, 0xc000273000}, 0x0, {0x0, 0x0, ...}, ...})
	/go/src/github.com/flyteorg/flyteadmin/pkg/manager/impl/task_manager.go:64 +0x7f
github.com/flyteorg/flyte/flyteadmin/pkg/rpc/adminservice.(*AdminService).CreateTask.func1()
	/go/src/github.com/flyteorg/flyteadmin/pkg/rpc/adminservice/task.go:25 +0x9b
github.com/flyteorg/flyte/flytestdlib/promutils.StopWatch.Time({{0x7fd8f7388b40?, 0xc001c91e00?}, 0x280d800?}, 0xc001b28170)
	/go/src/github.com/flyteorg/flytestdlib/promutils/scope.go:58 +0xb6
github.com/flyteorg/flyte/flyteadmin/pkg/rpc/adminservice/util.(*RequestMetrics).Time(...)
	/go/src/github.com/flyteorg/flyteadmin/pkg/rpc/adminservice/util/metrics.go:33
github.com/flyteorg/flyte/flyteadmin/pkg/rpc/adminservice.(*AdminService).CreateTask(0xc00215e000, {0x330c7a8?, 0xc004784e70?}, 0x2c1bea0?)
	/go/src/github.com/flyteorg/flyteadmin/pkg/rpc/adminservice/task.go:24 +0x158

Expected behavior

The expected behavior is that Flyte Admin does not throw a nil pointer if only defaults or limits are specified in a task resource configuration.

One of the questions that needs to be answered is what is a valid task resource configuration? Once that is understood we can add some validation to the API which accepts this configuration.

Additional context to reproduce

project: flyteexamples
domain: development
limits:
    cpu: "2"
    memory: 450Mi

Screenshots

No response

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@Sovietaced Sovietaced added bug Something isn't working untriaged This issues has not yet been looked at by the Maintainers labels Nov 8, 2024
@Sovietaced Sovietaced self-assigned this Nov 8, 2024
@eapolinario eapolinario self-assigned this Nov 21, 2024
@eapolinario eapolinario removed the untriaged This issues has not yet been looked at by the Maintainers label Nov 21, 2024
@eapolinario
Copy link
Contributor

@Sovietaced , after #5981 you shouldn't be seeing a panic anymore. This question of whether these incomplete task resource configuration objects should be considered invalid is still pertinent, but can be tackled separately, right?

@Sovietaced
Copy link
Contributor Author

@Sovietaced , after #5981 you shouldn't be seeing a panic anymore. This question of whether these incomplete task resource configuration objects should be considered invalid is still pertinent, but can be tackled separately, right?

I took a look at that PR but I don't think it will solve the issue where GetDefaults() or GetLimits() is nil

https://github.com/flyteorg/flyte/pull/5981/files#diff-36a9a91abc6f8873fbb6ca3e82f73a10f0cf99b69812a2715ea237b27bac1652R108-R109

@eapolinario
Copy link
Contributor

github is not cooperating, https://github.com/flyteorg/flyte/pull/5981/files#diff-36a9a91abc6f8873fbb6ca3e82f73a10f0cf99b69812a2715ea237b27bac1652R108-R109 doesn't render anything for me. Can you link to the file directly?

@Sovietaced
Copy link
Contributor Author

github is not cooperating, https://github.com/flyteorg/flyte/pull/5981/files#diff-36a9a91abc6f8873fbb6ca3e82f73a10f0cf99b69812a2715ea237b27bac1652R108-R109 doesn't render anything for me. Can you link to the file directly?

One of these two can still be nil it seems. https://github.com/flyteorg/flyte/blob/master/flyteadmin/pkg/manager/impl/util/resources.go#L108-L109

I can upstream a change I had.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Backlog
Development

No branches or pull requests

2 participants