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.

Sign up for GitHub

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

Jump to bottom

Report versioning errors in StorageCluster's Status #1921

Merged
merged 1 commit into from Feb 28, 2023
Merged

Report versioning errors in StorageCluster's Status #1921

merged 1 commit into from Feb 28, 2023

Conversation

Nikhil-Ladha
Copy link
Member

@Nikhil-Ladha Nikhil-Ladha commented Jan 23, 2023 •
edited

Update StorageCluster status on versioning error. Append an error status condition to StorageCluster status field when an error arises during versionCheck.

And, modified the hack scripts to update the VERSION env during the binary build.

Credits to #1447

NOTE
This PR is just a rebase with a couple of changes from the original PR

Signed-off-by: Nikhil-Ladha nikhilladha1999@gmail.com
Co-authored-by: Pranshu Srivastava rexagod@gmail.com

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 23, 2023

Hi @Nikhil-Ladha. Thanks for your PR.

I'm waiting for a red-hat-storage member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 23, 2023
@Nikhil-Ladha
Copy link
Member Author

I don't think we can make the "Version" field empty just now, as the downstream docker images don't have the "Version" env set during the build of the standalone ocs-operator images. So, if we remove them then the images would have the "Version" field empty in them.

@Nikhil-Ladha Nikhil-Ladha marked this pull request as draft January 23, 2023 12:36
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 23, 2023
@Nikhil-Ladha
Copy link
Member Author

Marking it as draft until I have tested these changes.

@Nikhil-Ladha Nikhil-Ladha force-pushed the bz-185539/update-version branch 3 times, most recently from 4db72cb to b388e64 Compare February 3, 2023 10:46
@Nikhil-Ladha Nikhil-Ladha marked this pull request as ready for review February 6, 2023 07:33
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 6, 2023
@Nikhil-Ladha
Copy link
Member Author

Tested it out on the latest 4.99 build and the version field for storagecluster is getting updated now.

(ocs_env) [nladha@nladha ocs-ci]$ oc get csv -n openshift-storage
NAME                                               DISPLAY                       VERSION                   REPLACES                          PHASE
mcg-operator.v4.99.1-38.custom-nikhil              NooBaa Operator               4.99.1-38.custom-nikhil   mcg-operator.v4.12.0              Succeeded
ocs-operator.v4.99.1-38.custom-nikhil              OpenShift Container Storage   4.99.1-38.custom-nikhil   ocs-operator.v4.12.0              Succeeded
odf-csi-addons-operator.v4.99.1-38.custom-nikhil   CSI Addons                    4.99.1-38.custom-nikhil   odf-csi-addons-operator.v4.12.0   Succeeded
odf-operator.v4.99.1-38.custom-nikhil              OpenShift Data Foundation     4.99.1-38.custom-nikhil                                     Succeeded
(ocs_env) [nladha@nladha ocs-ci]$ oc get storagecluster -n openshift-storage
NAME                 AGE   PHASE   EXTERNAL   CREATED AT             VERSION
ocs-storagecluster   14m   Ready              2023-02-06T07:19:00Z   4.99.1

@Nikhil-Ladha
Copy link
Member Author

If we intend to backport it to older releases we need to make one small change in the build scripts downstream.

@Nikhil-Ladha
Copy link
Member Author

@umangachapagain @iamniting can you please review the PR?
TIA :)

@iamniting
Copy link
Member

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 6, 2023
iamniting
iamniting requested changes Feb 6, 2023
View reviewed changes
Copy link
Member

@iamniting iamniting left a comment

Choose a reason for hiding this comment

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

@umanga whats your take on this?

controllers/storagecluster/reconcile.go Outdated Show resolved Hide resolved
@Nikhil-Ladha Nikhil-Ladha force-pushed the bz-185539/update-version branch 3 times, most recently from 16c15c7 to 1dbee14 Compare February 7, 2023 06:46
@Nikhil-Ladha
Copy link
Member Author

How to resolve the permission denied issue for the newly added hack/unit-test.sh file?
Also, I have rebased and run the make gen-latest-csv cmd and there are not updates in the CSV but it still complains that "it is not the latest csv", any idea?

@iamniting
Copy link
Member

How to resolve the permission denied issue for the newly added hack/unit-test.sh file?

chmod +x hack/unit-test.sh

Also, I have rebased and run the make gen-latest-csv cmd and there are not updates in the CSV but it still complains that "it is not the latest csv", any idea?

will look and let you know

@Nikhil-Ladha Nikhil-Ladha force-pushed the bz-185539/update-version branch 4 times, most recently from d05194d to 9c2be2d Compare February 7, 2023 08:40
@Nikhil-Ladha
Copy link
Member Author

Nikhil-Ladha commented Feb 7, 2023 •
edited

How to resolve the permission denied issue for the newly added hack/unit-test.sh file?

chmod +x hack/unit-test.sh

Also, I have rebased and run the make gen-latest-csv cmd and there are not updates in the CSV but it still complains that "it is not the latest csv", any idea?

will look and let you know

Ok, I seem to have understood the issue with the failing tests.
I think the use of VERSION variable in our hack scripts is contradicting with a VERSION env variable in the prow env. The later is taking priority and updating the value to 1.18 which seems like the golang version being used.
So, I have updated the code to use CSV_VERSION instead of VERSION env var.

I am not exactly sure if this could lead to any issue in future, i.e, if we use different values of VERSION and CSV_VERSION anywhere 🤔

@Nikhil-Ladha Nikhil-Ladha force-pushed the bz-185539/update-version branch 2 times, most recently from e75bdc4 to e5d2342 Compare February 20, 2023 13:29
@Nikhil-Ladha Nikhil-Ladha requested review from umangachapagain and iamniting and removed request for iamniting and umangachapagain February 21, 2023 05:31
@Nikhil-Ladha Nikhil-Ladha requested review from iamniting and removed request for umangachapagain February 21, 2023 14:05
@Nikhil-Ladha Nikhil-Ladha force-pushed the bz-185539/update-version branch 2 times, most recently from a73ddb7 to 655e0f6 Compare February 21, 2023 14:42
iamniting
iamniting requested changes Feb 21, 2023
View reviewed changes
controllers/util/status.go Outdated Show resolved Hide resolved
iamniting
iamniting requested changes Feb 23, 2023
View reviewed changes
controllers/util/status.go Outdated Show resolved Hide resolved
controllers/util/status.go Outdated Show resolved Hide resolved
@Nikhil-Ladha @rexagod
Report versioning errors in StorageCluster's Status
17a9887 
Update StorageCluster status on versioning error. Append an
`VersionMismatch` status condition to StorageCluster status field
when an error arises during `versionCheck`.

And, modified the hack scripts to update the VERSION env during the
binary build.

Co-authored-by: Pranshu Srivastava <rexagod@gmail.com>
Signed-off-by: Nikhil-Ladha <nikhilladha1999@gmail.com>
iamniting
iamniting approved these changes Feb 23, 2023
View reviewed changes
Copy link
Member

@iamniting iamniting left a comment

Choose a reason for hiding this comment

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

looks good to me. Holding for @umangachapagain to review.

/hold

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 23, 2023
@umangachapagain
Copy link
Contributor

/unhold

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 28, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, Nikhil-Ladha, umangachapagain

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:
  • OWNERS [iamniting,umangachapagain]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2023
@openshift-merge-robot openshift-merge-robot merged commit 445ffb7 into red-hat-storage:main Feb 28, 2023
@Nikhil-Ladha Nikhil-Ladha deleted the bz-185539/update-version branch October 3, 2023 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iamniting iamniting iamniting approved these changes

@umangachapagain umangachapagain umangachapagain approved these changes

Assignees

@umangachapagain umangachapagain

@iamniting iamniting

Labels
4.13-feature Indicates the PR's required to go to main before feature freeze of release 4.13 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Nikhil-Ladha @iamniting @umangachapagain @openshift-merge-robot