-
Notifications
You must be signed in to change notification settings - Fork 554
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
cleanup: Support matrix update and code cleanup #4262
Conversation
@Madhu-1 @Rakshith-R PTAL. |
Note to self: Link the upstream issue in the next update. |
README.md
Outdated
| | Creating and deleting snapshot | Alpha | >= v3.7.0 | >= v1.1.0 | Pacific (>=16.2.0) | >= v1.17.0 | | ||
| | Provision volume from snapshot | Alpha | >= v3.7.0 | >= v1.1.0 | Pacific (>=16.2.0) | >= v1.17.0 | | ||
| | Provision volume from another volume | Alpha | >= v3.7.0 | >= v1.1.0 | Pacific (>=16.2.0) | >= v1.16.0 | | ||
| RBD | Dynamically provision, de-provision Block mode RWO volume | GA | >= v1.0.0 | >= v1.0.0 | Pacific (>=v16.0.0) | >= v1.14.0 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature Status
need to be updated to change the status to next version based on the support we have (This is for followup PR)
unsupported | ||
) | ||
|
||
type localClusterState struct { | ||
// set the enum value i.e., unknown, supported, | ||
// unsupported as per the state of the cluster. | ||
resizeState operationState | ||
subVolMetadataState operationState | ||
subVolSnapshotMetadataState operationState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove subVolSnapshotMetadataState
as well?
can you also check do we have metadata support in v16.0.0
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
metadata & snapshot metadata support are added on pacific from v16.2.11 and on quincy it is from v17.2.1. Should we keep both these checks for some more time in that case, if someone uses versions lower than that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that is a supported version in ceph we need to keep it as well. ie (< 16.2.11) and (<17.2.1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the docs 16.2.* and 17.2.* are still supported. So we need to keep these checks for some more time.
Will update support matrix as well to point to >=v16.2.0 for all the pacific versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe in ResizeSubVolume
we can remove the check and also remove the call for createVolume. can you please check on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the support for it is present in all the active releases. Removed it now.
@@ -199,16 +199,13 @@ func (s *subVolumeClient) GetSubVolumeInfo(ctx context.Context) (*Subvolume, err | |||
type operationState int64 | |||
|
|||
const ( | |||
unknown operationState = iota | |||
supported | |||
supported operationState = iota | |||
unsupported | |||
) | |||
|
|||
type localClusterState struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localClusterState
might need to be removed as well as we don't require it anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the checks are needed, keeping it as it is for the time being.
internal/cephfs/core/volume.go
Outdated
// ResizeVolume will try to use ceph fs subvolume resize command to resize the | ||
// subvolume. If the command is not available as a fallback it will use | ||
// CreateVolume to resize the subvolume. | ||
// ResizeVolume will use ceph fs subvolume resize command to resize the subvolume. | ||
func (s *subVolumeClient) ResizeVolume(ctx context.Context, bytesQuota int64) error { | ||
newLocalClusterState(s.clusterID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newLocalClusterState(s.clusterID)
also need to be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
aa63d74
to
58a20cb
Compare
c106681
to
82cf777
Compare
/test ci/centos/mini-e2e/k8s-1.28 |
@Rakshith-R @nixpanic @yati1998 PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM!
@karthik-us |
@Rakshith-R just the mention of removal of resize code with the reason suffice in the pending release notes as the desscription for the PR right? under the Breaking Chnages. |
The code change is internal only. |
82cf777
to
f03cbb1
Compare
Pull request has been modified.
Done. |
@Mergifyio rebase |
Updating the support matrix to point to the suppotred ceph release. Fixes: ceph#4168 Signed-off-by: karthik-us <ksubrahm@redhat.com>
The ceph fs subvolume resize support is available in all the active ceph releases. Hence removing the code to check the supportability of the feature. Signed-off-by: karthik-us <ksubrahm@redhat.com>
Adding a note on supported ceph releases. Signed-off-by: karthik-us <ksubrahm@redhat.com>
✅ Branch has been successfully rebased |
f03cbb1
to
e9bcfe5
Compare
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at b5d8bf0 |
/test ci/centos/k8s-e2e-external-storage/1.26 |
/test ci/centos/k8s-e2e-external-storage/1.28 |
/test ci/centos/k8s-e2e-external-storage/1.27 |
/test ci/centos/mini-e2e-helm/k8s-1.26 |
/test ci/centos/upgrade-tests-cephfs |
/test ci/centos/mini-e2e-helm/k8s-1.28 |
/test ci/centos/mini-e2e-helm/k8s-1.27 |
/test ci/centos/upgrade-tests-rbd |
/test ci/centos/mini-e2e/k8s-1.26 |
/test ci/centos/mini-e2e/k8s-1.28 |
/test ci/centos/mini-e2e/k8s-1.27 |
Fixes: #4168