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

Fix device limitations in podman-remote update on remote systems #24760

Merged
merged 1 commit into from
Dec 18, 2024
Merged

Fix device limitations in podman-remote update on remote systems #24760

merged 1 commit into from
Dec 18, 2024
+277 −50

Conversation

Honny1
Copy link
Member

@Honny1 Honny1 commented Dec 4, 2024 •
edited
Loading

This PR fixes device limitations using podman-remote update on remote systems. The cause of the problem was that the major and minor numbers were loaded on the client system and then sent to the remote system, where there could be different major and minor numbers for each device.

Reproducer in Issue: #24734

Fixes: https://issues.redhat.com/browse/RUN-2381
Fixes: #24734

Does this PR introduce a user-facing change?

Fix device limitations in `podman-remote update` on remote systems

@openshift-ci openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Dec 4, 2024
@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Dec 4, 2024
@Honny1 Honny1 force-pushed the fix-major-minor-nums branch from f3d1418 to d56f8d4 Compare December 4, 2024 18:50
@Honny1 Honny1 changed the title Fix loading of major and minor numbers for remote systems Fix device limitations in podman-remote update on remote systems Dec 5, 2024
@Honny1 Honny1 force-pushed the fix-major-minor-nums branch from d56f8d4 to b775b6c Compare December 5, 2024 10:13
@Honny1 Honny1 marked this pull request as ready for review December 5, 2024 10:22
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2024
Luap99
Luap99 requested changes Dec 5, 2024
View reviewed changes
pkg/domain/entities/types/containers.go Outdated
Comment on lines 38 to 59
type ContainerUpdateOptions struct {
NameOrID string
Specgen *specgen.SpecGenerator
Resources *specs.LinuxResources
DevicesLimits *define.UpdateContainerDevicesLimits
ChangedHealthCheckConfiguration *define.UpdateHealthCheckConfig
RestartPolicy *string
RestartRetries *uint
Copy link
Member

Choose a reason for hiding this comment

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

This type is used in pkg/bindings/containers Update() function as such we cannot chnage the type as this will break bindings users, we can add new fields but we cannot remove the function of old fields.

@Honny1 Honny1 force-pushed the fix-major-minor-nums branch from b775b6c to bd14dd2 Compare December 5, 2024 14:56
@Honny1 Honny1 requested a review from Luap99 December 5, 2024 16:24
Luap99
Luap99 reviewed Dec 6, 2024
View reviewed changes
pkg/domain/entities/types/containers.go Outdated
Comment on lines 40 to 59
NameOrID string
// Deprecated: Specgen should not be used to change the container configuration.
// Specgen is processed first, so values will be overwritten by values from other fields.
// To change configuration use other fields:
// Resources to change resource configuration, DevicesLimits to Limit device,
// RestartPolicy to change restart policy and RestartRetries to change restart retries.
Specgen *specgen.SpecGenerator
Resources *specs.LinuxResources
DevicesLimits *define.UpdateContainerDevicesLimits
ChangedHealthCheckConfiguration *define.UpdateHealthCheckConfig
RestartPolicy *string
RestartRetries *uint
Copy link
Member

Choose a reason for hiding this comment

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

@mheon PTAL I never liked using specgen here as it is totally unclear what field are or are not supported so I am fine with this but I am not sure if have objects?

Copy link
Member

Choose a reason for hiding this comment

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

We're not actually using the full thing, so passing a full Specgen feels deceptive. A lot easier to say what we actually support like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the doc string with the list of fields that are currently used from SpecGenerator struct.

pkg/domain/entities/types/containers.go
ChangedHealthCheckConfiguration *define.UpdateHealthCheckConfig
RestartPolicy *string
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to define this *string? It seems pretty clear to me that string should work fine too. Just leave the string empty if it should not be changed sounds easy enough. I don't see what the extra pointer offers here? Allowing user to set the restart policy to an empty string seems pointless as this is not valid?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sure an empty string would work, but the intent was to be consistent in the representation of the unset value throughout the structure.

@Honny1 Honny1 force-pushed the fix-major-minor-nums branch from bd14dd2 to c515b67 Compare December 9, 2024 14:48
@Honny1 Honny1 requested a review from Luap99 December 9, 2024 14:50
@Honny1 Honny1 force-pushed the fix-major-minor-nums branch from c515b67 to 2f31a61 Compare December 9, 2024 16:36
@mheon
Copy link
Member

mheon commented Dec 9, 2024

Changes LGTM

@rhatdan
Copy link
Member

rhatdan commented Dec 17, 2024

@Honny1 @Luap99 what is the state of this PR?

@Honny1
Copy link
Member Author

Honny1 commented Dec 18, 2024

@rhatdan PR is waiting for a review from @Luap99. @Luap99 is on PTO.

@rhatdan
Copy link
Member

rhatdan commented Dec 18, 2024

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Honny1, rhatdan

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 Dec 18, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 7ba410a into containers:main Dec 18, 2024
80 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mheon mheon mheon left review comments

@Luap99 Luap99 Awaiting requested review from Luap99

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny lgtm Indicates that a PR is ready to be merged. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

podman update: limit device on remote system doesn't work
4 participants
@Honny1 @mheon @rhatdan @Luap99