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

Make the pidfile accessible by everyone #3252

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Make the pidfile accessible by everyone #3252

wants to merge 17 commits into from

Conversation

mcasquer
Copy link
Collaborator

@mcasquer mcasquer commented Oct 1, 2024 •
edited
Loading

Creates the pidfile at an accessible location
for every user, this way the manage pidfile
warning is avoided.

Pull Request Checklist

  • implement the feature
  • write the documentation
  • extend the test coverage
  • update the specification
  • mention the version
  • include a release note

@mcasquer mcasquer marked this pull request as ready for review October 1, 2024 11:27
@mcasquer
Copy link
Collaborator Author

mcasquer commented Oct 1, 2024

To discuss here which of the added # noqa could be avoided

happz
happz reviewed Oct 1, 2024
View reviewed changes
@mcasquer mcasquer force-pushed the 2877_pidfile_creation branch 3 times, most recently from 22ff571 to f5768f3 Compare October 11, 2024 10:23
@psss psss added this to the 1.38 milestone Oct 17, 2024
@mcasquer mcasquer force-pushed the 2877_pidfile_creation branch 2 times, most recently from 83819a0 to d2dafc9 Compare October 22, 2024 09:17
@happz happz added the ci | full test Pull request is ready for the full test execution label Oct 22, 2024
@mcasquer mcasquer force-pushed the 2877_pidfile_creation branch from d2dafc9 to acb4820 Compare October 22, 2024 09:50
@mcasquer mcasquer force-pushed the 2877_pidfile_creation branch 2 times, most recently from d45af53 to 0dc6e94 Compare October 24, 2024 12:15
@thrix thrix modified the milestones: 1.38, 1.39 Oct 25, 2024
@psss psss changed the title Makes the pidfile accessible by everyone Make the pidfile accessible by everyone Oct 31, 2024
psss
psss reviewed Oct 31, 2024
View reviewed changes
@mcasquer mcasquer force-pushed the 2877_pidfile_creation branch from 0dc6e94 to 124aa95 Compare October 31, 2024 12:29
@psss psss linked an issue Oct 31, 2024 that may be closed by this pull request
Only first user can run provision -h local. Other lack lock file permissions. #2877
Open
@mcasquer
Copy link
Collaborator Author

@happz one question, in the related issue with the patch it's used tmt -a provision -h local which IIUC it's the local host already executing tmt, in this case the code raises again permissions issues when doing the chmod

@happz
Copy link
Collaborator

happz commented Oct 31, 2024

@happz one question, in the related issue with the patch it's used tmt -a provision -h local which IIUC it's the local host already executing tmt, in this case the code raises again permissions issues when doing the chmod

Is your change here causing a regression? Or is your change here just making no difference WRT #2877?

@happz
Copy link
Collaborator

happz commented Feb 18, 2025 •
edited
Loading

@lukaszachy I'd argue #3518 should go in first, because of tests/provision/facts/test-guest.sh changes, and there is also #3483 which is very related and maybe should be a prerequisite of this PR, as mentioned in #3252 (comment)

@lukaszachy lukaszachy removed the status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. label Feb 18, 2025
@lukaszachy lukaszachy modified the milestones: 1.43, 1.44 Feb 25, 2025
@psss psss added the priority | should medium priority, should be included in the next release label Feb 27, 2025
@mcasquer mcasquer force-pushed the 2877_pidfile_creation branch from d50ece9 to 231f7d8 Compare March 3, 2025 08:28
@mcasquer mcasquer removed this from the 1.44 milestone Mar 11, 2025
@mcasquer mcasquer added the status | backlog Defined, prioritized, we plan to work on it. label Mar 11, 2025
@psss psss added this to the 1.45 milestone Mar 13, 2025
@psss psss added priority | must high priority, must be included in the next release and removed priority | should medium priority, should be included in the next release status | backlog Defined, prioritized, we plan to work on it. labels Mar 13, 2025
mcasquer added 17 commits March 18, 2025 08:05
@mcasquer
Makes the pidfile accessible by everyone
961a54c 
Creates the pidfile at an accessible location
for every user, this way the manage pidfile
warning is avoided.

Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Adds a check to only create the pid directory
bad2f9d 
when it doesn't exist.

Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Fixes the tmt reboot test, updating the pidfile location in the scripts.
9da6ffb 
Also formats the pid directory creation command.

Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Updates the environment variables in out-of-session test.
c7a35b1 
This way we avoid TestingFarm to set the old values.

Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Removes the sudo(s) from tmt commands and instead apply it directly
9954358 
in the pidfile creation. Also includes the super().setup() call to
the setup GuestSsh function.

Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Removes the self.become this way the condition is applied
ba01cac 
every time the user has no privileges

Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Changes bfu_* options to use unprivileged Fedora container image
c8e3ff5 
and fedora user

Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Adds sudo to setfacl command
56f3ce6 
Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Changes the setfacl command and adds it to the local workdir_root cre…
68f591e 
…ation too.

Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Adds missing directory in setfacl command
8eb0ea1 
Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Updates ACL set after workdir_root creation
5fda469 
After properly setting the permissions and the
access control list evrything should be world-writable.

Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Fixes issues with workdir setting
b5808fc 
Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Adds acl to be installed along with the tmt runner
cc39947 
Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Simplifies the arguments subprocess structure
eb0ebb1 
Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Changes subprocess by inner Command.run function
a65583f 
Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Removes directory check condition
ababd15 
Since it keeps the directory is directly mounted in the
containers, removing the check should always execute the chmod and
setfacl in the workdir directory.

Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer
Updates code according to ruff requirements
d7267f2 
Signed-off-by: mcasquer <mcasquer@redhat.com>
@mcasquer mcasquer force-pushed the 2877_pidfile_creation branch from 231f7d8 to d7267f2 Compare March 18, 2025 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@happz happz happz approved these changes

@martinhoyer martinhoyer martinhoyer approved these changes

@KwisatzHaderach KwisatzHaderach KwisatzHaderach approved these changes

@psss psss psss approved these changes

@lukaszachy lukaszachy Awaiting requested review from lukaszachy lukaszachy is a code owner

@thrix thrix Awaiting requested review from thrix thrix is a code owner

@janhavlin janhavlin Awaiting requested review from janhavlin janhavlin is a code owner

@carlosrodfern carlosrodfern Awaiting requested review from carlosrodfern

Assignees
No one assigned
Labels
ci | full test Pull request is ready for the full test execution command | run tmt run commanda priority | must high priority, must be included in the next release
Projects
None yet
Milestone
1.45
Development

Successfully merging this pull request may close these issues.

Only first user can run provision -h local. Other lack lock file permissions.
8 participants
@mcasquer @happz @psss @carlosrodfern @thrix @KwisatzHaderach @martinhoyer @lukaszachy