-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix(quadlet): Ensure .app application file is removed by quadlet rm --all (#27647) #27694
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -835,12 +835,18 @@ func (ic *ContainerEngine) QuadletRemove(ctx context.Context, quadlets []string, | |
| Errors: make(map[string]error), | ||
| Removed: []string{}, | ||
| } | ||
| removeList := []string{} | ||
| reverseMap, appMap, err := buildAppMap(systemdquadlet.GetInstallUnitDirPath(rootless.IsRootless())) | ||
|
|
||
| installDir := systemdquadlet.GetInstallUnitDirPath(rootless.IsRootless()) | ||
|
|
||
| reverseMap, appMap, err := buildAppMap(installDir) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to build app map: %w", err) | ||
| } | ||
| expandQuadletList := []string{} | ||
|
|
||
| // Use sets for deduplication | ||
| removeSet := make(map[string]struct{}) | ||
| expandQuadletSet := make(map[string]struct{}) | ||
|
|
||
| // Process all `.app` files in arguments, if `.app` file | ||
| // is found then expand it to its respective quadlet files | ||
| // and remove it from the processing list. | ||
|
|
@@ -852,32 +858,35 @@ func (ic *ContainerEngine) QuadletRemove(ctx context.Context, quadlets []string, | |
| if ok { | ||
| for _, file := range files { | ||
| if !systemdquadlet.IsExtSupported(file) { | ||
| removeList = append(removeList, file) | ||
| removeSet[file] = struct{}{} | ||
| } else { | ||
| expandQuadletList = append(expandQuadletList, file) | ||
| expandQuadletSet[file] = struct{}{} | ||
| } | ||
| } | ||
| } | ||
| // also add .app file itself to the remove list so it can | ||
| // be cleaned after removal of all components in the list | ||
| if !slices.Contains(removeList, quadlet) { | ||
| removeList = append(removeList, quadlet) | ||
| } | ||
| removeSet[quadlet] = struct{}{} | ||
| } else { | ||
| expandQuadletList = append(expandQuadletList, quadlet) | ||
| expandQuadletSet[quadlet] = struct{}{} | ||
| } | ||
| } | ||
| quadlets = expandQuadletList | ||
|
|
||
| if len(quadlets) == 0 && !options.All { | ||
| return nil, errors.New("must provide at least 1 quadlet to remove") | ||
| } | ||
| // Convert expandQuadletSet to slice | ||
| quadlets = make([]string, 0, len(expandQuadletSet)) | ||
|
kavishgr marked this conversation as resolved.
|
||
| for quadlet := range expandQuadletSet { | ||
| quadlets = append(quadlets, quadlet) | ||
| } | ||
|
|
||
| allQuadletPaths := make([]string, 0, len(quadlets)) | ||
| allServiceNames := make([]string, 0, len(quadlets)) | ||
| runningQuadlets := make([]string, 0, len(quadlets)) | ||
| serviceNameToQuadletName := make(map[string]string) | ||
| needReload := options.ReloadSystemd | ||
|
|
||
| if len(quadlets) == 0 && !options.All { | ||
| return nil, errors.New("must provide at least 1 quadlet to remove") | ||
| } | ||
|
|
||
| // Is systemd available to the current user? | ||
| // We cannot proceed if not. | ||
| conn, err := systemd.ConnectToDBUS() | ||
|
|
@@ -888,7 +897,12 @@ func (ic *ContainerEngine) QuadletRemove(ctx context.Context, quadlets []string, | |
|
|
||
| if options.All { | ||
| allQuadlets := getAllQuadletPaths() | ||
| quadlets = allQuadlets | ||
| for _, quadlet := range allQuadlets { | ||
| if _, exists := expandQuadletSet[quadlet]; !exists { | ||
| quadlets = append(quadlets, quadlet) | ||
| expandQuadletSet[quadlet] = struct{}{} | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // We are using index wise iteration here instead of `range` | ||
|
|
@@ -913,30 +927,43 @@ func (ic *ContainerEngine) QuadletRemove(ctx context.Context, quadlets []string, | |
| } | ||
| continue | ||
| } | ||
| value, ok := reverseMap[quadlet] | ||
| // Use base filename for reverseMap lookup since map keys are filenames, not full paths | ||
| quadletBaseName := filepath.Base(quadlet) | ||
| value, ok := reverseMap[quadletBaseName] | ||
| if ok { | ||
| // If this is part of app and we are cleaning entire .app | ||
| // make sure to add .app file itself to the removal list | ||
| // if it does not already exists. | ||
| if !slices.Contains(removeList, value) { | ||
| removeList = append(removeList, value) | ||
| } | ||
| appFilePath := filepath.Join(systemdquadlet.GetInstallUnitDirPath(rootless.IsRootless()), value) | ||
| removeSet[value] = struct{}{} | ||
| appFilePath := filepath.Join(installDir, value) | ||
| filesToRemove, err := getAssetListFromFile(appFilePath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to get list of files to remove: %w", err) | ||
| } | ||
| for _, entry := range filesToRemove { | ||
| if !systemdquadlet.IsExtSupported(entry) { | ||
| removeList = append(removeList, entry) | ||
| if !slices.Contains(removeList, value) { | ||
| // In the last also clean .<quadlet>.app file | ||
| removeList = append(removeList, value) | ||
| } | ||
| removeSet[entry] = struct{}{} | ||
| continue | ||
| } | ||
| if !slices.Contains(quadlets, entry) { | ||
| quadlets = append(quadlets, entry) | ||
| var entryToAdd string | ||
| // Note: We treat --all and specific arguments (e.g. foo.container) | ||
|
kavishgr marked this conversation as resolved.
|
||
| // as mutually exclusive here. The loop that runs to expand | ||
| // .app, adds filenames to expandQuadletSet. While the | ||
| // options.All uses getAllQuadletPaths() to add full paths. | ||
| // The dedup check won't detect these refer to the same file, | ||
| // so a quadlet could be processed twice. | ||
| // Given this is low-risk, --all is highly unlikely to used | ||
| // with explicit .app arguments such as: | ||
| // 'podman quadlet rm --all --force foo.app' | ||
| // Documenting the behavior is preferred over normalizing all entries to full paths. | ||
|
kavishgr marked this conversation as resolved.
|
||
| if options.All { | ||
|
kavishgr marked this conversation as resolved.
|
||
| entryToAdd = filepath.Join(installDir, entry) | ||
| } else { | ||
| entryToAdd = entry | ||
| } | ||
| if _, exists := expandQuadletSet[entryToAdd]; !exists { | ||
| quadlets = append(quadlets, entryToAdd) | ||
| expandQuadletSet[entryToAdd] = struct{}{} | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1012,13 +1039,23 @@ func (ic *ContainerEngine) QuadletRemove(ctx context.Context, quadlets []string, | |
| continue | ||
| } | ||
| } | ||
| for _, entry := range removeList { | ||
| os.Remove(filepath.Join(systemdquadlet.GetInstallUnitDirPath(rootless.IsRootless()), entry)) | ||
| } | ||
| report.Removed = append(report.Removed, quadletName) | ||
| } | ||
| } | ||
|
|
||
| // Remove .app and .asset files after the main quadlet removal loop | ||
| // This ensures they are cleaned up properly since they are not included in allQuadletPaths | ||
| for entry := range removeSet { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not blocking: metadata files in removeSet are cleaned up regardless of whether the associated quadlet removals succeeded. If all quadlets in an app fail to remove (e.g., running without --force), the .app file is still deleted, orphaning them. This was the same behavior before this PR, just noting it as something to potentially address in a follow-up. |
||
| entryPath := filepath.Join(installDir, entry) | ||
| if err := os.Remove(entryPath); err != nil { | ||
| if !errors.Is(err, fs.ErrNotExist) { | ||
| logrus.Warnf("Failed to remove metadata file %s: %v", entry, err) | ||
| } | ||
| } else { | ||
| logrus.Debugf("Removed metadata file %s", entry) | ||
| } | ||
| } | ||
|
|
||
| // Reload systemd, if necessary/requested. | ||
| if needReload { | ||
| if err := conn.ReloadContext(ctx); err != nil { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.