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

Allow empty services #10375

Merged

Conversation

chrispruitt
Copy link
Contributor

What does this PR do?

This enables the nomad provider the ability to allow empty services. For instance, when a service is scaled down to 0 the route will persist. This would be an opt-in feature (false by default).
Fixes #10374

Motivation

I currently have services in Nomad that scaled down to zero and use a port-knocking solution that scales the service up when the route receives traffic. Also, the Kubernetes provider already has this option, so I felt like it was missing from the nomad provider.

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

I attempted to implement this just like the Kubernetes provider for consistency.
Also, to make this possible, I had to utilize Nomad's job API methods, rather than the service API methods to retrieve the list of existing services. This is because Nomad does not currently list nonregistered services (or services scaled to 0) with service API methods.

@traefiker traefiker added this to the 3.0 milestone Jan 22, 2024
@chrispruitt chrispruitt force-pushed the feat/allowEmptyServices-nomad-provider branch 2 times, most recently from 5100ef2 to 1e15586 Compare January 24, 2024 20:28
@nmengin nmengin added the priority/P2 need to be fixed in the future label Jan 25, 2024
@chrispruitt chrispruitt force-pushed the feat/allowEmptyServices-nomad-provider branch from 7555391 to 3bfac8f Compare January 25, 2024 20:01
Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this contribution @chrispruitt

Few comments below

pkg/provider/nomad/nomad_test.go Outdated Show resolved Hide resolved
pkg/provider/nomad/nomad.go Outdated Show resolved Hide resolved
@mmatur
Copy link
Member

mmatur commented Feb 15, 2024

Hi @chrispruitt

Thanks for your changes. Seems good to me regarding the feature.

I am one more question what do you think to use a mock instead of creating these fixtures files for test?

@chrispruitt
Copy link
Contributor Author

@mmatur I just followed the same process this provider package already had in place. The only change I made was moving the JSON responses to files. I don't have a strong opinion either way. They do end up with the same result in my opinion and the fixtures approach is easy to track mocked responses IMO.

On another note, do I need to rebase this branch? Or would you do that before merging? Thanks!

@mmatur mmatur force-pushed the feat/allowEmptyServices-nomad-provider branch from f832ddf to 0ddf789 Compare February 15, 2024 19:41
Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

Thanks for your feedback @chrispruitt .

Let's move forward with that and see what other maintainers think about it

I gave it a try and it seems good to me

LGTM

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

Thank you @chrispruitt for this contribution!

LGTM 👏

@chrispruitt
Copy link
Contributor Author

Are we able to get one more approval and merge this guy???

Thanks!

@mmatur mmatur removed their assignment Mar 29, 2024
@rtribotte rtribotte self-assigned this Apr 2, 2024
Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

Thanks 👌

@traefiker traefiker merged commit ac1753a into traefik:v3.0 Apr 4, 2024
22 checks passed
@kevinpollet kevinpollet changed the title Nomad provider to allow empty services Allow empty services Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/provider/nomad kind/enhancement a new or improved feature. priority/P2 need to be fixed in the future size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants