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.

Sign up for GitHub

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

Jump to bottom

Warn about creating multiple replicas from kubernetes YAML #17082

Merged
merged 1 commit into from
Jan 14, 2023
Merged

Warn about creating multiple replicas from kubernetes YAML #17082

merged 1 commit into from
Jan 14, 2023

Conversation

dcermak
Copy link
Contributor

@dcermak dcermak commented Jan 11, 2023 •
edited
Loading

See: #16766 (comment)

- Limit replicas to one when creating pods/containers from kubernetes YAML

This fixes: #16765
cc @vrothberg

vrothberg
vrothberg reviewed Jan 11, 2023
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks!

The warning is not sufficient for what I had in mind. I think Podman should

  • Enforce 1 replica. Hence, even if 3 are defined, only one will be started.
  • Warn that Podman limits the replicas to one. Currently, the warning states they are not supported.

We should also add a test and make sure that warning is emitted. The test won't pass for remote clients because there is no plumbing for progress (yet).

@mheon
Copy link
Member

mheon commented Jan 11, 2023

Would this be considered a breaking change? We go from "supporting" replicas to ignoring them.

@rhatdan
Copy link
Member

rhatdan commented Jan 11, 2023

Replicas make little sense on a single node, so warning is the best we can do.

@rhatdan
Copy link
Member

rhatdan commented Jan 11, 2023

I don't think we really supported Replicas before.

@mheon
Copy link
Member

mheon commented Jan 11, 2023

We did create them (though I agree they didn't really function properly - or at all, really)

@TomSweeneyRedHat
Copy link
Member

If we want to do just a warning, the approach LGTM. I'll let others debate the merit of that approach.

@dcermak dcermak force-pushed the warn-about-replicas branch 2 times, most recently from 383b4d1 to 2e1e989 Compare January 12, 2023 07:23
@ygalblum ygalblum mentioned this pull request Jan 12, 2023
Closed
ygalblum
ygalblum reviewed Jan 12, 2023
View reviewed changes
Copy link
Collaborator

@ygalblum ygalblum left a comment

Choose a reason for hiding this comment

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

Code LGTM, but, need to see why tests are failing. Maybe there are some more tests that need adjustment

@dcermak dcermak force-pushed the warn-about-replicas branch 2 times, most recently from ecad704 to 6b81a71 Compare January 12, 2023 10:49
@dcermak
Copy link
Contributor Author

dcermak commented Jan 12, 2023

@ygalblum Tests should be green now.

vrothberg
vrothberg reviewed Jan 12, 2023
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Need a test that expects the warning to be printed to make sure we don't silently regress in the future.

test/e2e/play_kube_test.go Show resolved Hide resolved
@dcermak
Copy link
Contributor Author

dcermak commented Jan 12, 2023

@vrothberg I have added a test for the log output and also tried to add a test that verifies that podman really only creates a single pod. But I'm not really happy with it, because it relies on podman's stdout. However, I did not want to use the getPodNameInDeployment helpers, as these use internal knowledge about podman's naming scheme and will by design never give you more than one pod name.

vrothberg
vrothberg approved these changes Jan 12, 2023
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @dcermak !

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 12, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcermak, vrothberg

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2023
ygalblum
ygalblum reviewed Jan 12, 2023
View reviewed changes
Copy link
Collaborator

@ygalblum ygalblum left a comment

Choose a reason for hiding this comment

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

LGTM

@vrothberg
Copy link
Member

Tests aren´t happy yet

@dcermak dcermak force-pushed the warn-about-replicas branch 2 times, most recently from 5ec7588 to a78cc1d Compare January 12, 2023 16:29
@dcermak
Copy link
Contributor Author

dcermak commented Jan 12, 2023

Tests aren´t happy yet

Yes, the test checking for the log warning was still running for remote integration tests, but iirc that is not supported. Should be fixed now 🤞

@dcermak
Limit replica count to 1 when deploying from kubernetes YAML
d9bf3f1 
This fixes: containers#16765

Signed-off-by: Dan Čermák <dcermak@suse.com>
@dcermak
Copy link
Contributor Author

dcermak commented Jan 13, 2023

Tests aren´t happy yet

After a few complimentary force-pushes, they were convinced to be happy ;-)

@TomSweeneyRedHat
Copy link
Member

LGTM
and happy green test buttons.

@rhatdan
Copy link
Member

rhatdan commented Jan 14, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2023
@openshift-merge-robot openshift-merge-robot merged commit ae674a3 into containers:main Jan 14, 2023
@thedanielenavarra
Copy link

Hi everyone,
First of all I'd like to thank you guys for the amazing work on Podman
Here's my question: since the official docs indicate the replica field as supported:
podman-replicas

Is it going to be dropped as a supported feature or will it be actually implemented?

Since Podman can be used to test YAML files that will eventually be applied on Kubernetes clusters, in my humble opinion the concept of replicas is not secondary, it would be very useful to test them even on a single node.

Please let me know if this is the wrong place to ask this question 😄
Thanks in advance

@dcermak dcermak deleted the warn-about-replicas branch February 27, 2023 07:15
dcermak added a commit to dcermak/podman that referenced this pull request Feb 27, 2023
@dcermak
Remove replicas field from kubernetes deployment support doc
d1590ec 
Support for this field has been effectively removed via
containers#17082

Signed-off-by: Dan Čermák <dcermak@suse.com>
@dcermak
Copy link
Contributor Author

dcermak commented Feb 27, 2023

Is it going to be dropped as a supported feature or will it be actually implemented?

I think we have effectively dropped support for this feature with this PR, as we deemed it to not really make a lot of sense to offer multiple replicas on a single host without built-in support for an ingress.

@dcermak dcermak mentioned this pull request Feb 27, 2023
Merged
@ygalblum
Copy link
Collaborator

I think we have effectively dropped support for this feature with this PR, as we deemed it to not really make a lot of sense to offer multiple replicas on a single host without built-in support for an ingress.

Yes we have. But, Podman does not fail when replica is set to more than 1, it just logs a warning. So, your YAML file will continue to work. Just won't spin multiple replicas

dcermak added a commit to dcermak/podman that referenced this pull request Feb 28, 2023
@dcermak
Clarify that replicas are ignored in kubernetes deployment
6d352dc 
Replicas are ignored since containers#17082

Signed-off-by: Dan Čermák <dcermak@suse.com>
@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 7, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ygalblum ygalblum ygalblum left review comments

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@vrothberg vrothberg vrothberg approved these changes

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

podman play kube fails when a container has a containerPort set and replicas > 1
8 participants
@dcermak @mheon @rhatdan @TomSweeneyRedHat @vrothberg @thedanielenavarra @ygalblum @openshift-merge-robot