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

build: update s5cmd to version v2.2.1 #12898

Merged
merged 1 commit into from
Sep 15, 2023
Merged

Conversation

obnoxxx
Copy link
Contributor

@obnoxxx obnoxxx commented Sep 13, 2023

This is the latest available version.
The previously used version v2.0.0 was very outdated.

Fixes: #12883

Description of your changes:

This updates the version of s5cmd built into the container(s) to the latest available version v2.2.1

Which issue is resolved by this Pull Request:
Resolves #12883

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@obnoxxx
Copy link
Contributor Author

obnoxxx commented Sep 13, 2023

This PR provides a change that should update the s5cmd version as requested in #12883 . I wanted to add a test to verify the effect but could not yet come up with a simple way to test locally with podman run instead of requiring a full k8s deployment

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

LGTM, will merge after you've been able to test it locally.

@obnoxxx
Copy link
Contributor Author

obnoxxx commented Sep 14, 2023

@travisn wrote:

LGTM, will merge after you've been able to test it locally.

Thanks! thanks to all your help yesterday, I just managed to verify locally in minikube:

rook-ceph exec  rook-ceph-operator-b89ccd545-rhh8s -- s5cmd version
v2.2.1-be63977

@obnoxxx
Copy link
Contributor Author

obnoxxx commented Sep 14, 2023

@travisn : I also added a check to the canary integration test workflow to ensure this doesn't regress

@obnoxxx obnoxxx force-pushed the update-s5cmd branch 4 times, most recently from 23af411 to 0ac473d Compare September 14, 2023 11:05
Copy link
Contributor

@subhamkrai subhamkrai left a comment

Choose a reason for hiding this comment

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

commit title, huild h->b

@obnoxxx
Copy link
Contributor Author

obnoxxx commented Sep 14, 2023

@subhamkrai wrote:

commit title, huild h->b

done, thanks!

@obnoxxx obnoxxx force-pushed the update-s5cmd branch 3 times, most recently from d5e6af2 to f59e919 Compare September 14, 2023 16:27
timeout 60 sh -c "until kubectl -n rook-ceph exec $toolbox -- curl --silent --show-error ${mgr_raw%%:*}:9283; do echo 'waiting for mgr prometheus exporter to be ready' && sleep 1; done"

- name: check s5cmd version in the toolbox image
Copy link
Member

Choose a reason for hiding this comment

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

Need to remove this section of the test since we moved it down to line 319. I see the new test in that section is passing, but this section is failing since we know the toolbox-operator-image is not running.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@travisn wrote:

Need to remove this section of the test since we moved it down to line 319. I see the new test in that section is passing, but this section is failing since we know the toolbox-operator-image is not running.

what is it that you are suggesting yo remove? lines 77 -- 79 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried something different now, since I saw the attempt at a diagnostic output of the s5cmd version failing because it could not find the binary ...

Copy link
Member

Choose a reason for hiding this comment

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

We need to remove this whole section, lines 79-88. This CI test runs the different toolbox. The other CI test does run the toolbox operator image so we only can test it on lines 319-328.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@travisn wrote:

We need to remove this whole section, lines 79-88. This CI test runs the different toolbox. The other CI test does run the toolbox operator image so we only can test it on lines 319-328.

got it, thanks. removed.

@obnoxxx obnoxxx force-pushed the update-s5cmd branch 3 times, most recently from 0b7eccf to 2914395 Compare September 15, 2023 13:21
@@ -73,6 +73,7 @@ jobs:
toolbox=$(kubectl get pod -l app=rook-ceph-tools -n rook-ceph -o jsonpath='{.items[*].metadata.name}')
timeout 15 sh -c "until kubectl -n rook-ceph exec $toolbox -- ceph mgr dump -f json|jq --raw-output .active_addr|grep -Eosq \"(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\" ; do sleep 1 && echo 'waiting for the manager IP to be available'; done"
mgr_raw=$(kubectl -n rook-ceph exec $toolbox -- ceph mgr dump -f json|jq --raw-output .active_addr)

Copy link
Member

Choose a reason for hiding this comment

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

revert this blank line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks!

.github/workflows/canary-integration-test.yml Outdated Show resolved Hide resolved
.github/workflows/canary-integration-test.yml Outdated Show resolved Hide resolved
s5cmd_version="$(kubectl -n rook-ceph exec ${toolbox} -- /ust/local/bin/s5cmd version)"
echo ${s5cmd_version} | grep -q "^v2.2.1" || {
echo " Error: the version of s5cmd version in the toolbox is not the expected v2.2.1 but ${s5cmd_version}"

Copy link
Member

Choose a reason for hiding this comment

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

nit: remove blank line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks

This is the latest available version.
The previously used version v2.0.0 was very outdated.

Fixes: rook#12883

Co-authored-by: subham rai subhamkrai <srai@redhat.com>
Signed-off-by: Michael Adam <obnox@samba.org>
Signed-off-by: Subham Rai subhamkrai <srai@redhat.com>
  signed-off-by: Travis Nielsen <tnielsen@redhat.com>
    Co-authored-by: Travis Nielsen <tnielsen@redhat.com>
@obnoxxx
Copy link
Contributor Author

obnoxxx commented Sep 15, 2023

@travisn I think I have addressed all your requests now. Let's see how the check goes this time ...

@obnoxxx
Copy link
Contributor Author

obnoxxx commented Sep 15, 2023

@subhamkrai I think all your requests are addressed and the new test is finally passing! :-)

Please re-review

@travisn travisn merged commit 0bca2ce into rook:master Sep 15, 2023
50 checks passed
travisn added a commit that referenced this pull request Sep 15, 2023
build: update s5cmd to version v2.2.1 (backport #12898)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

s5cmd in the toolbox is downrev
4 participants