many: use changes + tasks for quota group operations #10420
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pedronis
reviewed
Jun 17, 2021
anonymouse64
force-pushed
the
feature/quota-groups-the-prequel-spinoff-6
branch
from
144a978
to
763dd01
Compare
This was referenced Jun 21, 2021
|
Unfortunately this is blocked now, after reviewing the spread test failures again, it seems that changes like those in 763dd01 are papering over a real issue. The issue is that we now have a task which gets marked done before it is done doing things. It is okay in the case where we get rebooted in the middle after unlocking the state and come back and re-execute the task, it will not break anything, but we now have race conditions in the following situation (which is what the spread test is doing):
There are a few ways we could solve this, but this is a bit of an unfortunate corner case. Options I can think of right now:
|
pedronis
force-pushed
the
feature/quota-groups-the-prequel-spinoff-6
branch
from
763dd01
to
cc3c683
Compare
|
This is now rebased on #10458 which should solve the issues we had with the spread tests. At this point the two last commits cancel each other, I have both to keep history but could also be dropped. |
The exported methods from the servicestate package all return tasksets now, which is expected to be put into changes that are executed by the overlord loop. This involves changes in many parts that use quotas, such as the tests, where a new mock function to create a quota group in state without running any servicectl commands as well as changes to the client side. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
These checks are also race conditions too Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
if the quota-control tasks don't set themselves as done prematurely
pedronis
force-pushed
the
feature/quota-groups-the-prequel-spinoff-6
branch
from
cc3c683
to
b574b07
Compare
|
Conflict detection is implemented in #10471 based on this one |
pedronis
reviewed
Jun 30, 2021
Comment on lines
+102
to
+107
| return nil, fmt.Errorf("group %q already exists", name) | ||
| } | ||
|
|
||
| if memoryLimit == 0 { | ||
| return nil, fmt.Errorf("cannot create quota group with no memory limit set") | ||
| } |
There was a problem hiding this comment.
these new precond checks are not tested
| // oom killer to be invoked when a new group is added as a sub-group to the | ||
| // larger group. | ||
| if memoryLimit <= 4*quantity.SizeKiB { | ||
| return nil, fmt.Errorf("memory limit for group %q is too small: size must be larger than 4KB", name) |
| // executing this directly | ||
| // make sure the specified snaps exist and aren't currently in another group | ||
| if err := validateSnapForAddingToGroup(st, snaps, name, allGrps); err != nil { | ||
| return nil, err |
Comment on lines
161
to
167
| return nil, fmt.Errorf("cannot remove non-existent quota group %q", name) | ||
| } | ||
|
|
||
| // XXX: remove this limitation eventually | ||
| if len(grp.SubGroups) != 0 { | ||
| return nil, fmt.Errorf("cannot remove quota group with sub-groups, remove the sub-groups first") | ||
| } |
| // now ensure that all of the snaps mentioned in AddSnaps exist as snaps and | ||
| // that they aren't already in an existing quota group | ||
| if err := validateSnapForAddingToGroup(st, updateOpts.AddSnaps, name, allGrps); err != nil { | ||
| return nil, err |
stolowski
approved these changes
Jul 1, 2021
| // EnsureSnapServices, see comment in ensureSnapServicesForGroup for | ||
| // full details | ||
| if updateOpts.NewMemoryLimit < grp.MemoryLimit { | ||
| return nil, fmt.Errorf("cannot decrease memory limit of existing quota-group, remove and re-create it to decrease the limit") |
There was a problem hiding this comment.
AFAICT this case isn't covered either.
There was a problem hiding this comment.
I think I now pushed tests that cover most of these
stolowski
approved these changes
Jul 1, 2021
anonymouse64
commented
Jul 1, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The exported methods from the servicestate package all return tasksets now,
which is expected to be put into changes that are executed by the overlord
loop. This involves changes in many parts that use quotas, such as the tests,
where a new mock function to create a quota group in state without running any
servicectl commands as well as changes to the client side.