OSD-21419: Fixed console container cleanup issues #386

samanthajayasinghe · 2024-04-02T22:20:27Z

What type of PR is this?

bug

What this PR does / Why we need it?

Fixing the console container cleanup issues for both docker and podman CRI

Which Jira/Github issue(s) does this PR fix?

https://issues.redhat.com/browse/OSD-21419

Pre-checks (if applicable)

Ran unit tests locally
Validated the changes in a cluster
Included documentation changes with PR

openshift-ci-robot · 2024-04-02T22:20:31Z

@samanthajayasinghe: This pull request references OSD-21419 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set.

In response to this:

What type of PR is this?

bug

What this PR does / Why we need it?

Fixing the console container cleanup issues for both docker and podman CRI

Which Jira/Github issue(s) does this PR fix?

https://issues.redhat.com/browse/OSD-21419

Pre-checks (if applicable)

Ran unit tests locally

Validated the changes in a cluster

Included documentation changes with PR

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 openshift-eng/jira-lifecycle-plugin repository.

codecov-commenter · 2024-04-04T03:06:34Z

Codecov Report

Attention: Patch coverage is 4.08163% with 47 lines in your changes are missing coverage. Please review.

Project coverage is 44.50%. Comparing base (fb608a8) to head (13e97c5).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #386      +/-   ##
==========================================
- Coverage   45.74%   44.50%   -1.24%     
==========================================
  Files          62       62              
  Lines        5122     5159      +37     
==========================================
- Hits         2343     2296      -47     
- Misses       2447     2547     +100     
+ Partials      332      316      -16

Files	Coverage Δ
cmd/ocm-backplane/console/console.go	`36.71% <4.08%> (-7.88%)`	⬇️

... and 1 file with indirect coverage changes

xiaoyu74 · 2024-04-07T22:34:42Z

cmd/ocm-backplane/console/console.go

@@ -1309,3 +1365,23 @@ func (ce *dockerLinux) stopContainer(containerName string) error {
 func (ce *dockerMac) stopContainer(containerName string) error {
 return generalStopContainer(DOCKER, containerName)
 }
+
+// podman-exist for Linux


Just my 2 cents. Looks the containerIsExist method is implemented separately for each struct (podmanLinux, podmanMac, dockerLinux, dockerMac), is that a bit redundancy? How about only write the logic for containerIsExist once and defining the container engine string podman, docker as a property of the structs themselves?

type baseContainerEngine struct { engineType string } func (b *baseContainerEngine) containerIsExist(containerName string) (bool, error) { return generalContainerIsExist(b.engineType, containerName) } type podmanLinux struct { baseContainerEngine }

So, in case we were to add another container engine in the future, is this structure more clear?

This is a good suggestion @xiaoyu74 , I feel that we need refactor most of the code to handle proper CRI( container runtime interface) and may belong to this PR . I have seen some more duplicate methods in console and will create a card for that.

xiaoyu74 · 2024-04-07T22:35:34Z

cmd/ocm-backplane/console/console.go

@@ -1023,10 +1053,12 @@ func podmanRunConsoleContainer(containerName string, port string, consoleArgs []
 }
 engRunArgs = append(engRunArgs, consoleArgs...)
 logger.WithField("Command", fmt.Sprintf("`%s %s`", PODMAN, strings.Join(engRunArgs, " "))).Infoln("Running container")
- runCmd := createCommand(PODMAN, engRunArgs...)
+ //runCmd := createCommand(PODMAN, engRunArgs...)


please remove the commented line here

xiaoyu74 · 2024-04-07T22:39:43Z

Great work Sam. I've built the binary with your commit and tested in an OSD 4.15.2 cluster. Following the reproduce steps and your PR can clean-up the console container properly.

xiaoyu74 · 2024-04-07T22:47:43Z

cmd/ocm-backplane/console/console.go

+ if exist {
+ err := ce.stopContainer(c)
+ if err != nil {
+ return fmt.Errorf("failed to stop container %s: %v", c, err)


how about add some more context for the return errors?

failed to stop container '%s' during cleanup process. Ensure the container engine is running and accessible

Update error message, As I noted, %v will throw the detailed exception as to why it failed

xiaoyu74 · 2024-04-07T22:58:32Z

As I can see the overall test coverage for console.go is 39.2%, as discussed before we probably need to do some coverage improvement in backplane maintenance EPIC some time?

samanthajayasinghe · 2024-04-08T02:48:26Z

As I can see the overall test coverage for console.go is 39.2%, as discussed before we probably need to do some coverage improvement in backplane maintenance EPIC some time?

Yeah, We need to change the way CRI executing( create/stop/fetch) to fake it properly. I'll create new card use CRI with podman/docker go libs that may allow us to code proper unit tests

feichashao · 2024-04-08T03:08:23Z

Works perfect in my Podman+MacOS environment.

It can remove 2 containers in a normal case.

INFO[0200] Container removed: monitoring-plugin-2adi24mc34eug59c1jqo8hvhelre7g9a 
INFO[0201] Container removed: console-2adi24mc34eug59c1jqo8hvhelre7g9a

If I kill the monitor container manually, it can remove the left behind console container.

openshift-ci · 2024-04-08T03:38:07Z

@samanthajayasinghe: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

xiaoyu74 · 2024-04-08T03:58:55Z

/lgtm

openshift-ci · 2024-04-08T03:59:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: samanthajayasinghe, xiaoyu74

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~~ [samanthajayasinghe,xiaoyu74]

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

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 2, 2024

openshift-ci bot requested review from supreeth7 and wanghaoran1988 April 2, 2024 22:20

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2024

samanthajayasinghe marked this pull request as draft April 2, 2024 22:44

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 2, 2024

samanthajayasinghe force-pushed the console-container branch from 4169c37 to 1cc4a1e Compare April 4, 2024 03:00

samanthajayasinghe marked this pull request as ready for review April 4, 2024 03:01

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 Apr 4, 2024

openshift-ci bot requested review from hectorakemp and harliu-01 April 4, 2024 03:02

samanthajayasinghe force-pushed the console-container branch 6 times, most recently from a0b4d74 to e58dd8a Compare April 4, 2024 21:10

xiaoyu74 reviewed Apr 7, 2024

View reviewed changes

OSD-21419: Fixed console container cleanup issues

13e97c5

samanthajayasinghe force-pushed the console-container branch from e58dd8a to 13e97c5 Compare April 8, 2024 03:33

openshift-ci bot assigned xiaoyu74 Apr 8, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2024

openshift-merge-bot bot merged commit 6f73ad5 into openshift:main Apr 8, 2024
7 checks passed

xiaoyu74 mentioned this pull request May 16, 2024

Use replace #419

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSD-21419: Fixed console container cleanup issues #386

OSD-21419: Fixed console container cleanup issues #386

samanthajayasinghe commented Apr 2, 2024

openshift-ci-robot commented Apr 2, 2024 •

edited by openshift-ci bot

Loading

What type of PR is this?

What this PR does / Why we need it?

Which Jira/Github issue(s) does this PR fix?

Pre-checks (if applicable)

codecov-commenter commented Apr 4, 2024 •

edited

Loading

xiaoyu74 Apr 7, 2024

samanthajayasinghe Apr 8, 2024

xiaoyu74 Apr 7, 2024

xiaoyu74 commented Apr 7, 2024

xiaoyu74 Apr 7, 2024

samanthajayasinghe Apr 8, 2024

xiaoyu74 commented Apr 7, 2024

samanthajayasinghe commented Apr 8, 2024

feichashao commented Apr 8, 2024

openshift-ci bot commented Apr 8, 2024

xiaoyu74 commented Apr 8, 2024

openshift-ci bot commented Apr 8, 2024

OSD-21419: Fixed console container cleanup issues #386

OSD-21419: Fixed console container cleanup issues #386

Conversation

samanthajayasinghe commented Apr 2, 2024

What type of PR is this?

What this PR does / Why we need it?

Which Jira/Github issue(s) does this PR fix?

Pre-checks (if applicable)

openshift-ci-robot commented Apr 2, 2024 • edited by openshift-ci bot Loading

What type of PR is this?

What this PR does / Why we need it?

Which Jira/Github issue(s) does this PR fix?

Pre-checks (if applicable)

codecov-commenter commented Apr 4, 2024 • edited Loading

Codecov Report

xiaoyu74 Apr 7, 2024

Choose a reason for hiding this comment

samanthajayasinghe Apr 8, 2024

Choose a reason for hiding this comment

xiaoyu74 Apr 7, 2024

Choose a reason for hiding this comment

xiaoyu74 commented Apr 7, 2024

xiaoyu74 Apr 7, 2024

Choose a reason for hiding this comment

samanthajayasinghe Apr 8, 2024

Choose a reason for hiding this comment

xiaoyu74 commented Apr 7, 2024

samanthajayasinghe commented Apr 8, 2024

feichashao commented Apr 8, 2024

openshift-ci bot commented Apr 8, 2024

xiaoyu74 commented Apr 8, 2024

openshift-ci bot commented Apr 8, 2024

openshift-ci-robot commented Apr 2, 2024 •

edited by openshift-ci bot

Loading

codecov-commenter commented Apr 4, 2024 •

edited

Loading