Rook PR https://github.com/rook/rook/pull/4409
OCS-operator PR https://github.com/openshift/ocs-operator/pull/352

While the resizer is not officially supported in the release, I would push back on this change for several reasons:
- We have a downstream resizer available so we are not unexpectedly shipping an upstream image
- This late in the release cycle we should only be taking blockers. This is a nice-to-have, not a blocker. No functionality is blocked, nothing crashes because it is enabled, no negative consequence for shipping it. 
- Any code change this late in the release is a risk. Do we really want to risk a regression after we have already been delayed?
- The feature is being widely used upstream. This doesn't mean we support it, but it means lower risk if someone does use the unsupported feature.

Alternatively to removing the resizer, can we consider sanity testing the resizer and say the feature is experimental? This is a really popular feature.

So while the resizer container is not needed for ocs 4.3 since we don't support the feature yet, the question is whether it does any harm. 

With the feature not supported in OCS 4.3 yet, one danger might be that customers could end up using it just because the component is there. PV resize is tech preview in OCP 4.3 (and 4.4. too, and will be in 4.5). However, the feature is enabled by default. https://github.com/openshift/origin/blob/release-4.3/vendor/k8s.io/kubernetes/pkg/features/kube_features.go#L539.

Per my understanding, not having the resizer container running would be a way to disable the feature. But do we need to block 4.3 to remove this container?

Just consider that we now also have an e2e test case for resizing under review:  https://github.com/red-hat-storage/ocs-ci/pull/1778

So my proposal is: Could we consider just having it for 4.3 (and not mentioning it...) and have it happy-path tested as TP for 4.4 which is supposed to come very soon?

(In reply to Travis Nielsen from comment #8)
> While the resizer is not officially supported in the release, I would push
> back on this change for several reasons:
> - We have a downstream resizer available so we are not unexpectedly shipping
> an upstream image
> - This late in the release cycle we should only be taking blockers. This is
> a nice-to-have, not a blocker. No functionality is blocked, nothing crashes
> because it is enabled, no negative consequence for shipping it. 
> - Any code change this late in the release is a risk. Do we really want to
> risk a regression after we have already been delayed?
> - The feature is being widely used upstream. This doesn't mean we support
> it, but it means lower risk if someone does use the unsupported feature.

Agree to all the above.

> Alternatively to removing the resizer, can we consider sanity testing the
> resizer and say the feature is experimental? This is a really popular
> feature.

Indeed! As mentioned, an automated test case is being proposed to the ocs-ci: https://github.com/red-hat-storage/ocs-ci/pull/1778

@travis

Then, with the resizer container still present,  will the scenario for Comment#7 hold & ceph PVs can get resized from UI? Also, this new size will be visible from both OCP and ceph backend ?

We haven't tested PV resize in OCS 4.3, hence wanted to confirm the implications.

A code change so late in release cycle is risky though :)

(In reply to Neha Berry from comment #11)
> @travis
> 
> Then, with the resizer container still present,  will the scenario for
> Comment#7 hold & ceph PVs can get resized from UI?

Yes.

> Also, this new size will be visible from both OCP and ceph backend ?

AFAICT, yes.

> We haven't tested PV resize in OCS 4.3, hence wanted to confirm the implications.
> 
> A code change so late in release cycle is risky though :)

That's why the proposal to do the automated ci-test and call it experimental (or so). Maybe TP in 4.4?

As just decided in the PgM meeting: We are not going to do anything for 4.3 here.
The feature can only be used of the storage class needs to be changed to use this.
So moving out of 4.3.
I think we should also close this since we instead of removing the feature we will strive to support it.

Removing needsinfo since Michael answered the questions.

Hi Michael,

PV resize will not be supported in OCS4.4 so we should fix this for 4.4

@Elad There are many features that are supported upstream that are not supported downstream. Why is it so critical to disable this feature? In 4.5 we want to support it anyway. Maybe that means we should say it is tech preview now.

This is not critical to resolve for OCS 4.4, and we should not be doing any further feature development for that release anyway. It is harming nothing, and cannot be accessed without considerable configuration on the admin's part. In particular, PV resizing is not even supported in OCP 4.4 anyway, and the downstream CSI sidecar images should reflect this accordingly. Since we intend to support it anyway, it is a waste of time to disable it for a matter of weeks. At best we should document that we explicitly don't support this.

This feature was never tested so the tech preview suggestion is good but off the table.
The fact we are exposing a feature that will land officially in the product in future release doesn't mean we can jeopardize the existing release we are working on.

Just to put things is perspective,
- OCS 4.4 should be released on May 6th and OCS 4.5 should be released on mid-July - 2.5 months between the releases
- The resizer image is not a downstream iiuc and therefore should not be part of OCS (a downstream product)
- PV resize as a tech preview in 4.4 is too late for ask at this point as we are 2 weeks before releasing 4.4 and we don't want to put this date at risk in case we will find more bugs around that area.

This needs to be addressed in 4.4

I respectfully disagree and will not be giving devel_ack+. As you said, we are two weeks away from releasing 4.4 and don't want to make such a big change to the code for something that is IMO as trivial as this. The resize won't work as-is, even if you configure a StorageClass to use it it will be ignored by the platform.

Thanks Jose,

I have a few questions:
1) Comment #9 states that pv resize is a tech preview in 4.3 (which is not) - does it mean this feature will work from UI? I know that we reported a bg to disable it and it was actually passed verification (by me)
2) If this staying with use until 4.5, does it mean the feature is again enabled in 4.4?
3) How easy is it to perform PV resize at this point? - I hope your answer will be impossible as it shouldn't be part of any release before 4.5

(In reply to Jose A. Rivera from comment #19)
> I respectfully disagree and will not be giving devel_ack+. As you said, we
> are two weeks away from releasing 4.4 and don't want to make such a big
> change to the code for something that is IMO as trivial as this. The resize
> won't work as-is, even if you configure a StorageClass to use it it will be
> ignored by the platform.

Above is not correct. If you adjust the storage class as mentioned earlier, resize *will*
work. I dont see any reason for its to NOT work in an OCS 4.4 setup. With that,
we are unwantedly opening up a slot for admins/users to try something. Yes, 
we can document it, but I dont know how effective it is?. 

Regarding this features' support state in future versions like OCP/OCS 4.5, even though it is Beta' at this stage, there is uncertainty on when this will be GA'd. 
There are some corner issues ( for example, you can not cancel a `failed` resize operation..etc) which 
upstream is still chasing on and trying to reach consensus, which has *not* yet happened.
So we never know when this will be promoted to GA at all. With all that, my take here is
this sidecar has to be disabled in OCS 4.4.

(In reply to Humble Chirammal from comment #21)
> (In reply to Jose A. Rivera from comment #19)
> > I respectfully disagree and will not be giving devel_ack+. As you said, we
> > are two weeks away from releasing 4.4 and don't want to make such a big
> > change to the code for something that is IMO as trivial as this. The resize
> > won't work as-is, even if you configure a StorageClass to use it it will be
> > ignored by the platform.
> 
> Above is not correct. If you adjust the storage class as mentioned earlier,
> resize *will*
> work.

That's the point with "as is": I understand that as "out of the box".
AFAIK, in OCS 4.4, we only support the default storage classes that are generated at install.
And those storage classes don't have the resize enabled.

So it does not work out of the box. You have to change the storageclass to enable it.
With that, I think it's safe to leave this as is.

I would like to close this bug as WONTFIX.

@Raz, @Elad?


> I dont see any reason for its to NOT work in an OCS 4.4 setup. With
> that,
> we are unwantedly opening up a slot for admins/users to try something. Yes, 
> we can document it, but I dont know how effective it is?. 
> 
> Regarding this features' support state in future versions like OCP/OCS 4.5,
> even though it is Beta' at this stage, there is uncertainty on when this
> will be GA'd. 
> There are some corner issues ( for example, you can not cancel a `failed`
> resize operation..etc) which 
> upstream is still chasing on and trying to reach consensus, which has *not*
> yet happened.
> So we never know when this will be promoted to GA at all. With all that, my
> take here is
> this sidecar has to be disabled in OCS 4.4.

Hi Michael,

I don't understand why this is so important to leave this as part of 4.4?
Why we can't just disable this sidecar?

To start, we cannot simply do a configuration to disable the sidecar, this required code change in the Ceph-CSI drivers. The only configuration we can do is not specify a container image, but then it would default to the upstream version: https://github.com/rook/rook/blob/master/pkg/operator/ceph/csi/spec.go#L293-L299

One more issue is that this is not something we want to do in the upstream. We don't want to disable the resizer sidecar just because the downstream product doesn't want it, so it would have to be a downstream-only patch. Even if we do that, it sounds like it would be a tricky code change to do in the Ceph-CSI drivers, and it may be a big enough change to introduce unwanted risk.

Madhu, Humble, does that sound fair?

And our point still stands: this is a non-issue. Nothing is being affected, and the feature can't really be used even if they try.

(In reply to Jose A. Rivera from comment #24)
> To start, we cannot simply do a configuration to disable the sidecar, this
> required code change in the Ceph-CSI drivers. The only configuration we can
> do is not specify a container image, but then it would default to the
> upstream version:
> https://github.com/rook/rook/blob/master/pkg/operator/ceph/csi/spec.go#L293-
> L299
> 
> One more issue is that this is not something we want to do in the upstream.
> We don't want to disable the resizer sidecar just because the downstream
> product doesn't want it, so it would have to be a downstream-only patch.
> Even if we do that, it sounds like it would be a tricky code change to do in
> the Ceph-CSI drivers, and it may be a big enough change to introduce
> unwanted risk.
> 
Yes, to disable it we have to do a lot of code changes in cephcsi(specific to downstream).

if we want to really disable it better to remove the deployment on resizer sidecar in rook downstream

the code changes will be minimal in rook compare to cephcsi.


> Madhu, Humble, does that sound fair?
> 
> And our point still stands: this is a non-issue. Nothing is being affected,
> and the feature can't really be used even if they try.

(In reply to Jose A. Rivera from comment #24)
> To start, we cannot simply do a configuration to disable the sidecar, this
> required code change in the Ceph-CSI drivers. The only configuration we can
> do is not specify a container image, but then it would default to the
> upstream version:
> https://github.com/rook/rook/blob/master/pkg/operator/ceph/csi/spec.go#L293-
> L299

Jose, I dont think we ever considered or pointing to disable it in the code path of 'Ceph CSI' 
or it shouldnt be way as long as the intention here is to 'block' the request reaching the resizer and beyond. 
It was not the method we followed when we disabled snapshotter side car too. The 
change was in rook template and OCS operator to set the knob.

> 
> One more issue is that this is not something we want to do in the upstream.
> We don't want to disable the resizer sidecar just because the downstream
> product doesn't want it, so it would have to be a downstream-only patch.

yeah, indeed thats the plan if we have ack here.

> Even if we do that, it sounds like it would be a tricky code change to do in
> the Ceph-CSI drivers, and it may be a big enough change to introduce
> unwanted risk.
> 
> Madhu, Humble, does that sound fair?

As mentioned above, disabling in CSI is not the way we are referring to.

> 
> And our point still stands: this is a non-issue. Nothing is being affected,
> and the feature can't really be used even if they try.

As referenced in my previous comment, if admin wants 3 extra params can enable it for him. But, yeah 
he is going out of support knowingly.

(In reply to Raz Tamir from comment #23)
> Hi Michael,
> 
> I don't understand why this is so important to leave this as part of 4.4?
> Why we can't just disable this sidecar?

It is not important to keep it in 4.4.
The point is (summarizing the above comments):

1) It is not easy to disable the sidecar, it would mean several possibly bigger code changes across various components.
2) These code changes are only going to be downstream, which is something that we want to usually avoid, because it will make future backports a lot more difficult.

Such bigger, downstream-only patches are risky. So we do not want to keep the sidecar, but we want to avoid the risk and work.

So yes, the functionality would be there, but in order to use it, the customer has to consciously and actively *edit the storage class* to an unsupported state.
I don't see that we have a severe problem here.

ok,

removing the blocker flag

Moving to 4.5 for a final discussion / decision what to do in 4.5+ :

I don't think we should ever do this as

1) the patch is not trivial
2) the patch will be downstream only
    ==> I.e. further patch backports will potentially be a lot more work
3) for the time being at least, we are protected by only supporting the default created storage classes and defaulting to resize not supported

==> I suggest to close this as WONTFIX

@Humble, @Madhu - what do you think?

(In reply to Michael Adam from comment #29)
> Moving to 4.5 for a final discussion / decision what to do in 4.5+ :
> 
> I don't think we should ever do this as
> 
> 1) the patch is not trivial
> 2) the patch will be downstream only
>     ==> I.e. further patch backports will potentially be a lot more work
> 3) for the time being at least, we are protected by only supporting the
> default created storage classes and defaulting to resize not supported
> 
> ==> I suggest to close this as WONTFIX
> 
> @Humble, @Madhu - what do you think?

If our support statement is, we support *only* the *default* storage class created by the "ocs-operator" ( without any manual tweaking), we are good.

We don't need to address this in OCS 4.5.

https://github.com/openshift/ocs-operator/pull/488

Above PR could add the required secret to the default SC, but still `allowVolumeExpansion` is `false` by default.

Closing this BZ - we want the resize functioality, TP or not, in 4.5.