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

prevent assignment of duplicate labels to agents #3042

Merged
merged 1 commit into from
Jul 28, 2020
Merged

prevent assignment of duplicate labels to agents #3042

merged 1 commit into from
Jul 28, 2020

Conversation

jameslamb
Copy link

  • adds new tests (if appropriate)
  • add a changelog entry in the changes/ directory (if appropriate)
  • updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

What does this PR change?

This PR attempts to prevent the situation where an agent has duplicate labels.

Why is this PR important?

While testing #3000 , I noticed something surprising...it's possible for duplicates to exist in an agent's set of labels:

For example:

prefect agent start \
    -t ${PREFECT_RUNNER_TOKEN} \
    --label 'i-am-duplicated' \
    --label 'i-am-duplicated' \
    --label 'i-am-duplicated'

image

image

I'm not sure if this would cause any problems today, but I could see it causing these types of problems:

  • breaking code that allows wildcards for matching flows to agents, but throws an error if more than one label is matched
  • breaking code that queries for agents by label and cannot handle duplicates
  • slightly hurting the experience in the Prefect Cloud UI if duplicate labels are enough to push other labels into the collapsed +n box like in the screenshot above

So, I'm not aware of anything that is not working well today because of this, but I think this change might be a minor way to prevent some weird cases in the future.

Thanks for your time and consideration.

Copy link

@joshmeek joshmeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change won't affect anything but pinging @zdhughes if he knows what would happen anyway when using duplicate labels. e.g. is a flow and agent with labels A,A different than a flow and agent only with the label A 🤔

@zdhughes
Copy link

Just confirmed-- label matching for run retrieval is done by way of sets, so the list ["foo", "foo"] gets collapsed down to the set {"foo"} on the API side. It looks like the same is not true for label-based flow run concurrency, but I think that's likely an oversight on the Cloud side. 👍

@joshmeek
Copy link

@zdhughes Awesome thanks for clarifying! Since this is only agent related it's good to go 👍

@joshmeek joshmeek merged commit 8e4db16 into PrefectHQ:master Jul 28, 2020
@jameslamb jameslamb deleted the fix/duplicate-labels branch September 15, 2020 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants