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

add static ip to kube play #8617

Closed
wants to merge 1 commit into from

Conversation

mier85
Copy link

@mier85 mier85 commented Dec 6, 2020

Signed-off-by: Michael Ernst [email protected]

this is implementing #8442

tried running lint & validate & tests, but they all failed before and after my commit. ( even tried the gate which is still partially in the CONTRIBUE.md but then realized that this seems to be no longer a thing )

Signed-off-by: Michael Ernst <[email protected]>
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mier85
To complete the pull request process, please assign mheon after the PR has been reviewed.
You can assign the PR to them by writing /assign @mheon in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@zhangguanzhang
Copy link
Collaborator

zhangguanzhang commented Dec 7, 2020

Lack of documentation and unit tests in https://github.com/containers/podman/blob/master/test/e2e/play_kube_test.go

@mier85
Copy link
Author

mier85 commented Dec 7, 2020

@zhangguanzhang I will add the documentation in https://github.com/containers/podman/blob/master/docs/source/markdown/podman-play-kube.1.md and some unit tests in the file you pointed to.

@zhangguanzhang
Copy link
Collaborator

👌

Copy link
Member

@Luap99 Luap99 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 the contribution @mier85

I left some comment to simplify this a bit.

podman play kube can be run via the api and podman-remote. So you also need to add this option to the api and remote client.

The api parameter is defined here: https://github.com/containers/podman/blob/0c2a43b99db8a4fd75412a277da6de2731017d3e/pkg/api/handlers/libpod/play.go

Documentation for the api parameter has to be added here: https://github.com/containers/podman/blob/0c2a43b99db8a4fd75412a277da6de2731017d3e/pkg/api/server/register_play.go

Parameter for the remote client has to be added here: https://github.com/containers/podman/blob/master/pkg/bindings/play/play.go

@@ -23,6 +24,7 @@ type playKubeOptionsWrapper struct {
TLSVerifyCLI bool
CredentialsCLI string
StartCLI bool
StaticIPCLI string
Copy link
Member

Choose a reason for hiding this comment

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

Not needed see other comment

Copy link
Author

Choose a reason for hiding this comment

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

flags will make things much easier, will update that

Comment on lines +80 to +83
staticIPFlagName := "ip"
flags.StringVar(&kubeOptions.StaticIPCLI, staticIPFlagName, "", "Static IP address to assign to this pod")
_ = kubeCmd.RegisterFlagCompletionFunc(staticIPFlagName, completion.AutocompleteDefault)

Copy link
Member

Choose a reason for hiding this comment

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

You can just use an IP flag. No need for the extra parsing logic

Suggested change
staticIPFlagName := "ip"
flags.StringVar(&kubeOptions.StaticIPCLI, staticIPFlagName, "", "Static IP address to assign to this pod")
_ = kubeCmd.RegisterFlagCompletionFunc(staticIPFlagName, completion.AutocompleteDefault)
staticIPFlagName := "ip"
flags.IPVar(&kubeOptions.StaticIP, staticIPFlagName, nil, "Static IP address to assign to this pod")
_ = kubeCmd.RegisterFlagCompletionFunc(staticIPFlagName, completion.AutocompleteNone)

Comment on lines +128 to +135
if kubeOptions.StaticIPCLI != "" {
ipAddress := net.ParseIP(kubeOptions.StaticIPCLI)
if ipAddress == nil {
return fmt.Errorf("failed parsing ip address: %s", kubeOptions.StaticIPCLI)
}
kubeOptions.StaticIP = ipAddress
}

Copy link
Member

Choose a reason for hiding this comment

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

Not needed see other comment

@github-actions
Copy link

github-actions bot commented Jan 7, 2021

A friendly reminder that this PR had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Jan 7, 2021

@mier85 Still working on this? We need to get this in by next week, for the 3.0 release, if we are going to get it done.

@mier85
Copy link
Author

mier85 commented Jan 7, 2021

@rhatdan still working on it. Will try to get this done ASAP this week.

But I have a question. As several Pods could be spawned when deploying ( e.g. replicas in a kubernetes manifest ) , a single static IP wouldn't work. In this case, should be assign a range starting with the IP or should we fail and ask for several IPs or should this be a "single container only" option?

@rhatdan
Copy link
Member

rhatdan commented Jan 7, 2021

I am no kubernetes expert, but I would figure fail would be the correct thing to do.

@mheon
Copy link
Member

mheon commented Jan 7, 2021

Making static IPs incompatible with Deployments, at least initially, sounds like a fine solution.

@hoonetorg
Copy link

hoonetorg commented Jan 7, 2021

@mier85: i see it that way:
if kubernetes yaml is of type pod, only one pod will be deployed. if the kind is deployment and replicas is one, also only one pod will be deployed. that are two valid scenarios for play kube, where a static ip would make sense.
the thing with the range is probably a bit too difficult for the initial shot.

@vrothberg
Copy link
Member

Friendly ping. Are we good to continue?

@rhatdan
Copy link
Member

rhatdan commented Mar 15, 2021

@umohnani8 Could you take this one over?

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2021
@openshift-ci-robot
Copy link
Collaborator

@mier85: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Luap99
Copy link
Member

Luap99 commented Apr 17, 2021

Replaced by #10043

@Luap99 Luap99 closed this Apr 17, 2021
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. stale-pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants