Skip to content
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

(SUP-4215) Add creates param to archive resource #81

Merged
merged 1 commit into from
May 5, 2023

Conversation

m0dular
Copy link
Contributor

@m0dular m0dular commented May 2, 2023

The new version of this module, 6.1.2, includes a fix for ensuring that the cleanup of the archive happens. However, this introduces an idempotency issue where it will always download and cleanup the archive.

@m0dular m0dular requested a review from a team as a code owner May 2, 2023 17:12
@m0dular m0dular force-pushed the SUP-4215-archive_version branch from 12fb44d to 4e37d29 Compare May 2, 2023 17:52
@bastelfreak
Copy link
Collaborator

can you tell me where the idempotency issue is? I am not aware of any issues in our archive module.

@m0dular m0dular force-pushed the SUP-4215-archive_version branch from 4e37d29 to e952bea Compare May 2, 2023 18:07
The new version of this module, 6.1.2, includes a fix for ensuring that
the cleanup of the archive happens.  However, this introduces an
idempotency issue unless the creates param is used.
@m0dular m0dular force-pushed the SUP-4215-archive_version branch from e952bea to b7df455 Compare May 3, 2023 16:51
@m0dular
Copy link
Contributor Author

m0dular commented May 3, 2023

It ended up being that we were not using the creates attribute, which only surfaced because of the recent cleanup fix. I added it and it works 👍

@m0dular m0dular changed the title (SUP-4215) Pin archive module to 6.1.1 (SUP-4215) Add creates param to archive resource May 3, 2023
@@ -177,6 +177,7 @@
extract => true,
extract_command => 'tar xfz %s --strip-components=1',
extract_path => '/opt/influxdb',
creates => '/opt/influxdb/influxd',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you will have trouble when updating influxdb. The path doesn't contain the version number. When you update the version of influxdb, the creates path won't change and archive won't download the new version. In our prometheus module we always add the version to the path. Adding the version number will also provide a local history of files (when you don't clean it up), which makes rollbacks easier. I think that's something that should be changed in another PR. Vox Pupuli example: https://github.com/voxpupuli/puppet-prometheus/blob/master/manifests/daemon.pp#L108-L134

which will create something like this for the node exporter:

root@voxpupu ~ # tree /opt/node_exporter-1.*
/opt/node_exporter-1.3.1.linux-amd64
├── LICENSE
├── NOTICE
└── node_exporter
/opt/node_exporter-1.4.0.linux-amd64
├── LICENSE
├── NOTICE
└── node_exporter

0 directories, 3 files
root@voxpupu ~ # ls -la /usr/local/bin/node_exporter
lrwxrwxrwx 1 root root 50 Oct 10  2022 /usr/local/bin/node_exporter -> /opt/node_exporter-1.4.0.linux-amd64/node_exporter
root@voxpupu ~ #

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I was wondering how upgrades worked with archive sources. Essentially, there should be:

  • A versioned directory that the archive is extracted to, e.g. /opt/influxdb/influxd-2.6.1
  • A symlink for the main influxd binary that points to the latest archive directory, e.g. /usr/local/bin/influxd -> /opt/influxdb/influxd-2.6.1/influxd

Is that about right? I think we would need to make the startup script into a template so that it picks up the right binary. Maybe we should mark this "do not merge" until that is added, or just make a new PR with all of the changes? Although, I'm not sure if upgrading with archive sources works currently unless you delete /opt/influxdb first.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should merge this PR, because it fixes the idempotency problem/works with newer puppet/archive version. Fixing the upgrade problem is a different issue. I would not make the startup script a template. When you add the symlink that's fine because /usr/local/bin is usually in $PATH. To be sure, you can update PATH="/opt/influxdb:${PATH}" to PATH="/usr/local/bin:${PATH}" in the script.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, if someone can approve it I'll merge this PR.

@MartyEwings MartyEwings merged commit 72cc2dd into puppetlabs:main May 5, 2023
@m0dular m0dular added the bug Something isn't working label May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants