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

must-gather: add helper pod yaml with compatible secret for <4.6 #977

Merged

Conversation

crombus
Copy link
Contributor

@crombus crombus commented Jan 6, 2021

toolbox has secrets updated for 4.6. ROOK_CEPH_USERNAME and ROOK_CEPH_SECRET replacing ROOK_ADMIN_SECRET. So when must-gather creates helper pod with updated secrets it cannot used for version before 4.6. To make must-gather compatible with all existing and upcoming version two yamls has to maintained one with ROOK_ADMIN_SECRET secret and one which is latest. based on the version must-gather will deploy helper pod.
Signed-off-by: crombus pkundra@redhat.com

@rajatsing
Copy link

Can you please update the commit message and description of the PR with some more context, you can write about why you're making this change, the reason being it will help me understand the code even better and more logically.
A short summary of the changes in the code would be awesome!.

@rajatsing rajatsing self-requested a review January 6, 2021 09:03
if [[ oc get csv -n "${ns}" --no-headers | awk '{print $5}' -gt 4.5.0 ]]; then
apply_latest_helper_pod "$ns" "$operatorImage"
else
apply_helper_pod "$ns" "$operatorImage"

Choose a reason for hiding this comment

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

just a naming suggestion and I feel it might be subjective but, instead of naming apply_helper_pod, change it to apply_standard_helper_pod since latest means when OCS version is > 4.5.0 and standard means the usual pod when the OCS version is <= 4.5.0.
What do you think?,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes naming has to be correct. How about we keep the latest for 4.5.0> and standard for 4.6.0<.

Choose a reason for hiding this comment

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

Yes
That would make much more sense

@crombus crombus force-pushed the backward_compatibility branch 3 times, most recently from 3831215 to 5644da6 Compare January 6, 2021 13:13
@crombus crombus requested a review from rajatsing January 6, 2021 13:13
@@ -107,7 +113,11 @@ for ns in $namespaces; do
if [ "${operatorImage}" = "" ]; then
echo "not able to find the rook's operator image. Skipping collection of ceph command output" | tee -a "${BASE_COLLECTION_PATH}"/gather-debug.log
elif [[ $cephClusterCount -gt 0 ]]; then
apply_helper_pod "$ns" "$operatorImage"
if [[ $(oc get csv -n "${ns}" --no-headers | awk '{print $5}') -gt 4.5.0 ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

What about 4.5.z (e.g. 4.5.1/4.5.2)? Will this condition cover that too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, updated

add helper pod yaml with compatible secret for version <4.6
and change naming of templates according to the versions
>=4.6 will be latest and <4.6 will be standard.

Signed-off-by: crombus <pkundra@redhat.com>
Copy link
Member

@agarwal-mudit agarwal-mudit left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2021
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: agarwal-mudit, iamniting, rajatsing

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:

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2021
@agarwal-mudit
Copy link
Member

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 18, 2021

@crombus: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/red-hat-storage-ocs-ci-e2e-aws 4038a7a link /test red-hat-storage-ocs-ci-e2e-aws

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.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit a3be603 into red-hat-storage:master Jan 18, 2021
@crombus crombus deleted the backward_compatibility branch January 18, 2021 07:19
@crombus
Copy link
Contributor Author

crombus commented Jan 18, 2021

/cherry-pick release-4.7

1 similar comment
@crombus
Copy link
Contributor Author

crombus commented Jan 18, 2021

/cherry-pick release-4.7

@openshift-cherrypick-robot

@crombus: new pull request created: #996

In response to this:

/cherry-pick release-4.7

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.

@crombus
Copy link
Contributor Author

crombus commented Jan 18, 2021

/cherry-pick release-4.6

@openshift-cherrypick-robot

@crombus: new pull request created: #997

In response to this:

/cherry-pick release-4.6

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants