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.

Sign up for GitHub

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

Jump to bottom

Invalid names being generated for Azure Container Instance runs #13708

Open
4 tasks done
bjorhn opened this issue May 31, 2024 · 1 comment · May be fixed by #14333
Open
4 tasks done

Invalid names being generated for Azure Container Instance runs #13708

bjorhn opened this issue May 31, 2024 · 1 comment · May be fixed by #14333
Labels
bug Something isn't working integrations Related to integrations with other services

Comments

@bjorhn
Copy link

bjorhn commented May 31, 2024 •
edited
Loading

First check

  • I added a descriptive title to this issue.
  • I used the GitHub search to find a similar issue and didn't find it.
  • I searched the Prefect documentation for this issue.
  • I checked that this issue is related to Prefect and not one of its dependencies.

Bug summary

There's something wrong with the logic in container_instance.py for workers. I can use job_variables to override the name for the azure container instances container group that should be created in Azure, using the "name" variable, but only if the name of the flow itself is over a certain character length. If it's under that character length, the "name" variable is ignored and Prefect attempts to create the Azure resource using the flow name instead. This doesn't work, since the flow name, in my case, contains invalid characters such as spaces.

I have tried this with the following two flows:
Keyvault Notification (with the name in job_variables set to "keyvault-notification" - this works
Refresh PBI (with the name in job_variables set to "refresh-pbi" - this doesn't work

If I change the name of the Refresh PBI flow to Refresh PBIWorkerTest, which is the same character length as Keyvault Notification, everything works as expected. Everything also works as expected when using agents instead of workers.

Reproduction

Don't have an MRE at the moment, sorry.

Error

The provided deployment name 'prefect-Refresh PBI-77de0178-0dc6-4c17-a1fe-3c79cb0a7528' has these invalid characters: ' '

Versions

2.19.2

Additional context

No response
@bjorhn bjorhn added bug Something isn't working needs:triage Needs feedback from the Prefect product team labels May 31, 2024
@bjorhn bjorhn changed the title Invalid names being generated for Azure Container Instances runs Invalid names being generated for Azure Container Instance runs May 31, 2024
@bjorhn
Copy link
Author

bjorhn commented Jun 15, 2024 •
edited
Loading

I compared the code for the old ACI agents with the worker code and I've found the source of the problem:

  • The name job variable isn't used for workers, it's ignored. This means it's impossible to set a custom name for the aci container group, it will always be based on the name of the flow.
  • The slugify function is only called for flow names over a certain length. This is what causes flows with longer names to work, since any spaces and other invalid characters are replaced. If the flow name is short, slugify is never called. This results in short flow names with spaces in them not being compatible with the ACI workers.

I think this could be dealt with in two ways. Either the name job variable should be used instead of the flow name, like how the agent implemented it, or the slugify code path should be used regardless of how long the flow name is. The latter should be easier to implement, but will result in us remaining unable to manually set the name of the container group separately from the flow name.

@desertaxle desertaxle added integrations Related to integrations with other services and removed needs:triage Needs feedback from the Prefect product team labels Jun 19, 2024
@bjorhn bjorhn linked a pull request Jun 25, 2024 that will close this issue
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working integrations Related to integrations with other services
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Made Azure ACI infra names consistent regardless of flow name length bjorhn/prefect
2 participants
@bjorhn @desertaxle