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

Fix wrong mutation of virt sc storage class meta #2218

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

akalenyu
Copy link
Contributor

@akalenyu akalenyu commented Oct 17, 2023

We weren't performing the changes on the pointer, and thus name/annotations are not applied.

https://bugzilla.redhat.com/show_bug.cgi?id=2244383

@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 Oct 17, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 17, 2023

Hi @akalenyu. 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.

@malayparida2000
Copy link
Contributor

/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 Oct 17, 2023
@malayparida2000
Copy link
Contributor

@akalenyu Can you please test the changes and confirm they work as intended,

Install kubevirt, Install ODF

Then

oc get storageclass

oc get storageclass ocs-storagecluster-ceph-rbd -o yaml

oc get storageclass ocs-storagecluster-ceph-rbd-virtualization -o yaml

@agarwal-mudit
Copy link
Member

/cherry-pick release-4.14

@openshift-cherrypick-robot

@agarwal-mudit: once the present PR merges, I will cherry-pick it on top of release-4.14 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.14

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.

@agarwal-mudit
Copy link
Member

@akalenyu if this is not merged today then we have to move this to the next release. Can we finish it quickly?
cc @iamniting

@akalenyu
Copy link
Contributor Author

@akalenyu if this is not merged today then we have to move this to the next release. Can we finish it quickly? cc @iamniting

Yes, sure, would I use the CI here to spin up a cluster? or do I get an existing cluster and deploy my build on it?

@agarwal-mudit
Copy link
Member

/cc @nixpanic

@openshift-ci openshift-ci bot requested a review from nixpanic October 17, 2023 13:16
@agarwal-mudit
Copy link
Member

@akalenyu if this is not merged today then we have to move this to the next release. Can we finish it quickly? cc @iamniting

Yes, sure, would I use the CI here to spin up a cluster? or do I get an existing cluster and deploy my build on it?

I don't think we have a cluster, you need to respin @malayparida2000 @iamniting Do you guys have a cluster

@malayparida2000
Copy link
Contributor

@agarwal-mudit I do have a cluster but currently I am outside. But I think the changes apeear to be good enough. I will request @iamniting to review the unit test changes and approve it today if it's urgent enough.

@akalenyu
Copy link
Contributor Author

@agarwal-mudit I do have a cluster but currently I am outside. But I think the changes apeear to be good enough. I will request @iamniting to review the unit test changes and approve it today if it's urgent enough.

I think I figured it out, I have a cluster, where I can edit the CSV and inject my own ocs-operator image. Sounds good?

@malayparida2000
Copy link
Contributor

@agarwal-mudit I do have a cluster but currently I am outside. But I think the changes apeear to be good enough. I will request @iamniting to review the unit test changes and approve it today if it's urgent enough.

I think I figured it out, I have a cluster, where I can edit the CSV and inject my own ocs-operator image. Sounds good?

Yes that sounds good.

@akalenyu
Copy link
Contributor Author

akalenyu commented Oct 17, 2023

$ oc get deploy -n openshift-storage ocs-operator -o yaml | grep image:
        image: quay.io/akalenyu/ocs-operator:latest
# Delete existing storage classes
$ oc get storageclass ocs-storagecluster-ceph-rbd -o yaml
allowVolumeExpansion: true
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  annotations:
    description: Provides RWO Filesystem volumes, and RWO and RWX Block volumes
  creationTimestamp: "2023-10-17T13:35:59Z"
  name: ocs-storagecluster-ceph-rbd
  resourceVersion: "3325649"
  uid: 6d3bd962-1b1e-4d88-99ff-f58d3784acda
parameters:
  clusterID: openshift-storage
  csi.storage.k8s.io/controller-expand-secret-name: rook-csi-rbd-provisioner
  csi.storage.k8s.io/controller-expand-secret-namespace: openshift-storage
  csi.storage.k8s.io/fstype: ext4
  csi.storage.k8s.io/node-stage-secret-name: rook-csi-rbd-node
  csi.storage.k8s.io/node-stage-secret-namespace: openshift-storage
  csi.storage.k8s.io/provisioner-secret-name: rook-csi-rbd-provisioner
  csi.storage.k8s.io/provisioner-secret-namespace: openshift-storage
  imageFeatures: layering,deep-flatten,exclusive-lock,object-map,fast-diff
  imageFormat: "2"
  pool: ocs-storagecluster-cephblockpool
provisioner: openshift-storage.rbd.csi.ceph.com
reclaimPolicy: Delete
volumeBindingMode: Immediate
$ oc get storageclass ocs-storagecluster-ceph-rbd-virtualization -o yaml
allowVolumeExpansion: true
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  annotations:
    description: Provides RWO and RWX Block volumes suitable for Virtual Machine disks
    storageclass.kubevirt.io/is-default-virt-class: "true"
  creationTimestamp: "2023-10-17T13:35:59Z"
  name: ocs-storagecluster-ceph-rbd-virtualization
  resourceVersion: "3325650"
  uid: 5d724954-5d49-4db6-aa35-d6f5a5ee2eaf
parameters:
  clusterID: openshift-storage
  csi.storage.k8s.io/controller-expand-secret-name: rook-csi-rbd-provisioner
  csi.storage.k8s.io/controller-expand-secret-namespace: openshift-storage
  csi.storage.k8s.io/fstype: ext4
  csi.storage.k8s.io/node-stage-secret-name: rook-csi-rbd-node
  csi.storage.k8s.io/node-stage-secret-namespace: openshift-storage
  csi.storage.k8s.io/provisioner-secret-name: rook-csi-rbd-provisioner
  csi.storage.k8s.io/provisioner-secret-namespace: openshift-storage
  imageFeatures: layering,deep-flatten,exclusive-lock,object-map,fast-diff
  imageFormat: "2"
  mapOptions: krbd:rxbounce
  mounter: rbd
  pool: ocs-storagecluster-cephblockpool
provisioner: openshift-storage.rbd.csi.ceph.com
reclaimPolicy: Delete
volumeBindingMode: Immediate

@malayparida2000
Copy link
Contributor

Looks clean, @agarwal-mudit can be approved & merged

Copy link
Contributor

@malayparida2000 malayparida2000 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 openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2023
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.

Can you pls update the commit msg description to have 73 chars per line?

controllers/storagecluster/storageclasses_test.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2023
@akalenyu akalenyu force-pushed the virt-default-sc-ann-fix branch from bc5736e to 8d30aeb Compare October 17, 2023 13:49
We weren't performing the changes on the pointer,
and thus name/annotations are not applied.

Signed-off-by: Alex Kalenyuk <[email protected]>
@akalenyu akalenyu force-pushed the virt-default-sc-ann-fix branch from 8d30aeb to 974985a Compare October 17, 2023 13:53
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 17, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akalenyu, iamniting, malayparida2000

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2023
@iamniting
Copy link
Member

@akalenyu Can you pls add the bug in the description box and get the acks?

@openshift-ci openshift-ci bot merged commit 2f69d25 into red-hat-storage:main Oct 17, 2023
6 checks passed
@openshift-cherrypick-robot

@agarwal-mudit: new pull request created: #2221

In response to this:

/cherry-pick release-4.14

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants