podman system reset removed machines incorrectly #14066
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashley-cui 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 |
openshift-ci
bot
added
the
approved
ashley-cui
force-pushed
the
sysres
branch
2 times, most recently
from
93ca1ca to
09c3c06
Compare
|
Could you change the title when you repush please? correctly -> incorrectly |
ashley-cui
force-pushed
the
sysres
branch
2 times, most recently
from
1ab0158 to
7ee9d36
Compare
ashley-cui
changed the title
ashley-cui
force-pushed
the
sysres
branch
3 times, most recently
from
9897dbc to
588ac1f
Compare
| @@ -1442,3 +1442,57 @@ func (v *MachineVM) getResources() (resources machine.ResourceConfig) { | |||
| resources.DiskSize = getDiskSize(v) | |||
| return | |||
| } | |||
|
|
|||
| // RemoveAndCleanMachines removes all machine and cleans up any other files associatied with podman machine | |||
| func (p *Provider) RemoveAndCleanMachines() error { | |||
There was a problem hiding this comment.
Why is this function logrus.Error rather then returning it? Looks like it never returns an error?
There was a problem hiding this comment.
The function should not error just because one machine failed to be removed, but rather keep going to check and remove more machines. I can either logrus.err it or return a list of errors and also error, but I think logrus is cleaner in this case
There was a problem hiding this comment.
If the function never returns an error, then it should not have error in its core.
I think you should do
var prevError Error
...
if err != nil {
if prevError != nil {
logrus.Errorf(prevErr)
}
prevError = err
}
...
Return prevError
There was a problem hiding this comment.
So the calling function would also just logrus the returned error too?
I updated the function to return an error if it's unable to list machines to remove, so the function will return an error if it fails there, do you think it's still necessary to do what you said?
There was a problem hiding this comment.
It would logrus all errors except for the last error. So if only one error, then no logging. If two errors logus.Error First Error, return second.
podman system reset did not clean up machines fully, leaving some config files, and breaking machines. Now it removes all machines files fully. Signed-off-by: Ashley Cui <[email protected]>
|
LGTM |
|
/lgtm |
|
/cherry-pick v4.1 |
|
@ashley-cui: new pull request created: #14117 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
github-actions
bot
added
the
locked - please file new issue/PR
podman system reset did not clean up machines fully, leaving some config
files, and breaking machines. Now it removes all machines files fully.
Signed-off-by: Ashley Cui [email protected]
Does this PR introduce a user-facing change?