ex: dockergc: various fixes #17479
ex: dockergc: various fixes #17479
Conversation
openshift-ci-robot
added
the
size/M
sjenning
changed the title
sjenning
force-pushed
the
issue-17443
branch
from
5827f95 to
b198f06
Compare
contrib/completions/bash/openshift
Outdated
| @@ -429,30 +429,12 @@ _openshift_ex_dockergc() | |||
| flags_with_completion=() | |||
| flags_completion=() | |||
|
|
|||
| flags+=("--dry-run") | |||
| local_nonpersistent_flags+=("--dry-run") | |||
There was a problem hiding this comment.
No. Don't do it. This is a common option.
| break | ||
| } | ||
| fmt.Printf("removing container %v (size: %v, age: %v)\n", c.ID, c.SizeRw, age) | ||
| glog.Infof("removing container %v (size: %v, age: %v)", c.ID, c.SizeRw, age) | ||
| err := client.ContainerRemove(ctx, c.ID, dockertypes.ContainerRemoveOptions{RemoveVolumes: true}) |
There was a problem hiding this comment.
Do not make any real changes if the --dry-run is used.
| break | ||
| } | ||
| fmt.Printf("removing image %v (size: %v, age: %v)\n", i.ID, i.Size, age) | ||
| glog.Infof("removing image %v (size: %v, age: %v)", i.ID, i.Size, age) | ||
| _, err := client.ImageRemove(ctx, i.ID, dockertypes.ImageRemoveOptions{PruneChildren: true}) |
There was a problem hiding this comment.
Do not make any real changes if the --dry-run is used.
openshift-ci-robot
added
size/L
|
@legionus ok I added the dry-run flag back. It runs in a single-pass mode with no effect. I also noticed that I had completely misunderstood how contexts work. I've added a commit to fix that. |
|
/retest |
1 similar comment
|
/retest |
sjenning
changed the title
|
/retest |
| ctx, cancel := c.getTimeoutContext() | ||
| defer cancel() | ||
| containers, err := c.client.ContainerList(ctx, options) | ||
| if ctx.Err() == context.DeadlineExceeded { |
There was a problem hiding this comment.
Why do you need this additional check?
There was a problem hiding this comment.
There are two places an error can happen: the ctx can timeout or the client operation an error. We just need to check both. AFAICT DeadlineExceeded and Canceled are the only possible context errors. Could probably just change this to ctx.Err() != nil, but since there is no way for the context to be cancelled here since the call to cancel() is a defer for this function, they are functionally equivalent.
Since all tests are green, I'd like to avoid non-functional changes if we can. This ok?
There was a problem hiding this comment.
If the context is cancelled/timed out, ContainerList should return an error. Though, if it isn't, why do you want to abandon the result in this case?
There was a problem hiding this comment.
fair enough. updated.
There was a problem hiding this comment.
func clientErr(ctx context.Context, err error) error {
if ctx.Err() != nil {
return ctx.Err()
}
return err
}@sjenning You move an error handling to this function, it checks ctx.Err() first even if err == nil in the function arguments. I do not see any changes in the old logic.
|
@legionus look good to you now? |
|
/approve |
openshift-merge-robot
added
approved
LGTM but I think that @dmage's comments should be addressed. |
sjenning
force-pushed
the
issue-17443
branch
2 times, most recently
from
8dc6205 to
5b70b9f
Compare
openshift-merge-robot
added
needs-rebase
sjenning
force-pushed
the
issue-17443
branch
from
5b70b9f to
d5227c1
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@derekwaynecarr can I get lgtm? |
sjenning
force-pushed
the
issue-17443
branch
from
d5227c1 to
3547f93
Compare
|
flake #17698 |
|
opened e2e flake #17721 |
|
/test end_to_end |
|
/lgtm |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: derekwaynecarr, eparis, sjenning The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Automatic merge from submit-queue (batch tested with PRs 17932, 18037, 17479, 18051, 18052). |
Fixes #17443
xref https://bugzilla.redhat.com/show_bug.cgi?id=1532966
@legionus @dmage