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

cephadm: add --container-cli-args parameter #42671

Closed
wants to merge 2 commits into from

Conversation

guits
Copy link
Contributor

@guits guits commented Aug 5, 2021

So we can add any extra parameters to any calls to podman run executed
by cephadm.

Fixes: https://tracker.ceph.com/issues/52065

Signed-off-by: Guillaume Abrioux gabrioux@redhat.com

@github-actions
Copy link

github-actions bot commented Aug 5, 2021

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

src/cephadm/cephadm Outdated Show resolved Hide resolved
src/cephadm/cephadm Outdated Show resolved Hide resolved
@guits
Copy link
Contributor Author

guits commented Aug 5, 2021

jenkins test make check

src/cephadm/cephadm Outdated Show resolved Hide resolved
Copy link

@Daniel-Pivonka Daniel-Pivonka left a comment

Choose a reason for hiding this comment

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

this all looks goods and is working as expected. you can pass a flag into bootstrap and it will add that flag to all unit.run file so the podman run uses that flag. the original problem you tried to solve in #42521 doesnt seemed to be solved though. podman run has no verrify-tls flag that i could figure out. so although this is all good and functional you still dont have a way to pull images from http.

@guits
Copy link
Contributor Author

guits commented Aug 6, 2021

the original problem you tried to solve in #42521 doesnt seemed to be solved though. podman run has no verrify-tls flag that i could figure out.

That's why I opened containers/podman#11129 @Daniel-Pivonka

@sebastian-philipp
Copy link
Contributor

See sebastian-philipp/registries-conf-ctl#6 for an alternative to this one

@guits guits changed the title cephadm: add --container_cli_args parameter cephadm: add --container-cli-args parameter Aug 12, 2021
@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@guits
Copy link
Contributor Author

guits commented Sep 21, 2021

jenkins test dashboard cephadm

qa/tasks/cephadm.py Outdated Show resolved Hide resolved
src/cephadm/cephadm Show resolved Hide resolved
@guits guits force-pushed the guits-container-extra-opts branch 4 times, most recently from e43fe2b to 99cf1f9 Compare October 8, 2021 06:57
@guits
Copy link
Contributor Author

guits commented Oct 13, 2021

jenkins test make check

doc/man/8/cephadm.rst Outdated Show resolved Hide resolved
doc/man/8/cephadm.rst Show resolved Hide resolved
parser_deploy.add_argument(
'--allow-ptrace',
action='store_true',
help='Allow SYS_PTRACE on daemon container')
Copy link
Member

Choose a reason for hiding this comment

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

This removes an option (--allow-ptrace) without deprecating it. If we cherry-pick this to pacific, it will destabilize users who might have relied on this.

What if you changed this to help=argparse.SUPPRESS and printed a warning to the user instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ktdreyer do you suggest I should drop this commit entirely?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok to drop it in Ceph's Quincy release.

If you're cherry-picking this change to the pacific branch, it's a good idea to not cherry-pick this removal to that stable branch. For example, I think Red Hat has existing documentation that refers to --allow-ptrace right?

@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

So we can add any extra parameters to any calls to `podman run` executed
by cephadm.

Fixes: https://tracker.ceph.com/issues/52065

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
`--container_cli_args` which is more generic allows to set that kind of
parameter.
Since there's no need to have a dedicated option for this, let's drop
this one.

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
@guits guits force-pushed the guits-container-extra-opts branch from b6bf048 to 5098b26 Compare March 7, 2022 08:55
@guits guits requested a review from a team as a code owner March 7, 2022 08:55
@djgalloway djgalloway changed the base branch from master to main July 2, 2022 00:00
@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Dec 6, 2022
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

@github-actions github-actions bot closed this Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants