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

Use a specific value for nodePort when the type of Service is NodePort #8840

Merged
merged 3 commits into from
May 6, 2020

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Apr 24, 2020

The idea behind this is that subsequent updates / pushes to a cluster
using a NodePort Service (most likely to be used with Minikube)
will result in the same external URL being used.
This way users don't have to find the new URL each time they update / push.

@geoand
Copy link
Contributor Author

geoand commented Apr 24, 2020

@iocanel I am not sure that the Decorator implementation I added is the most appropriate...

@Ladicek @maxandersen I would like to hear your thougths as well.

@geoand
Copy link
Contributor Author

geoand commented Apr 27, 2020

@maxandersen brought up the point that this will fail if more than one replicas are deployed.

Max also raised the point that these kind of settings should be applied when the user is in "development mode", which unfortunately for the time being doesn't tied in to the Quarkus Dev Mode, since the Kubernetes extension requires the prod jar to be built.

My idea with this PR is to make the deployment / update experience for a user using Mininkube as friction-less as possible.
In my view when a user uses type NodePort for a service, it's pretty much guaranteed that they in a sort of "developer mode", not production mode, therefore we should make it as easy as possible to deploy and update the application.
This also means that the user will probably be using a single replica (we can of course warn when there are more).
Another idea I had was to not use a specific nodePort as the default, but to derive it from the application name - thus guaranteeing that it stays the same between application updates. Again this would only be done when nodePort is set for the service-type and would only be done when a single replica is used.

Thoughts?

Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

I am a bit skeptical about using a fixed node port.

What will happen if for any reason the port is already taken?
What will happen if I am using the kubernetes extension in multiple applications or modules?

So, I would like to do some small research for alternatives.

If no other alternative can be found, then I would add the following behavior:

  1. Feature flag to turn this on and off (default value is debatable).
  2. I would hash the service name and namespace and get the default node port (to prevent conflicts if no node port has been explicitly specified).
  3. Deployer should display the node port to the user.


public AddNodePortDecorator(String name, int nodePort) {
super(name);
if (nodePort < MIN_VALUE || nodePort > MAX_VALUE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This https://kubernetes.io/docs/concepts/services-networking/service/#nodeport says that the default range is 30000-32767, and apparently it's configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, nice find! I need to remove these checks then.

/**
* The nodePort to set when serviceType is set to nodePort
*/
@ConfigItem(defaultValue = "31987")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to not specify the concrete nodePort and let Kubernetes assign a port automatically? Perhaps allow the 0 value, which typically means "port should be auto-assigned"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you don't set it, it gets assigned automatically. The problem is that it gets re-assigned to another value when you update the application and that's what I am trying to avoid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. What I'm trying to say is: is there a way how to get back to the default behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that essentially boils down to what @iocanel is asking for :)

@geoand
Copy link
Contributor Author

geoand commented Apr 27, 2020

I am a bit skeptical about using a fixed node port.

What will happen if for any reason the port is already taken?
What will happen if I am using the kubernetes extension in multiple applications or modules?

So, I would like to do some small research for alternatives.

If no other alternative can be found, then I would add the following behavior:

  1. Feature flag to turn this on and off (default value is debatable).

Why would you need a feature flag? Isn't the use of NodePort along with 1 replica enough?

  1. I would hash the service name and namespace and get the default node port (to prevent conflicts if no node port has been explicitly specified).

Yes, this is pretty much what I suggested above.

  1. Deployer should display the node port to the user.

Good idea

@Ladicek
Copy link
Contributor

Ladicek commented Apr 27, 2020

I'd say there's too much assumptions in this, so there should be a way out. Other than that, I don't have an opinion, because I have no idea what NodePort services are good for.

@geoand
Copy link
Contributor Author

geoand commented Apr 27, 2020

I'd say there's too much assumptions in this, so there should be a way out.

The way I see it, is that this thing is opt-in. You need to set the service-type to nodePort (which is not the default), and you also need to make sure replicas are set to 1 (which is a check I need to add)

Other than that, I don't have an opinion, because I have no idea what NodePort services are good for.

AFAIK, they are only good for "local development" - meaning they are the easiest way to expose a service to the outside world.

@iocanel
Copy link
Contributor

iocanel commented Apr 27, 2020

I am a bit skeptical about using a fixed node port.
What will happen if for any reason the port is already taken?
What will happen if I am using the kubernetes extension in multiple applications or modules?
So, I would like to do some small research for alternatives.
If no other alternative can be found, then I would add the following behavior:

  1. Feature flag to turn this on and off (default value is debatable).

Why would you need a feature flag? Isn't the use of NodePort along with 1 replica enough?

Not really, the user should be able to select 1 replica, nodePort without using a fixed port.

  1. I would hash the service name and namespace and get the default node port (to prevent conflicts if no node port has been explicitly specified).

Yes, this is pretty much what I suggested above.

I am guilty of posting without reading :-D

  1. Deployer should display the node port to the user.

Good idea

@iocanel
Copy link
Contributor

iocanel commented Apr 27, 2020

If we get the deployer to display the node port to the user, we could make it somewhat smarter and not reapply the service if it hasn't actually changed (in the hope of the using the same port), so that the user doesn't have to change ports all the time.

@geoand
Copy link
Contributor Author

geoand commented Apr 27, 2020

So if I add feature flag and nice logging messages, do we agree on this approach?

@geoand
Copy link
Contributor Author

geoand commented Apr 27, 2020

If we get the deployer to display the node port to the user, we could make it somewhat smarter and not reapply the service if it hasn't actually changed (in the hope of the using the same port), so that the user doesn't have to change ports all the time.

This seems more complicated to get right to me.

@maxandersen
Copy link
Member

The docs on nodeport gives the reasons what nodeport is useful for:

Using a NodePort gives you the freedom to set up your own load balancing solution, to configure environments that are not fully supported by Kubernetes, or even to just expose one or more nodes’ IPs directly.

We are kinda doing the latter here.

I still think the default behavior is best kept at "let kubernetes deal with it" but make it easy to activate this "dev-deployment" mode.

@geoand
Copy link
Contributor Author

geoand commented Apr 27, 2020

It looks like having more than one replica, does not cause a problem with nodePort. However the exposed service only "targets" a single one of them, the other replicas are not accessible from outside the cluster.

@geoand
Copy link
Contributor Author

geoand commented Apr 27, 2020

The docs on nodeport gives the reasons what nodeport is useful for:

Using a NodePort gives you the freedom to set up your own load balancing solution, to configure environments that are not fully supported by Kubernetes, or even to just expose one or more nodes’ IPs directly.
I still think the default behavior is best kept at "let kubernetes deal with it" but make it easy to activate this "dev-deployment" mode.

That seems to be the consencus here - if I have read all the comments correctly that is.
I'll look into it.

@maxandersen
Copy link
Member

It looks like having more than one replica, does not cause a problem with nodePort. However the exposed service only "targets" a single one of them, the other replicas are not accessible from outside the cluster.

are you sure ? I read the docs stating this: "you can specify a value in the nodePort field. The control plane will either allocate you that port or report that the API transaction failed. This means that you need to take care of possible port collisions yourself. You also have to use a valid port number, one that’s inside the range configured for NodePort use."

@geoand
Copy link
Contributor Author

geoand commented Apr 27, 2020

It looks like having more than one replica, does not cause a problem with nodePort. However the exposed service only "targets" a single one of them, the other replicas are not accessible from outside the cluster.

are you sure ? I read the docs stating this: "you can specify a value in the nodePort field. The control plane will either allocate you that port or report that the API transaction failed. This means that you need to take care of possible port collisions yourself. You also have to use a valid port number, one that’s inside the range configured for NodePort use."

Yes, I just tried it. There was no failure, all pods came up and the service was exposed, but only one of the pods was receiving traffic.

@iocanel
Copy link
Contributor

iocanel commented Apr 27, 2020

It looks like having more than one replica, does not cause a problem with nodePort. However the exposed service only "targets" a single one of them, the other replicas are not accessible from outside the cluster.

are you sure ? I read the docs stating this: "you can specify a value in the nodePort field. The control plane will either allocate you that port or report that the API transaction failed. This means that you need to take care of possible port collisions yourself. You also have to use a valid port number, one that’s inside the range configured for NodePort use."

Yes, I just tried it. There was no failure, all pods came up and the service was exposed, but only one of the pods was receiving traffic.

Which makes sense cause you are not load balancing.
This is a sideffect of using NodePort not a fixed NodePort.

@geoand
Copy link
Contributor Author

geoand commented Apr 27, 2020

Here is my latest idea:

We don't use a flag per se, but instead have an enum configuration value named quarkus.kubernetes.dev-mode (or something like that) that behaves like so:

  • OFF - No changes are made whatsoever
  • ON - We enable nodePort and pick a proper port, set replica to 1 (and in the future perhaps we do more)
  • INFER - We try to infer whether the cluster is Minikube (and whatever else makes sense) and if it is we make the same changes as if ON was set.

The default should be OFF most likely.

What do you think?

@iocanel
Copy link
Contributor

iocanel commented Apr 27, 2020

IMHO, it doesn't feel quite right.

For starters, we shouldn't INFER. Anything related to talking to an existing cluster should be off the table. In the same spirit, the same should apply to things like minikube status and the likes :-D

I also don't like the dev-mode thing, as it might be too confusing.
When the user specifies NodPort type, we could do the following:

  1. Use the provided port.
  2. Use a unique port based on name & namespace (if no port is specified).
  3. Let kubernetes pick port if the user explicitly specifies 0 as the desired port value.

3 allows us to disable the special feature without introducing weird-looking properties.

Now, since we can't query the environment for minikube / minishift cluster status etc, I am wondering if we could introduce minikube and minishit as individual deployment targets, with their own presets.

@geoand
Copy link
Contributor Author

geoand commented Apr 27, 2020

IMHO, it doesn't feel quite right.

For starters, we shouldn't INFER. Anything related to talking to an existing cluster should be off the table. In the same spirit, the same should apply to things like minikube status and the likes :-D

I don't see why we shouldn't do this. It's purely a dev thing, so why not make developers lives easier?

I also don't like the dev-mode thing, as it might be too confusing.
When the user specifies NodPort type, we could do the following:

  1. Use the provided port.
  2. Use a unique port based on name & namespace (if no port is specified).
  3. Let kubernetes pick port if the user explicitly specifies 0 as the desired port value.

3 allows us to disable the special feature without introducing weird-looking properties.

Letting Kubernetes pick the port however disrupts the developers workflow by changing the URL that is used to access the application. That's what I am really trying to avoid here.

Now, since we can't query the environment for minikube / minishift cluster status etc, I am wondering if we could introduce minikube and minishit as individual deployment targets, with their own presets.

Sure, but why do this if we can infer the environment?

@davsclaus
Copy link
Contributor

Having a really awesome and easy "kick the tires" experience for new users to quarkus and kubernetes running on their local developer laptop (especially for windows, mac users) is something that can really help quarkus and kick us ahead.

Just think what Docker Desktop does for user experience and how to install and use it.

Minikube is reasonable easy to install - and having a great blog, and video how to do the code.quarkus.io -> zip -> docker mutlistage build -> kubectl apply -f -> runs on kubernetes with 4mb of memory usage.

And you would need the NodePort to call your deployed kubernetes.

And btw would also be good if quarkus could log on startup whether it runs in JVM or Native mode.

@Ladicek
Copy link
Contributor

Ladicek commented Apr 27, 2020

If we're after good developer experience, what's the point of messing with NodePorts when you can minikube addons enable ingress and do everything properly?

@geoand
Copy link
Contributor Author

geoand commented Apr 27, 2020

If we're after good developer experience, what's the point of messing with NodePorts when you can minikube addons enable ingress and do everything properly?

Because NodePort is the easiest OOTB experience :)

@Ladicek
Copy link
Contributor

Ladicek commented Apr 27, 2020

That I find hard to agree with.

EDIT: I mean, we actually should point people towards using an Ingress, as that's how traffic income in Kubernetes should be done. To the best of my knowledge.

@Ladicek
Copy link
Contributor

Ladicek commented Apr 27, 2020

In any case, if we make nodePort configurable (I don't disagree with that!), and if we make the default value some specific number (I don't disagree with that either, if it's documented), then there needs to be a way how to get back to the default behavior (automatic assignment), because you just can't assume that NodePorts will only be used for the scenario you're thinking of right now.

@iocanel
Copy link
Contributor

iocanel commented Apr 27, 2020

IMHO, it doesn't feel quite right.
For starters, we shouldn't INFER. Anything related to talking to an existing cluster should be off the table. In the same spirit, the same should apply to things like minikube status and the likes :-D

I don't see why we shouldn't do this. It's purely a dev thing, so why not make developers lives easier?

I also don't like the dev-mode thing, as it might be too confusing.
When the user specifies NodPort type, we could do the following:

  1. Use the provided port.
  2. Use a unique port based on name & namespace (if no port is specified).
  3. Let kubernetes pick port if the user explicitly specifies 0 as the desired port value.

3 allows us to disable the special feature without introducing weird-looking properties.

Letting Kubernetes pick the port however disrupts the developers workflow by changing the URL that is used to access the application. That's what I am really trying to avoid here.

Now, since we can't query the environment for minikube / minishift cluster status etc, I am wondering if we could introduce minikube and minishit as individual deployment targets, with their own presets.

Sure, but why do this if we can infer the environment?

IMHO resource generation should be idempotent ( should never rely on external things ).
Having generated resources change from machine to machine and from time to time based on what minikube status returns guaranteed to cause issues. We've tried in the past with FMP and it didn't end up well.

@geoand
Copy link
Contributor Author

geoand commented May 5, 2020

So, perhaps I misunderstood something, but I realized yesterday that the quarkus.kubernetes.deployment-target config property has 2 distinct purposes: select which [many] YAML files to generate, and select which [single] YAML file to deploy when quarkus.kubernetes.deploy=true. I think that's probably what's been bugging me.

That understanding is correct.

If we really want to have io.quarkus:quarkus-minikube, then perhaps quarkus.kubernetes.deployment-target should never affect which [many] YAML files are generated (this is selected by the extensions that are included), and it is only used to select which [single] YAML file is deployed. (Alternatively, would it be possible to discover the kind of the cluster we are connected to, and select the [single] YAML file based on that info?)

So currently if the user adds the quarkus-minikube extension that I added in this PR, when the then when no deployment-target is selected, both minikube.yaml and kubernetes.yaml are geneated, but minikube.yaml is applied when quarkus.kubernetes.deploy=true is set.
If the user wants to do something different, he/she needs to explicitly set deployment-target.

I personally think this behavior is reasonable, but it definitely needs to be properly documented.

@geoand
Copy link
Contributor Author

geoand commented May 5, 2020

(Alternatively, would it be possible to discover the kind of the cluster we are connected to, and select the [single] YAML file based on that info?)

That was my initial thought as well. We could certainly add that as an enhancement later on.

@iocanel
Copy link
Contributor

iocanel commented May 5, 2020

IIUC, this is perfectly aligned with what we currently have. So, this pull request just introduces a new deployment target, that the user can optionally enable. I like it!

Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

LGTM,

I would like though a minor additon: A testcase that shows that minikube resource is only added when explicitly specified.

@geoand
Copy link
Contributor Author

geoand commented May 5, 2020

LGTM,

I would like though a minor additon: A testcase that shows that minikube resource is only added when explicitly specified.

In BasicKubernetesTest we have:

        final Path kubernetesDir = prodModeTestResults.getBuildDir().resolve("kubernetes");
        assertThat(kubernetesDir)
                .isDirectoryContaining(p -> p.getFileName().endsWith("kubernetes.json"))
                .isDirectoryContaining(p -> p.getFileName().endsWith("kubernetes.yml"))
                .satisfies(p -> assertThat(p.toFile().listFiles()).hasSize(2));

which does test that when there is no minikube extension and that the minikube target isn't added, then no minikube.yaml is generated.

Did you have something more in mind @iocanel ?

@maxandersen
Copy link
Member

Alternatively, would it be possible to discover the kind of the cluster we are connected to, and select the [single] YAML file based on that info?)

this is kinda what the original PR was doing which would make things slow and you would always have a remote lookup - which could fail if the cluster not available etc.

Thus I like this issue much better where if you don't say anything it will pick the extension that are avialable to generate - but if any ambiguity you have to state which to use - either on command line or in application.properties to use as default.

@geoand
Copy link
Contributor Author

geoand commented May 5, 2020

I think the myself, Ioannis and Max agree on the current approach, so unless @Ladicek objects very strongly, I believe the way forward is to merge this.

@Ladicek
Copy link
Contributor

Ladicek commented May 5, 2020

You took it out of context. I only proposed discovering what kind of cluster is there when stuff is being deployed to it. Not when YAML files are generated. (If that would be possible, we could get rid of quarkus.kubernetes.deployment-target altogether.)

Sorry, I currently don't have time to respond properly.

@Ladicek
Copy link
Contributor

Ladicek commented May 5, 2020

The current approach adds more inconsistency (new quarkus-minikube extension, new quarkus.kubernetes.deployment-target=minikube value, but not new quarkus.minikube.* configuration options?), which is bad IMO, but as I said, I unfortunately don't have time to respond properly :-(

@geoand
Copy link
Contributor Author

geoand commented May 5, 2020

The idea is to create a new minikube.yaml based on defaults we think should apply for it. If a user doesn't want to use those, they can always just use the fully controllable kubernetes.yaml.

I don't think it's adding any inconsistency, it's just adding more options. But like I said, we need to move forward one way or another, so if you don't like it (which is perfectly understandable and acceptable), then we really need a proposal for an alternative.

@Ladicek
Copy link
Contributor

Ladicek commented May 5, 2020

More options that are not orthogonal = inconsistency.

(My proposal? Determine which YAML files to generate solely based on which quarkus-{kubernetes,minikube,openshift} extensions are present. Ignore quarkus.kubernetes.deployment-target for that process. If possible to detect which cluster we're talking to, determine which YAML file to deploy based on that and get rid of quarkus.kubernetes.deployment-target completely; otherwise determine which YAML file to deploy based on quarkus.kubernetes.deployment-target.)

At this point, I cave in. I officially retract everything I wrote in this issue, please treat it as if I never commented. Sorry, I don't have the required time these days.

@geoand
Copy link
Contributor Author

geoand commented May 5, 2020

@Ladicek thanks for the input.

The problem I see with your proposal is that for the example the minikube extensions always implies the kubernetes extension, so the only way to disambiguate when deploying would be to try to determine the target cluster - which I am not entirely opposed to, but I don't think should be the default.

At this point I'll leave it up to the man in change to make the call, @maxandersen :)

@maxandersen
Copy link
Member

I know you said you want to ignore but I think you made some interesting points @Ladicek I'm going to ask in hope to better understand your concerns.

"not orthogonal = inconsistency" - is that the issue that there are (currently) no minishift.* options to tweak ? if yes, then this approach does not exclude this from happening/getting added if we get some usecases where relying on the kubernetes config is not enough for minikube variation.

About ignoring deployment target for generating files - maybe I'm being naive here but what would that give you if only the specific files will be used when deploying ? I'm missing some usecase where you think its valuable to have this behavior.

@Ladicek
Copy link
Contributor

Ladicek commented May 6, 2020

I think what I'm trying to say is that this is getting progressively harder to understand. minikube now looks very much like kubernetes or openshift (or knative? Never played with that one), but it isn't. Instead of adding special cases to cater for more usecases, we should strive to generalize. My suggestions were not well thought through in the last few days, so I'm not going to add more :-)

@geoand geoand force-pushed the k8s-service-update branch from 01b1a3a to 0c2b5f7 Compare May 6, 2020 07:18
@geoand
Copy link
Contributor Author

geoand commented May 6, 2020

I rebased the PR onto master because I wanted to make sure that nothing from #8894 would interfere with this one.

geoand added 3 commits May 6, 2020 14:01
Essentially now the first user supplied value is used to deploy.
When no user supplied value exists, then priorities are used to
determine which target should be deployed
@geoand geoand force-pushed the k8s-service-update branch from 0c2b5f7 to 981e546 Compare May 6, 2020 11:01
@maxandersen
Copy link
Member

I think what I'm trying to say is that this is getting progressively harder to understand. minikube now looks very much like kubernetes or openshift (or knative? Never played with that one), but it isn't.

I'm a bit puzzled by your comment here - minikube, kubernetes and openshift looks much a like since they are alike.

kubernetes is the plain one, we add minkube deployment target to tweak defaults in how you would use it out-of-box but we don't add additional minikube.* properties as we believe (thus far) that minikube is as close as it gets to kubernetes.

for openshift we do the same, have an extension to tweak the default choices for a better out of box experience and since openshift actually have openshift specific settings we have those as properties you can tweak.

I feel the configuration and behaviors we allow to set here are as close to reality as we can get.

Instead of adding special cases to cater for more usecases, we should strive to generalize. My suggestions were not well thought through in the last few days, so I'm not going to add more :-)

I agree we should generalize where we can - I'm struggling to see where we can generalize more in this case. if you have some ideas i'm all ears on that!

on this issue I say we should LGTM as it fits into the current approach of deployment targets thus adding it does not "hurt" and if we find ways to generalize more it should be doable with 4 deployment target options as it would be with the current 3.

@geoand geoand merged commit a8e59ac into quarkusio:master May 6, 2020
@geoand geoand added this to the 1.5.0 milestone May 6, 2020
@geoand geoand deleted the k8s-service-update branch May 6, 2020 13:36
@geoand
Copy link
Contributor Author

geoand commented May 6, 2020

I still need to follow up with some documentation

geoand added a commit to geoand/quarkus that referenced this pull request May 25, 2020
geoand added a commit to geoand/quarkus that referenced this pull request May 25, 2020
Follows up on quarkusio#8840

Co-authored-by: Guillaume Smet <guillaume.smet@gmail.com>
gsmet added a commit to gsmet/quarkus that referenced this pull request May 26, 2020
Follows up on quarkusio#8840

Co-authored-by: Guillaume Smet <guillaume.smet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/kubernetes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants