plugins: improve `remove` command to support multiple plugins #17030

yaauie · 2025-02-06T00:51:56Z

Release notes

The plugin manager bin/logstash-plugin's remove command now allows multiple plugins to be removed in a single call. This simplifies removing plugins that depend on other plugins, so that the caller no longer needs to perform the removals in dependency order.

What does this PR do?

Removal works by optimistically removing all specified plugin-gems from a mutable gemspec, then validating that doing so does not leave plugin-gems with orphaned runtime dependencies, saving the result and succeeding only if no conflicts were created.

╭─{ rye@perhaps:~/src/elastic/logstash@main (pluginmanager-remove-multiple ✘) }
╰─● bin/logstash-plugin remove logstash-input-syslog logstash-filter-grok
Using system java: /Users/rye/.jenv/shims/java
Resolving dependencies......
Successfully removed logstash-input-syslog
Successfully removed logstash-filter-grok
[success (00:00:05)]

Why is it important/What is the impact to the user?

Because the plugin manager (rightly) refuses to remove plugins that are depended on by other plugins and the plugin manager has historically only allowed one plugin to be removed at a time, the caller removing many plugins has needed to be aware of dependency order.

By allowing a list of plugins to be provided, the plugin manager is able to remove the requirement for dependency-order, and is able to handle the removals in a single execution.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~[ ] I have made corresponding changes to the documentation~~
~~[ ] I have made corresponding change to the default configuration files (and/or docker env variables)~~
I have added tests that prove my fix is effective or that my feature works

Author's Checklist

TODO: file separate docs issue/PR

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

donoghuc

I know you are still working on some tests, had a couple suggestions in the mean time :)

lib/pluginmanager/bundler/logstash_uninstall.rb

donoghuc · 2025-02-07T17:42:41Z

Refactors based on initial review look great!

Removal works in a single pass by finding plugins that would have unmet dependencies if all of the specified plugins were to be removed, and proceeding with the removal only if no conflicts were created. > ~~~ > ╭─{ rye@perhaps:~/src/elastic/logstash@main (pluginmanager-remove-multiple ✘) } > ╰─● bin/logstash-plugin remove logstash-input-syslog logstash-filter-grok > Using system java: /Users/rye/.jenv/shims/java > Resolving dependencies...... > Successfully removed logstash-input-syslog > Successfully removed logstash-filter-grok > [success (00:00:05)] ~~~

elastic-sonarqube · 2025-02-18T17:04:49Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

elasticmachine · 2025-02-18T17:13:21Z

💚 Build Succeeded

Buildkite Build
Commit: 3e00a74

History

💔 Build #2319 failed f6ea649
💚 Build #2230 succeeded 334cb55
💚 Build #2221 succeeded 76a2f43

yaauie

@donoghuc I've requested re-review, and the diff isn't pretty so it's best to just look at the result of the changes in lib/pluginmanager/bundler/logstash_uninstall.rb instead of trying to map what changed.

yaauie · 2025-02-18T17:25:00Z

lib/pluginmanager/bundler/logstash_uninstall.rb

+      unsatisfied_dependency_mapping = Dsl.evaluate(gemfile_path, lockfile_path, {}).specs.each_with_object({}) do |spec, memo|
+        next if gems_to_remove.include?(spec.name)
+        deps = spec.runtime_dependencies.collect(&:name)
+        deps.intersection(gems_to_remove).each do |missing_dependency|
+          memo[missing_dependency] ||= []
+          memo[missing_dependency] << spec.name
        end
      end
-    end
+      if unsatisfied_dependency_mapping.any?
+        unsatisfied_dependency_mapping.each { |gem_to_remove, gems_depending_on_removed| display_cant_remove_message(gem_to_remove, gems_depending_on_removed) }
+        LogStash::PluginManager.ui.info("No plugins were removed.")
+        return false
+      end


I gave up on the pre-validation "optimistic removal" approach.

Bundler did not like being told to materialize a dependency graph that had missing dependencies (e.g., the gems we had optimistically removed), which prevented us from finding the conflicts to present them in a way consistent with how we have previously done.

Instead, this approach is a lot closer to the original. It looks at the existing dependency graph to identify which dependencies would be unmet before removing them from the gemfile.

donoghuc

I think this works well. I like the optimistic approach for removal. I dont think this introduces an issue with orphaned dependencies but perhaps it creates a larger surface area for that? I see you have a PR for a clean routine as well.

Note: I realize I seem to have conflated "optimistic" #17030 (comment) I assumed that meant doing the traversal upon validation list with the complete set of gems to be removed and only committing the change if no conflicts are detected.

yaauie · 2025-02-19T19:17:44Z

@logstashmachine backport 9.0

yaauie · 2025-02-19T19:17:56Z

@logstashmachine backport 8.x

Removal works in a single pass by finding plugins that would have unmet dependencies if all of the specified plugins were to be removed, and proceeding with the removal only if no conflicts were created. > ~~~ > ╭─{ rye@perhaps:~/src/elastic/logstash@main (pluginmanager-remove-multiple ✘) } > ╰─● bin/logstash-plugin remove logstash-input-syslog logstash-filter-grok > Using system java: /Users/rye/.jenv/shims/java > Resolving dependencies...... > Successfully removed logstash-input-syslog > Successfully removed logstash-filter-grok > [success (00:00:05)] ~~~ (cherry picked from commit 0895588)

This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See elastic#17030 for change.

#17120) Removal works in a single pass by finding plugins that would have unmet dependencies if all of the specified plugins were to be removed, and proceeding with the removal only if no conflicts were created. > ~~~ > ╭─{ rye@perhaps:~/src/elastic/logstash@main (pluginmanager-remove-multiple ✘) } > ╰─● bin/logstash-plugin remove logstash-input-syslog logstash-filter-grok > Using system java: /Users/rye/.jenv/shims/java > Resolving dependencies...... > Successfully removed logstash-input-syslog > Successfully removed logstash-filter-grok > [success (00:00:05)] ~~~ (cherry picked from commit 0895588) Co-authored-by: Ry Biesemeyer <[email protected]>

#17121) Removal works in a single pass by finding plugins that would have unmet dependencies if all of the specified plugins were to be removed, and proceeding with the removal only if no conflicts were created. > ~~~ > ╭─{ rye@perhaps:~/src/elastic/logstash@main (pluginmanager-remove-multiple ✘) } > ╰─● bin/logstash-plugin remove logstash-input-syslog logstash-filter-grok > Using system java: /Users/rye/.jenv/shims/java > Resolving dependencies...... > Successfully removed logstash-input-syslog > Successfully removed logstash-filter-grok > [success (00:00:05)] ~~~ (cherry picked from commit 0895588) Co-authored-by: Ry Biesemeyer <[email protected]>

This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See #17030 for change.

This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See #17030 for change. (cherry picked from commit e094054)

…17123) This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See #17030 for change. (cherry picked from commit e094054) Co-authored-by: Cas Donoghue <[email protected]>

This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See elastic#17030 for change.

This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See #17030 for change.

This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See #17030 for change. (cherry picked from commit e8e24a0)

This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See elastic#17030 for change.

…7129) This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See #17030 for change. (cherry picked from commit e8e24a0) Co-authored-by: Cas Donoghue <[email protected]>

…7122) This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See elastic#17030 for change.

…7131) This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See #17030 for change. Co-authored-by: Cas Donoghue <[email protected]>

yaauie added the enhancement label Feb 6, 2025

donoghuc reviewed Feb 6, 2025

View reviewed changes

yaauie force-pushed the pluginmanager-remove-multiple branch from 334cb55 to f6ea649 Compare February 14, 2025 19:48

yaauie force-pushed the pluginmanager-remove-multiple branch from f6ea649 to 3e00a74 Compare February 18, 2025 16:50

yaauie requested a review from donoghuc February 18, 2025 17:17

yaauie commented Feb 18, 2025

View reviewed changes

donoghuc self-assigned this Feb 18, 2025

donoghuc approved these changes Feb 18, 2025

View reviewed changes

yaauie merged commit 0895588 into elastic:main Feb 19, 2025
7 checks passed

yaauie deleted the pluginmanager-remove-multiple branch February 19, 2025 19:17

github-actions bot added the v9.0.0 label Feb 19, 2025

github-actions bot mentioned this pull request Feb 19, 2025

Backport PR #17030 to 9.0: plugins: improve remove command to support multiple plugins #17120

Merged

4 tasks

github-actions bot added the v8.19.0 label Feb 19, 2025

github-actions bot mentioned this pull request Feb 19, 2025

Backport PR #17030 to 8.x: plugins: improve remove command to support multiple plugins #17121

Merged

4 tasks

donoghuc added a commit to donoghuc/logstash that referenced this pull request Feb 19, 2025

Fix acceptance test assertions for updated plugin remove

e33665d

This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See elastic#17030 for change.

donoghuc mentioned this pull request Feb 19, 2025

Fix acceptance test assertions for updated plugin remove #17122

Merged

1 task

yaauie pushed a commit that referenced this pull request Feb 19, 2025

Fix acceptance test assertions for updated plugin remove (#17122)

e094054

This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See #17030 for change.

github-actions bot mentioned this pull request Feb 19, 2025

Backport PR #17122 to 9.0: Fix acceptance test assertions for updated plugin remove #17123

Merged

1 task

donoghuc added a commit to donoghuc/logstash that referenced this pull request Feb 20, 2025

Fix acceptance test assertions for updated plugin remove

01ad2ef

This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See elastic#17030 for change.

donoghuc mentioned this pull request Feb 20, 2025

Fix acceptance test assertions for updated plugin remove #17126

Merged

1 task

donoghuc added a commit that referenced this pull request Feb 20, 2025

Fix acceptance test assertions for updated plugin remove (#17126)

e8e24a0

This commit updates the acceptance tests to expect messages in the updated format for removing plugins. See #17030 for change.

github-actions bot mentioned this pull request Feb 20, 2025

Backport PR #17126 to 9.0: Fix acceptance test assertions for updated plugin remove #17129

Merged

1 task

yaauie mentioned this pull request Feb 20, 2025

Backport PR #171226 to 8.x: Fix acceptance test assertions for updated plugin remove #17131

Merged

1 task

donoghuc mentioned this pull request Feb 20, 2025

Backport PR #171222 to 8.x: Fix acceptance test assertions for updated plugin remove #17132

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plugins: improve `remove` command to support multiple plugins #17030

plugins: improve `remove` command to support multiple plugins #17030

yaauie commented Feb 6, 2025

donoghuc left a comment

donoghuc commented Feb 7, 2025

elastic-sonarqube bot commented Feb 18, 2025

elasticmachine commented Feb 18, 2025

yaauie left a comment

yaauie Feb 18, 2025

donoghuc left a comment •

edited

Loading

yaauie commented Feb 19, 2025

yaauie commented Feb 19, 2025

plugins: improve remove command to support multiple plugins #17030

plugins: improve remove command to support multiple plugins #17030

Conversation

yaauie commented Feb 6, 2025

Release notes

What does this PR do?

Why is it important/What is the impact to the user?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

donoghuc left a comment

Choose a reason for hiding this comment

donoghuc commented Feb 7, 2025

elastic-sonarqube bot commented Feb 18, 2025

Quality Gate passed

elasticmachine commented Feb 18, 2025

💚 Build Succeeded

History

yaauie left a comment

Choose a reason for hiding this comment

yaauie Feb 18, 2025

Choose a reason for hiding this comment

donoghuc left a comment • edited Loading

Choose a reason for hiding this comment

yaauie commented Feb 19, 2025

yaauie commented Feb 19, 2025

plugins: improve `remove` command to support multiple plugins #17030

plugins: improve `remove` command to support multiple plugins #17030

donoghuc left a comment •

edited

Loading