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

Support size options on builtin volumes #10973

Merged
merged 1 commit into from
Aug 4, 2021
Merged

Support size options on builtin volumes #10973

merged 1 commit into from
Aug 4, 2021

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Jul 17, 2021

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 17, 2021
@rhatdan
Copy link
Member Author

rhatdan commented Jul 17, 2021

@mheon PTAL

@mheon
Copy link
Member

mheon commented Jul 18, 2021

Nice and simple, LGTM

Luap99
Luap99 reviewed Jul 19, 2021
View reviewed changes
libpod/volume.go Outdated
@@ -49,6 +49,8 @@ type VolumeConfig struct {
UID int `json:"uid"`
// GID the volume will be created as.
GID int `json:"gid"`
// Size maximum of the volume.
Size int64 `json:"gid"`
Copy link
Member

Choose a reason for hiding this comment

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

Should we store this as uint64 given that SetQuota() accepts uint64?
Also the json field name is wrong s/gid/size/.

@rhatdan rhatdan force-pushed the quota branch 2 times, most recently from f381ca5 to af30626 Compare July 19, 2021 12:42
@rhatdan rhatdan mentioned this pull request Jul 27, 2021
Merged
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2021
mtrmac
mtrmac reviewed Jul 30, 2021
View reviewed changes
libpod/runtime_volume_linux.go
Comment on lines +123 to +124
}
if projectQuotaSupported {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
if projectQuotaSupported {

Just like this? We don’t need to set quota (and potentially conflict with XFS project IDs).

Copy link
Collaborator

Choose a reason for hiding this comment

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

don’t need to set quota … if there isn’t a quota request.

Copy link
Collaborator

Choose a reason for hiding this comment

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

… actually, all of this, including the quota support check, can be wrapped inside the volume.config.{Size,Inodes} > 0 condition.

/me steps away to regain focus.

@rhatdan rhatdan changed the title [WIP] Support size options on builtin volumes Support size options on builtin volumes Jul 31, 2021
@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 Jul 31, 2021
@rhatdan
Support size and inode options on builtin volumes
c0952c7 
[NO TESTS NEEDED] Since it is difficult to setup xfs quota

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1982164

Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan
Copy link
Member Author

rhatdan commented Aug 4, 2021

@giuseppe @vrothberg @TomSweeneyRedHat @saschagrunert @baude @jwhonce PTAL

giuseppe
giuseppe approved these changes Aug 4, 2021
View reviewed changes
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

saschagrunert
saschagrunert approved these changes Aug 4, 2021
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 4, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, rhatdan, saschagrunert

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:
  • OWNERS [giuseppe,rhatdan,saschagrunert]

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

@rhatdan rhatdan added the lgtm Indicates that a PR is ready to be merged. label Aug 4, 2021
@openshift-ci openshift-ci bot merged commit 8aa869e into containers:main Aug 4, 2021
@TomSweeneyRedHat
Copy link
Member

@mheon @rhatdan, unless I missed it, it doesn't look like this was cherry-picked into the v3.3 branch. Can we make that so, so we get this bug fixed in RHEL 8.5?

@mheon
Copy link
Member

mheon commented Aug 9, 2021

We're feature-frozen at this point, and this counts as a feature. Will that cause us problems?

@TomSweeneyRedHat
Copy link
Member

@mheon it has an associated BZ, so I think it's fair game.

@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 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@mtrmac mtrmac mtrmac left review comments

@saschagrunert saschagrunert saschagrunert approved these changes

@giuseppe giuseppe giuseppe approved these changes

Assignees
No one assigned
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rhatdan @mheon @TomSweeneyRedHat @giuseppe @saschagrunert @mtrmac @Luap99