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

support kubernetes native deployment yaml. #422

Closed
wants to merge 1 commit into from
Closed

support kubernetes native deployment yaml. #422

wants to merge 1 commit into from

Conversation

umutcann
Copy link
Contributor

@umutcann umutcann commented Mar 5, 2021

Kubernetes Deployment is an object that can represent an application running on your cluster. All of configurations is stored in a yaml file.

Some communities can manage app configurations in deployment yaml as a DevOps practice. spring-cloud-deployer-kubernetes library can support it with these implementation.

@ilayaperumalg
Copy link
Contributor

@umutcann Thanks for the PR! I believe this PR doesn't effectively address the issue reported in #414 as we would like to have the sidecar container injected for the app getting deployed via AppDeploymentRequest.

Anyway, as you suggested earlier this would benefit non SCDF use cases. We'll follow up on this soon!

@umutcann
Copy link
Contributor Author

umutcann commented Mar 8, 2021

Hi @ilayaperumalg

Thanks for your following up.

I thought that we are able to support sidecar container solution If the spring cloud deployer kubernetes supports a way which we are able to set kubernetes native deployment yaml.

I tried to create more extensive solution :)

I'm waiting your reviews for my contribution . I can fix code by your review.

@ilayaperumalg
Copy link
Contributor

@umutcann Please check this PR #423 which adds support for injecting sidecar container into PodSpec.
The K8s deployer can allow adding additional sidecar container using the deployer property spring.cloud.deployer.kubernetes.sidecarContainer.

@ilayaperumalg
Copy link
Contributor

While I like and appreciate your current PR, it addresses a generic injection of K8s spec which negates the Spring Cloud Deployer contract for deploying applications using AppDeploymentRequest.

@umutcann
Copy link
Contributor Author

umutcann commented Mar 8, 2021

Hi @ilayaperumalg,

As I understand, you are considering negatively for this PR.

Actually we are managing all of deployment configuration in deployment yaml at our community as a DevOps standard.

Current state is forcing us. Because we have to manage deployment standards for batch/online applications separately .

We need to set generic generic deployment yaml for our batch and online apps.

I think it would be great feature for all of people who use this library. Users of SCDF will be able to launch task using 2 params instead of more than so many params.

May you please judge this implementation as a new feature If it is suitable your design?

If it is not, please let me know that how we can adapt this need to your design.

@ilayaperumalg
Copy link
Contributor

Hi @umutcann,

This PR certainly adds a new implementation which could be useful. I completely agree.
Also, the PR #423 isn't a replacement for this PR instead it is to address the original issue #414.
I want to discuss your implementation with the team and will follow up on how we can get this in as a generic deployment feature. Thanks!

@umutcann
Copy link
Contributor Author

umutcann commented Mar 8, 2021

I got it @ilayaperumalg. I glad to have same idea with you :)

I am waiting output of your discuss curiously.

@umutcann
Copy link
Contributor Author

Hi @ilayaperumalg,

Is there any update for your discuss?

Thanks in advance.

@ilayaperumalg
Copy link
Contributor

Hi @umutcann, The consensus from the team is not to get this feature for now as the deployment of pure YAML from within spring-cloud-deployer-kubernetes pollutes the purpose this library as this library needs to conform to the spring-cloud-deployer SPI and specifically using the AppDeploymentRequest.

We certainly value your contribution and efforts. We want to wait until the team discusses the next major version planning which would happen mid April and we'll see if we can re-consider this feature as an extra option. Hence, we'll keep this open until then. Sorry about the delay or inconvenience this may have caused. Thank you!

@umutcann
Copy link
Contributor Author

Thanks @ilayaperumalg . I hope that your consensus decision would turn against. Because it will be very useful for consumers.

I am trusting you regarding convince your team :)

@ilayaperumalg
Copy link
Contributor

Hi @umutcann , Sorry to disappoint you but, We don't have enough votes to get this merged yet. I want to keep this open to bring this to the team again during the planning for the next release scope (end of April).

@umutcann
Copy link
Contributor Author

Hi @ilayaperumalg ,

Thank you for your following.

Actually I had asked for PR of sidecar feature.

Is that also being merged after your next release scope?

@ilayaperumalg
Copy link
Contributor

Hi @umutcann , We plan to merge this in the current release. Thanks for clarifying!

@ilayaperumalg ilayaperumalg requested a review from chrisjs April 19, 2021 15:03
@chrisjs
Copy link
Contributor

chrisjs commented Apr 19, 2021

we have had an issue open since 2017 on this subject - #154

as @ilayaperumalg mentions, having the ability to send over arbitrary yaml sidesteps the deployer and any logic we may wrap to track what objects were created in the system.

if we were to consider this ability at some point, i think we need to spend some cycles researching and testing how this may work, edge cases and so on -- potentially augmenting the system to deal with these potential issues.

@chrisjs chrisjs closed this Apr 19, 2021
@umutcann
Copy link
Contributor Author

Hi @chrisjs,

As I mentioned before, many community manage their deployment in k8s deployment yaml as devops standard. It's forcing us while scdf are not supporting this util.

May you please let me know how long time you need to research and test for supporting deployment yaml solution? I'm asking that in order to consider alternative solutions for our community, If you not plan to support it for short term.

If there is issue which exist since 2017, I believe that you have plan for this need. If you specify to scope of research and test, I glad to do that.

@ilayaperumalg
Copy link
Contributor

Hi @umutcann ,

May you please let me know how long time you need to research and test for supporting deployment yaml solution? I'm asking that in order to consider alternative solutions for our community, If you not plan to support it for short term.

We understand that this could be handy in your use cases but after careful consideration, we decided not to have this feature in Spring Cloud Deployer Kubernetes as it essentially introduces further complications when it comes to deploying apps using dedicated yaml which could override what is set in AppDeploymentRequest. One of the main reasons is that, it removes the controlling and orchestrating we have in SCDF when it comes to deploying the stream/task applications.

May you please let me know how long time you need to research and test for supporting deployment yaml solution? I'm asking that in order to consider alternative solutions for our community, If you not plan to support it for short term.

We feel bad that we could not support your use case and changes at this time and to be honest we can't provide any timeline when this feature could be in scope. So, please consider using alternative solutions such as having additional containers (sidecar, adapter patterns) .

If there is issue which exist since 2017, I believe that you have plan for this need. If you specify to scope of research and test, I glad to do that.

I didn't realise this issue's existence until Chris pointed out. Irrespective of this, once the scope of this makes sense in this project, we definitely re-open this issue and get you started on this. We appreciate your time you already spent on this and your willingness to take this further. Thank you!

@umutcann
Copy link
Contributor Author

Hi @ilayaperumalg

Thanks for your kindness.

Although we could not add this feature for now, I hope we would proceed with alternative solutions.

@ilayaperumalg
Copy link
Contributor

Although we could not add this feature for now, I hope we would proceed with alternative solutions.

Sure, thank you!

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